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

Handle multiple data for string atoms #290

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

gnattu
Copy link
Contributor

@gnattu gnattu commented Nov 26, 2024

The MP4 container supports multiple data atoms within a single metadata field atom. However, this is handled incorrectly now. The extra data atom is treated as an additional field, and this also causes the seek position to be incorrect. Consequently, parsing of other fields, such as the trkn field might also break.

See jellyfin/jellyfin#13004 for more info

@Zeugma440
Copy link
Owner

Thanks a lot for the PR!

I've been very busy with another project lately. I'll do a review when things have calmed down.

Thanks for your understanding~

@Zeugma440 Zeugma440 self-requested a review November 27, 2024 10:49
@Zeugma440
Copy link
Owner

I'm afraid we'll need to work a little more on that PR. I have three issues so far :

1/ I can't find any official nor non-official specification documenting the structure you describe

2/ If one updates the part that reads metadata, one also has to update the part that writes it.
I have no problem with doing that myself, but I can't do it without any spec.

3/ Last but not least, I don't know how to find or forge an M4A/MP4 file that exhibits the data structure you're describing.

Thanks in advance for your help.

@gnattu
Copy link
Contributor Author

gnattu commented Nov 30, 2024

1/ I can't find any official nor non-official specification documenting the structure you describe

This is very unfortunate as the documentation of this behavior is poor, but the data structure is basically just multiple 'data' atoms inside one metadata, for example 'ART' box. This is a reference layout https://xhelmboyx.tripod.com/formats/mp4-layout.txt, you scroll to the bottom of this file, and the single atom multi data structure is basically repeating this part:

* 8+ bytes APPLE item data box
                   = long unsigned offset + long ASCII text string 'data'
                 -> 4 bytes version/flags = byte hex version + 24-bit hex flags
                     (current version = 0 ; contains text flag = 0x000001
                      contains data flag = 0x000000 ; for tmpo/cpil flag = 0x000015
                      contains image data flag = 0x00000D)
                 -> 4 bytes reserved = 32-bit value set to zero
                 -> annotation text or data values = text or hex dump
                     (NOTE: Genre is either text or a 16-bit short ID3 value and
                        most other non-text data are short unsigned values
                        with the exception of compilation which is a byte flag)

The data size of the parent atom, (e.g. ART) will now be the sum of all of the data atom length plus its own data length.

iTunes only use this structure for covr atom which is already handled by atl, but a lot of tag writers use the same structure for all the string data fields.

2/ If one updates the part that reads metadata, one also has to update the part that writes it.
I have no problem with doing that myself, but I can't do it without any spec.
3/ Last but not least, I don't know how to find or forge an M4A/MP4 file that exhibits the data structure you're describing.

I have attached a sample with sinewave sound tagged with multiple values in the artist filed.

output_sinewave.m4a.zip

@Zeugma440
Copy link
Owner

Alright, that works for me. I'll start from what you pushed 👍

@Zeugma440 Zeugma440 merged commit 3a32775 into Zeugma440:main Nov 30, 2024
1 check failed
@Zeugma440
Copy link
Owner

That's a wrap!

FYI, the implementation isn't exactly the same as for embedded pictures, as the latter uses multiple data atoms behind one single covr field, whereas here, we're repeating the entire metadata structure multiple times.

@gnattu
Copy link
Contributor Author

gnattu commented Nov 30, 2024

That's an oversight from my side making that sample. We do have TWO flavors of doing this. The one I intended to implment is using the exact same as covr data structure. That sample I uploaded was using a different flavor where multi value Artist field are written to multiple ©ART atoms each with a single data atom. This one is technically invalid (not within spec at all) but some taggers defaults to this mode. If we don't want to handle this non-standard case we can just ignore extra values with the same atom tag, or doing it read-only as write as single atom multi data.

This sample is intended to demonstrate the exact same data structure of the covr:

output_sinewave2.m4a.zip

This one is more problematic because the seek position will be wrong due to multi data in the ©ART atom.

@gnattu
Copy link
Contributor Author

gnattu commented Nov 30, 2024

If you need more info on how popular metadata tagger are handling this:

  • MP3tagger will use Multiple Atoms flavor to write multiple ©ART atoms, out of spec but probably being widely used.
  • Picard will use the Single Atom/Multiple Data flavor to write multiple data boxes in one ©ART atom. Technically more correct.

@Zeugma440
Copy link
Owner

Okaaaay... thanks for the extra sample anyway, that's precious.
I'll try to get that variant straight as well.

Please tell me I just need to implement that one in read mode, not write mode.

@gnattu
Copy link
Contributor Author

gnattu commented Nov 30, 2024

Please tell me I just need to implement that one in read mode, not write mode.

What I think is more correct:

  • Multiple Atoms (first sample) flavor in read-only mode as this flavor technically violates the spec
  • Single Atom/Multiple Data (second sample) in read-write mode as the same structure is already used by covr

@Zeugma440
Copy link
Owner

I'd rather say in the absence of a bona fide spec, the user's the one who decides.

I'm gonna follow your lead on that one.

@Zeugma440
Copy link
Owner

Turns out we did the heavy lifting yesterday. Everything should be good with the latest commit.

@Zeugma440
Copy link
Owner

Now available in today's v6.09

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

Successfully merging this pull request may close these issues.

2 participants