PICARD-2616: id3v2.4 TDRL -> releasedate mapping

[certuna]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other

Picard currently cannot read or write the official Release Time (TDRL) frame in id3v2.4 due to a missing mapping of TDRL to the internal variable releasedate, like it currently has with TDRC -> date and TDOR -> originaldate.

This PR fixes it and provides a graceful fallback + upgrade path for id3v2.3.

This make no changes to any default behaviour of Picard. It only adds one internal variable, available to scripting

Problem

Read:
Picard does not read the TDRL frame in id3v2.4 & does not show it in the list of tags.
With Vorbis Comments, Picard does read the (equivalent) RELEASEDATE tag

Write:
When Picard is instructed to write the variable releasedate, for example in this script: $set(releasedate,%date%), the following happens:

1. With id3v2.4, it currently creates a custom frame TXXX:releasedate and not TDRL. While this is compliant with id3v2.3 (which does not have the TDRL frame defined), this is not compliant with id3v2.4, where it is part of the official standard: id3v2.4 paragraph 4.2.5

2. With Vorbis Comments, the RELEASEDATE frame is written correctly, albeit without date sanitation.

Most other tag editors (mp3tag, mutagen, Yate, PuddleTag, kid3, ffmpeg, TagLib, etc) have a mapping of the internal variable releasedate <-> TDRL, Picard is the only modern tag editor I'm aware of that is not able to write (or even read!) this frame.

• JIRA ticket (optional): PICARD-2616

Solution

I have deliberately not touched any of the default MusicBrainz JSON -> Picard tag mappings, as I understand from my conversation with Philipp Wolfer that it is a deliberate decision that Picard does not write anything to the TDRL frame in its default settings, as to not upset people's existing workflows. However, this mapping at least makes the releasedate internal variable accessible to scripts that do.

• added a TDRL -> releasedate mapping for id3v2.4
• added an explicit TXXX:releasedate custom frame fallback for id3v2.3 + upgrade path for v2.3 -> v2.4
• added date sanitation of the releasedate field for Vorbis Comments, like Picard already does with originaldate and date

Note: id3v2.3 fallback is done in the same way Picard currently handles Mood (TMOO) which is another frame that exists in v2.4 but not v2.3

Action

Please review & comment

[rdswift]

One more suggestion... You might want to be a bit more descriptive in your commit messages. I suggest having a look at articles like https://initialcommit.com/blog/git-commit-messages-best-practices and https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/ for some tips.

[zas]

Tests should be added for this.

[phw]

Thanks a lot for tackling this. Before we merge this we should have a look over how other tools handle TDRL and how it maps to other tagging format, especially MP4 and ASF (which had not yet been considered in this PR). https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md#audio-metadata-specifications provides some resources for tag specs and mappings in other tools.

The current implementation will not write this tag to ASF, for MP4 it would be treated as a custom tag ----:com.apple.iTunes:releasedate.

So a quick look over what we have here:

Software	ID3v2.3	Vorbis	MP4	ASF	Ape
MP3Tag	TDRL	RELEASETIME?	-	-	-
Kid3	-	RELEASEDATE?	----:com.apple.iTunes:RELEASEDATE	-	-
Yate	TXXX:Release Time	RELEASETIME	----:com.apple.iTunes:RELEASETIME	-	Release Time
JaudioTagger (Jaikoz, SongKong)	-	-	-	-	-
MediaMonkey	-	-	-	-	-
MusicBee	-	-	-	-	-

Notes;

• JaudioTagger, MediaMonkey, MusicBee do not support TDRL (according to the docs)
• MP3Tag: Vorbis is not specified, but AFAIK it ends up with internal name RELEASETIME
• Kid3: MP4 uses uppercase ----:com.apple.iTunes:releasedate
• Kodi treats TDRL just as another source for its DATE, just like TDRC. Unsure which it prefers.
• foobar2000: Supports TDRL. Internal name is RELEASE DATE. Unsure how it handles it for other formats

