Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add operation to generate CUE files. #3620

Merged
merged 24 commits into from
Jan 10, 2024
Merged

Add operation to generate CUE files. #3620

merged 24 commits into from
Jan 10, 2024

Conversation

smimram
Copy link
Member

@smimram smimram commented Jan 9, 2024

Fixes #3608.

Copy link
Member

@toots toots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been waiting for this! Thanks!

Couple of remarks:

  • I think this should go on the extra lib
  • Are you interested in extracting the time split logic into its own function (provided we don't have one already!)
  • Can we add test to see what the file looks like?
  • Can we add parameters to make it work with infinite sources? Like keeping the last n entries? Could be disabled by default.
  • Can we make file change atomic? Otherwise you will run into situations where a partial file might be read. This happened with HLS outputs. You could factor out this code from output.hls:
          Fun.protect
            ~finally:(fun () -> try Sys.remove tmp_file with _ -> ())
            (fun () ->
              (try Unix.rename tmp_file filename
               with Unix.Unix_error (Unix.EXDEV, _, _) ->
                 self#log#important
                   "Rename failed! Directory for temporary files appears to be \
                    on a different file system. Please set it to the same one \
                    using `temp_dir` argument to guarantee atomic file \
                    operations!";
                 Utils.copy ~mode ~perms tmp_file filename;
                 Sys.remove tmp_file);

src/libs/list.liq Outdated Show resolved Hide resolved
src/libs/source.liq Outdated Show resolved Hide resolved
@smimram
Copy link
Member Author

smimram commented Jan 9, 2024

I think I have taken most of your suggestions in account. I propose that we merge as is and improve if usecases do show up...

@toots toots merged commit e536a57 into main Jan 10, 2024
24 of 26 checks passed
@toots toots deleted the cue-file branch January 10, 2024 22:41

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smimram: Looks like you assume hours:minutes:seconds for the INDEX times, but they are actually MM:SS:FF (minutes, seconds, frames). Hydrogenaudio tells us there are 75 frames to a second: https://wiki.hydrogenaud.io/index.php?title=Cue_sheet#Most_often_used

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frames are numbered 00…74.

@Moonbase59
Copy link

@toots @smimram Please check 50da0a3#r1448803161 (MM:SS:FF INDEX times).

@smimram
Copy link
Member Author

smimram commented Jan 11, 2024

Thanks for the comments, I am starting a bugfix session on #3624...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible to record live stream including a .cue file?
3 participants