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

MediaMetadata doesn't include some ID3 tags #1305

Closed
1 task
akshdeep-singh opened this issue Apr 22, 2024 · 6 comments
Closed
1 task

MediaMetadata doesn't include some ID3 tags #1305

akshdeep-singh opened this issue Apr 22, 2024 · 6 comments
Assignees
Labels

Comments

@akshdeep-singh
Copy link

Version

Media3 1.3.1

More version details

No response

Devices that reproduce the issue

Pixel 3a Emulator API 34

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

  1. Play any mp3 file which contains id3 tags with ids TCON, COMM, etc.
  2. Check values in onMediaMetadataChanged and onTracksChanged
override fun onTracksChanged(tracks: Tracks) {
    tracks.groups.forEach { group ->
        for (i in 0 until group.length) {
            group.mediaTrackGroup.getFormat(i).metadata?.let { metadata ->
                for (j in 0 until metadata.length()) {
                    metadata.get(j).let {
                        if (it is TextInformationFrame) {
                            println("${it.id} ${it.values}") // TCON [<value>]
                        }

                        if (it is CommentFrame) {
                            println("${it.id} ${it.text}") // COMM <value>
                        }
                    }
                }
            }
        }
    }
}

override fun onMediaMetadataChanged(mediaMetadata: MediaMetadata) {
    println("genre: ${mediaMetadata.genre}") // null
    println("description: ${mediaMetadata.description}") // null
}

Expected result

Value of MediaMetadata.genre is filled with TCON frame content
Value of MediaMetadata.description is filled with COMM frame content

Actual result

MediaMetadata values described above are null

Media

Sample MP3 link

Bug Report

@icbaker
Copy link
Collaborator

icbaker commented Apr 23, 2024

Please can you try listening to Player.Listener.onMetadata? That's where I would expect this to be emitted, as described here: https://developer.android.com/media/media3/exoplayer/retrieving-metadata#during-playback

@akshdeep-singh
Copy link
Author

onMetadata isn't getting called for me, also I would prefer MediaMetadata as I am also filling some custom info when building MediaItem, with onTracksChanged I am reading bitrate information.

@icbaker
Copy link
Collaborator

icbaker commented Apr 23, 2024

I played the provided link in the demo app and I see COMM and TCOM logged as part of the metadata in onTracksChanged (this logging is part of EventLogger, see https://developer.android.com/media/media3/exoplayer/debug-logging):

tracks [eventTime=1.27, mediaPos=0.00, window=0, period=0
  group [
    [X] Track:0, id=null, mimeType=audio/mpeg, bitrate=128000, channels=2, sample_rate=44100, supported=YES
  ]
  Metadata [
    COMM: language=xxx, description=
    TCOM: description=null: values=[SikhSangeet.com]
    TCON: description=null: values=[Shabad Kirtan]
    TCOP: description=null: values=[SikhSangeet.com]
    TENC: description=null: values=[SikhSangeet.com]
    WXXX: url=http://SikhSangeet.com
    TALB: description=null: values=[Teri Bhagat Na Chhodon]
    TIT2: description=null: values=[Nindo Nindo Mauko]
    TOPE: description=null: values=[Harjinder Singh (Sri Nagar Wale)]
    TPE1: description=null: values=[Harjinder Singh (Sri Nagar Wale)]
    TRCK: description=null: values=[01]
  ]
]

@akshdeep-singh
Copy link
Author

yes, onTracksChanged gets called but onMetadata doesn't, but my actual point of reporting this issue was that information such as "genre" is very basic which should be filled in MediaMetadata, right?

@icbaker
Copy link
Collaborator

icbaker commented Apr 24, 2024

Ah gotcha, I misunderstood your original comment.

Value of MediaMetadata.genre is filled with TCON frame content

This makes sense, based on my reading of the definition of the TCON frame here, though we probably don't want to naively popoulate the string directly and instead optionally do the numeric resolution described there, and somewhat already implemented here (although there may be an off-by-one bug there, I'm going to dig a bit further):