So this is really a mixed bag (and as a side note is one of the reasons this has not yet been supported by Picard). Anyway, we should just choose. I think the current approach is overall fine. Not supporting ASF seems to be widely the standard way of handling it (and ASF has lost relevance anyway, I can't remember the last time any user had some question about that).

Maybe explicitly add it to the mapping for MP4 as uppercases (----:com.apple.iTunes:RELEASEDATE). What do you think?

[Sophist-UK]

Before we merge this we should have a look over how other tools handle TDRL and how it maps to other tagging format, especially MP4 and ASF (which had not yet been considered in this PR).

1. It is not just tagging tools that we need to research, but also players - it is equally important to know what players support what tags.

2. In the absence of de-jure or de-facto standard, I think we should be willing to think about creating new de-facto standards. If there is no prior use at all, then we can go ahead and do whatever we consider is sensible - if there is prior use but not a de-facto standard e.g. in one tagger / player, then we should consider whether it is reasonable to stay compatible, or whether there is a good reason to set a different de-facto standard. (IMO we should not avoid doing something new just because no one else has done it yet and we are afraid of doing something and then finding someone else does something different.)

[certuna]

A lot of players these days just use boilerplate libraries like TagLib and mutagen, which both map the internal variable releasedate to TDRL (id3v2.4) and RELEASEDATE (Vorbis). Kodi, Navidrome and Jellyfin for example all use TagLib.

I think whether many music players use it isn't so much of a reason to not support writing a tag? Hardly any player uses isrc or encodersettings, it's still supported by all tag editors including Picard.

TDRL is part of the id3v2.4 specs, it's supported by most other major tag libraries/editors, and in terms of code added it's just a tiny addition that doesn't change or hold back anything in Picard itself, it just allows people that do use this frame to write it, and aligns Picard's internal capabilities with other tag editors.

We're also in a chicken/egg situation where application developers are discouraged from using the tag because they know Picard cannot write it - you can see that for example in the Kodi code comments TDRL -> not set by Picard. So using application usage as justification to not support ends up in a circular situation. (This reminds me of the limbo that id3v2.4 was in - lots of music apps didn't read it because two big players Apple and Microsoft (until Windows 10!) only supported id3v2.3)

Anyway, I'm currently writing support for originaldate, date and releasedate (i.e., all three frames) into Navidrome, hopefully in its next release, so that may help :)

In the absence of de-jure or de-facto standard, I think we should be willing to think about creating new de-facto standards.

Yes this is why I restricted this PR to the narrow case where there is a de-jure (id3v2.4) or de-facto (Vorbis & MP4, through mp3tag/Yate/kid3/TagLib) standard. ASF I'm not touching with a bargepole.

If you think the id3v2.3 fallback frame TXXX:releasedate is not 'de facto' enough, happy to remove it - then the default behaviour of mutagen kicks in and it would automatically never write anything to v2.3, only to v2.4.

One more suggestion... You might want to be a bit more descriptive in your commit messages.

As these are tiny (one-line) commits I thought the code would more or less speak for itself, and this PR discussion would provide the required context - but happy to be more verbose in future commits. Unfortunately I cannot edit past commits I think?

[phw]

I think we are essentially good with this approach. The TDRL tag is nod widely supported, but I agree with the approach taken here. What I'd wish to see is adding an explicit support for ----:com.apple.iTunes:RELEASEDATE to MP4 in __r_freeform_tags_ci (https://github.com/metabrainz/picard/blob/master/picard/formats/mp4.py#L159). Then we would be close how Kid3 supports it, and also be able to handle both lower and uppercase tag name there.

A lot of players these days just use boilerplate libraries like TagLib and mutagen,

To be fair just because these libraries are used does not necessarily mean the player has support for this tag. It's still the decision of the developers which tags and how they support. And some tools are very open and allow flexible access to a lot of tags, and others are very limited and only support the essentials. But I get your point.

If you think the id3v2.3 fallback frame TXXX:releasedate is not 'de facto' enough, happy to remove it - then the default behaviour of mutagen kicks in and it would automatically never write anything to v2.3, only to v2.4.

I think it is ok to write it. I just wonder if we should maybe uppercase this. Picard's original approach was to write TXXX frames as title case. Yate also does this in this case with TXXX:Release Time. The reason was because a lot of software did it that way and it seemed to be the format typical approach to name these tags.

But in the last years newer tags had usually been all uppercase, essentially unifying the naming between Vorbis and ID3 TXXX. This makes handling custom tags much easier, as flexible players like foobar2000 can handle these tags independent of what format is being used and without the player needing actual knowledge of the tags. Especially from the foobar2000 side Picard receives frequent criticism for how it handles tags.

