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 cover manager #3651

Merged
merged 13 commits into from
Jan 22, 2024
Merged

Add cover manager #3651

merged 13 commits into from
Jan 22, 2024

Conversation

toots
Copy link
Member

@toots toots commented Jan 22, 2024

Add an extra API that facilitates the handling of covers to be used in video streams. Original code by @vitoyucepi

@toots
Copy link
Member Author

toots commented Jan 22, 2024

@Moonbase59 do you know who authored this initially?

@toots toots changed the base branch from main to chatgpt January 22, 2024 01:37
@toots toots force-pushed the chatgpt branch 2 times, most recently from 03a348e to eb4acf7 Compare January 22, 2024 02:01
Base automatically changed from chatgpt to main January 22, 2024 02:01
@Moonbase59
Copy link

Moonbase59 commented Jan 22, 2024

@toots No, I don’t know for sure, but I think it was @vitoyucepi – see #3402 (reply in thread), #3402 (comment) and #3402 (reply in thread).

src/libs/extra/metadata.liq Outdated Show resolved Hide resolved
@toots toots enabled auto-merge January 22, 2024 06:40
src/libs/extra/metadata.liq Outdated Show resolved Hide resolved
Comment on lines 24 to 36
def set_current_cover(filename) =
current_cover = current_cover_file()
if
current_cover != default
then
log.info(
label=id,
"Removing #{string.quote(current_cover)}"
)
file.remove(current_cover)
end
current_cover_file := filename
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if current_cover_file might point to a non-existent file after removing the previous one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we can inverse the order of operations to avoid a race condition.

@toots toots added this pull request to the merge queue Jan 22, 2024
@@ -173,14 +173,21 @@ let _ =
"Return a fresh temporary filename. The temporary file is created empty, \
with permissions 0o600 (readable and writable only by the file owner)."
[
( "dir",
Copy link
Member

Choose a reason for hiding this comment

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

I don't like abbreviations in argument names

Suggested change
( "dir",
( "directory",

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry just seeing this. Will apply to main.

("", Lang.string_t, None, Some "File prefix");
("", Lang.string_t, None, Some "File suffix");
]
Lang.string_t
(fun p ->
let temp_dir =
Lang.to_valued_option Lang.to_string (List.assoc "dir" p)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Lang.to_valued_option Lang.to_string (List.assoc "dir" p)
Lang.to_valued_option Lang.to_string (List.assoc "directory" p)

Comment on lines +1 to +4
# Store and retrieve file covers using metadata. This returns a set of
# getter/setter methods that can be used to store and retrieve cover art.
# Typical usage is to set cover art in a `on_metadata` handler and retrieve
# it in a `video.add_image` operator.
Copy link
Member

Choose a reason for hiding this comment

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

Since this can be a bit complicated for some users, it would be nice to add another source.video_from_cover operator based on this which takes an audio track and adds a video track based on the cover!

Merged via the queue into main with commit 16d6962 Jan 22, 2024
26 checks passed
@toots toots deleted the cover-manager branch January 22, 2024 09:06
toots added a commit that referenced this pull request Jan 22, 2024
Co-authored-by: Samuel Mimram <[email protected]>
Co-authored-by: Vito <[email protected]>
@toots toots mentioned this pull request Feb 5, 2024
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.

4 participants