private static TextInformationFrame parseStandardGenreAttribute(ParsableByteArray data) {
int genreCode = parseUint8AttributeValue(data);
@Nullable
String genreString =
(0 < genreCode && genreCode <= STANDARD_GENRES.length)
? STANDARD_GENRES[genreCode - 1]
: null;
if (genreString != null) {
return new TextInformationFrame(
"TCON", /* description= */ null, ImmutableList.of(genreString));
}
Log.w(TAG, "Failed to parse standard genre code");
return null;
}


Value of MediaMetadata.description is filled with COMM frame content

I'm not sure sure about this. COMM is described in a very free-form way, in particular there might be more than one, so I don't think it maps very clearly to MediaMetadata.description. I'm not sure that any of these ID3 frame types really contain a 'description'.


I'll prepare a change to map TCON to MediaMetadata.genre.

copybara-service bot pushed a commit that referenced this issue Apr 25, 2024
This change also adds some runtime redirection of calls to
`Dumper.add(String, Object)` to `add(String, byte[])` if
`value instanceof byte[]`. This simplifies the implementation of
`PlaybackOutput.dumpIfNotEqual` and seems like a reasonable amound of
'magic' for a test utility.

Issue: #1305
PiperOrigin-RevId: 628119473
copybara-service bot pushed a commit that referenced this issue Apr 26, 2024
The first has a string genre, and various other values set, generated
from `sample.mp4` with the command below [1].

The second has a numeric genre, to test `gnre` atom parsing. This
parsing is currently broken, the fix is in a follow-up change. This
file was also generated from `sample.mp4` with the command below [2].

This change also includes `CommentFrame.text` in its `toString`
representation, otherwise there's no difference between e.g. different
levels of `ITUNESADVISORY` in the extractor dump files.

Issue: #1305

-----

[1]

```shell
$ AP_PADDING="DEFAULT_PAD=0" \
    AtomicParsley sample.mp4 \
    --artist "Test Artist" \
    --album "Test Album" \
    --tracknum 2/12 \
    --disk 2/3 \
    --year 2024  \
    --genre "Gorpcore" \
    --bpm 120 \
    --compilation true \
    --advisory clean \
    --gapless true \
    --sortOrder artist "Sorting Artist" \
    --sortOrder album "Sorting Album" \
    --preventOptimizing \
    -o sample_with_metadata.mp4
```

[2]
```shell
$ AP_PADDING="DEFAULT_PAD=0" \
    AtomicParsley sample.mp4 \
    --genre "Metal" \
    --preventOptimizing \
    -o sample_with_numeric_genre.mp4
```

PiperOrigin-RevId: 628345458
copybara-service bot pushed a commit that referenced this issue Apr 26, 2024
This fixes parsing the `gnre` and `tmpo` values, as seen in the test
dump changes in this CL.

Issue: #1305
PiperOrigin-RevId: 628352773
copybara-service bot pushed a commit that referenced this issue Apr 29, 2024
This change adds a 'free-form' text genre to the existing
`bear-id3.mp3` sample, and adds a new sample with a 'numeric' genre ([9
is Metal](https://mutagen-specs.readthedocs.io/en/latest/id3/id3v2.4.0-frames.html#appendix-a-genre-list-from-id3v1)).

The samples were modified with:

```shell
$ id3edit --set-genre "Gorpcore" bear-id3.mp3
$ id3edit --set-genre "9" bear-id3-numeric-genre.mp3
```

Reading the numeric genre with `exiftool` shows the mapping back to
'Metal':

```
$ exiftool bear-id3-numeric-genre.mp3 | grep Genre
Genre                           : Metal
```

The playback dumps don't contain the genre because it's not yet
propagated to `MediaMetadata.genre`. That is done in a follow-up
change.

Issue: #1305
PiperOrigin-RevId: 629043506
copybara-service bot pushed a commit that referenced this issue Apr 29, 2024
This change also includes mapping the numeric ID3v1 codes to their
string equivalents before setting them into `MediaMetadata`. This
mapping already existed, but it was previously only used when parsing
MP4 `gnre` atoms.

Issue: #1305
PiperOrigin-RevId: 629113480
@icbaker
Copy link
Collaborator

icbaker commented Apr 30, 2024

This is now implemented by the commits referenced above.

@icbaker icbaker closed this as completed Apr 30, 2024
@androidx androidx locked and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants