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 release date to track metadata #223

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Conversation

chrislo
Copy link
Member

@chrislo chrislo commented Oct 2, 2024

This PR reworks #222 to get it ready to merge.

@lenikadali - thanks so much for your hard work on this! I've made a couple of small changes:

  • I squashed your commits together and added a bit of explanation in the commit message to document what you and @floehopper discussed about metadata field names
  • I added a more "end to end" test that extracts the metadata using ffprobe to make it a bit easier for us to change the metadata fields in the future and have some confidence that ffmpeg is doing the right thing.

lenikadali and others added 2 commits October 2, 2024 16:51
Fixes #185

This commit adds the DATE metadata field to the transcoded MP3 and
FLAC files. The FLAC documentation we can find seems to suggest using
DATE for this purpose[1]. For MP3, `date` also seems to be an
acceptable field name[2]. We've verified that the data shows up in the
metadata displayed by VLC[3] for both formats.

[1] https://xiph.org/vorbis/doc/v-comment.html#fieldnames
[2] https://docs.mp3tag.de/format/
[3] https://www.videolan.org/vlc/

Co-authored-by: Chris Lowis <[email protected]>
This test runs a WAV file through the TranscodeJob and then ensures
that the correct metadata has been added to the generated MP3 and FLAC
file using `ffprobe`.
@chrislo chrislo marked this pull request as ready for review October 2, 2024 16:02
@chrislo
Copy link
Member Author

chrislo commented Oct 2, 2024

I'm pretty happy with where this has got to, so I plan to land it tomorrow. I don't think it's worth re-transcoding every file currently in the system to have the new metadata field, but it is something we could do - I don't think the background job machine is working very hard generally so it might be practical to retranscode to keep everything consistent.

@lenikadali
Copy link
Contributor

@chrislo thanks a lot for getting to this as promised 🙌🏿

I'm pretty happy with where this has got to, so I plan to land it tomorrow. I don't think it's worth re-transcoding every file currently in the system to have the new metadata field, but it is something we could do - I don't think the background job machine is working very hard generally so it might be practical to retranscode to keep everything consistent.

Makes sense. In any case, it looks like something we can revisit should we find that it is having a disproportionate impact on performance.

Our best understanding is that the MP3 ID3 tag TORY[1] should be used to
store the *year* (i.e. as a 4-digit number) the music was
released. The convention for FLAC tags seems to have even less
consensus/real-world usage, so for simplicity in the code, we've
decided to use the four digit year in the DATE tag.

[1] https://community.mp3tag.de/t/how-do-i-add-the-tag-for-when-my-music-was-released/55319

Co-authored-by: James Mead <[email protected]>
@chrislo chrislo merged commit 980f2e5 into main Oct 3, 2024
2 checks passed
@chrislo chrislo deleted the add-release-date-to-track-metadata branch October 3, 2024 09:57
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.

3 participants