An alternative approach would be to use TDRL anyway, as e.g. MP3Tag chose to handle it. There is precedence for this (https://github.com/metabrainz/picard/blob/master/picard/formats/mutagenext/compatid3.py#L76-L77), but usually we did this for tags were there was a wider support for this deviating behavior.

As these are tiny (one-line) commits I thought the code would more or less speak for itself, and this PR discussion would provide the required context - but happy to be more verbose in future commits. Unfortunately I cannot edit past commits I think?

Actually I think in this case we will just squash them on merge into a single commit, it's only a few lines of changes that belong together. So for me the commit history is currently fine as it is. But otherwise I would indeed prefer more descriptive commit messages.

[rdswift]

Just adding a note here so that we don't forget... Once this is all sorted out and merged, we'll need to update the tag mapping appendix information (and spreadsheet) in the Picard documentation. This should be done in the https://github.com/metabrainz/picard-docs/blob/master/tag_mapping.py file, which is used to automatically update all of the references.

[zas]

Unfortunately I cannot edit past commits I think?

You can, git rebase -i is your friend, check out https://about.gitlab.com/blog/2020/11/23/keep-git-history-clean-with-interactive-rebase/ for examples.

That said, in this case, I think you can just squash all commits in one, and set a clean message for it.

[certuna]

What I'd wish to see is adding an explicit support for ----:com.apple.iTunes:RELEASEDATE to MP4 in __r_freeform_tags_ci (https://github.com/metabrainz/picard/blob/master/picard/formats/mp4.py#L159). Then we would be close how Kid3 supports it, and also be able to handle both lower and uppercase tag name there.

Done: 38c739a

I think it is ok to write it. I just wonder if we should maybe uppercase this.

Happy to do this, but shouldn't this then also be done with mood (currently written in lowercase as TXXX:mood in id3v2.3?)

Tests should be added for this.

@zas also happy do do this, I was a bit hesitant to just upload new test.mp3/mp4/flac files without discussion/consultation :)
Should I modify the existing test.mp3/etc files in picard/test/data, or do I create and upload fresh test-releasedate23.mp3, test-releasedate24.mp3, test-releasedate.flac and test-releasedate.m4a files?

[phw]

Happy to do this, but shouldn't this then also be done with mood (currently written in lowercase as TXXX:mood in id3v2.3?)

We usually are rather careful changing tags once they are implemented, unless there is a good reason to change it. Anyway, as part of this PR we should focus on releasedate.

Should I modify the existing test.mp3/etc files in picard/test/data, or do I create and upload fresh test-releasedate23.mp3, test-releasedate24.mp3, test-releasedate.flac and test-releasedate.m4a files?

Most of the test cases use the existing files as basis to first create the tags in the state to test and then perform the tests. The first step for having basic test coverage is to add the releasedate with a value in https://github.com/metabrainz/picard/blob/master/test/formats/common.py#L104 . This verifies the format can write the tag and read it back.

Specific tests for ID3 can be added to https://github.com/metabrainz/picard/blob/master/test/formats/test_id3.py . E.g. to specifically test how the tag gets written to ID3 v2.3 vs v2.4 something like this could work (just some ad-hoc code written here, might contain mistakes):

    class Id3TestCase(CommonTests.TagFormatsTestCase):
        
         # ....

        @skipUnlessTestfile
        def test_releasedate_v23(self):
            config.setting['write_id3v23'] = True
            metadata = Metadata({
                'releasedate': '2023-04-28',
            })
            save_metadata(self.filename, metadata)
            raw_metadata = load_raw(self.filename)
            self.assertEqual(metadata['releasedate', raw_metadata['TXXX:RELEASEDATE')
   
     @skipUnlessTestfile
        def test_releasedate_v24(self):
            config.setting['write_id3v23'] = False
            metadata = Metadata({
                'releasedate': '2023-04-28',
            })
            save_metadata(self.filename, metadata)
            raw_metadata = load_raw(self.filename)
            self.assertEqual(metadata['releasedate', raw_metadata['TDRL')

[zas]

@certuna can you update the PR as requested?

[certuna]

@certuna can you update the PR as requested?

Yes I will have time this weekend to work on the tests.

I have added the uppercase mapping for MP4 as requested by @phw : 38c739a

Are there any more changes needed?

[phw]

@certuna Cool, thanks a lot. Except for tests nothing else from my side. @zas ?

[zas]

@certuna Cool, thanks a lot. Except for tests nothing else from my side. @zas ?

We'll see after tests are implemented, if we squash everything or not. But @certuna please ensure your commit messages are meaningful.

[phw]

As this looks good code wise I'll going to merge it so we can include it in the 2.9 release. Thanks a lot for the contribution, much appreciated.

I'll add some tests after the merge. If you have anything to add feel free to open a new PR for it.

[certuna]

As this looks good code wise I'll going to merge it so we can include it in the 2.9 release. Thanks a lot for the contribution, much appreciated.

I'll add some tests after the merge. If you have anything to add feel free to open a new PR for it.

Thank you Philipp, I've tried creating the tests but I have to admit I had some issues figuring out how to set up the dev environment correctly for that.