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.

Sign up for GitHub

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

Jump to bottom

EmbedThumbnailPP: add support for WAVE and AIFF files #11485

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

EmbedThumbnailPP: add support for WAVE and AIFF files #11485

wants to merge 2 commits into from

Conversation

7x11x13
Copy link

@7x11x13 7x11x13 commented Nov 9, 2024

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Implements #11481

I didn't see any tests for EmbedThumbnailPP so I did not add any tests, but I did some end-to-end testing myself to check that it works.

Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

@7x11x13
Copy link
Author

7x11x13 commented Nov 9, 2024

I'm not sure why some of the CI tests are failing, I don't think it's related to my changes though?

@bashonly
Copy link
Member

bashonly commented Nov 9, 2024 •
edited
Loading

I'm not sure why some of the CI tests are failing, I don't think it's related to my changes though?

It's the new version of websockets' fault, not yours. You can ignore the failing CI

EDIT: Merged master into this branch, since seeing actual core test results may be useful for a PR like this

P.S. note that I merged instead of rebased; we try to avoid force-pushing to PRs, especially after reviewing has begun

@seproDev seproDev added the enhancement New feature or request label Nov 9, 2024
@7x11x13
Copy link
Author

7x11x13 commented Dec 14, 2024

This seems to have problems when combined with --embed-metadata for WAV files, because we use FFmpeg to write RIFF metadata for WAV files and mutagen can only read/write ID3 tags embedded in RIFF chunks (see quodlibet/mutagen#207). IMO, the best solution would be to write ID3 tags for WAV files via mutagen (if it's available) instead of using FFmpeg. ID3 embedded in RIFF is pretty well supported in my experience, and ID3 supports more tags (including cover art).

@7x11x13
Copy link
Author

7x11x13 commented Dec 15, 2024

IMO, the best solution would be to write ID3 tags for WAV files via mutagen (if it's available) instead of using FFmpeg. ID3 embedded in RIFF is pretty well supported in my experience, and ID3 supports more tags (including cover art).

Made a PR for this solution here: #11817

This PR shouldn't be looked at until that one is

@7x11x13 7x11x13 marked this pull request as draft December 15, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@7x11x13 @bashonly @seproDev