Parse ReplayGain data from LAME Xing/Info header

[nift4]

see http://gabriel.mp3-tech.org/mp3infotag.html#replaygain

[icbaker]

We don't need to perfectly match the spec here - it might be a clearer API to have an IntDef that reflects the three possible values of unset, radio & audiophile.

Similarly, maybe a nested type structure would be clearer than name1A, name1B, name2A etc.

Something like:

public final class Mp3InfoReplayGain {

  public static final class GainField {
    public final @Name int name;
    public final @Originator int originator;
    public final int gain;
  }
  public final float peak;
  public final GainField field1;
  public final GainField field2;
}

[nift4]

Thanks for the feedback, that makes a lot of sense and I implemented that.

[icbaker]

Similar comment here, instead of a boolean controlling the presence of all the other fields, move them into a type that can then be nullable.

Maybe you can even re-use the Metadata.Entry subclass here? Instead of duplicating all the fields.

[nift4]

Similar to above, that works great and is now implemented.

[nift4]

Hi @icbaker, I had pushed a new commit to address the review comments, could you please review again? Thanks! :)

[icbaker]

The change looks generally good.

I ran all the tests in Mp3ExtractorTest and Mp3PlaybackTest with the change, and nothing failed (i.e. it doesn't find any replaygain data in our existing MP3 test assets).

Can you include a very small MP3 test asset in this PR that we can use to test the extraction?

Ideally by re-encoding or re-muxing one of the existing test assets in our library.

[nift4]

Hi @icbaker,

That is odd, I assume the additional metadata log line should've triggered a difference in any case. With EventLogger I see that 9 files (one of them is libraries/test_data/src/test/assets/media/mp3/bear-vbr-xing-header.mp3) have empty data (common for non-LAME encoders writing LAME headers)

D    Metadata [
D      TSSE: description=null: values=[Lavf58.29.100]
D      ReplayGain Xing/Info: peak=0.0, f1 name=0, f1 orig=0, f1 val=0.0, f2 name=0, f2 orig=0, f2 val=0.0
D    ]

What indeed seems missing is an file with actual data, so I have re-encoded one of the samples with lame <file> (with the default replaygain-fast mode) and with lame --replaygain-accurate <file> in the new patchset using LAME 64bits version 3.100 (http://lame.sf.net).

[icbaker]

Ah sorry, I ran the tests in the wrong IDE - so not pointing at the branch with your pending changes... When I run them in the correct place, some do fail (but yes, as you say with 'zero' values).

But it's still good to have a test for the non-zero case, so thanks for providing that.

I've updated the test dump files, and added new ones for the two new files.

I've also made a few tweaks to the implementation (mostly re-ordering).

[nift4]

@icbaker Regarding isValid: the spec explicitly advises against interpreting unknown field names, and isValid() doesn't prevent apps from manually performing a similar check if someone really does make a new field name. But as only these two field names are in use for the better part of two and a half decades, I don't think it's very likely, which is why I think isValid() provides some value by suggesting users to check the name is not 0 (in which case all data in the gain field is junk) nor invalid.

If you still think it shouldn't be there, fine with me, but I wanted to mention this extra context.

[icbaker]

I think we should either parse the data that's there and expose it to the developer to choose what to do with (current state with isValid removed).

Or confidently say that only radio & audiophile will ever be seen, and everything else should be ignored, in which case we just skip any other values and don't expose them at all. In this case there's no need for isValid we just have null values either for one of the GainField fields or we just don't emit the Mp3InfoReplayGain entry at all.

If ExoPlayer was also implementing the handling of this data, then I'd lean more strongly towards the second approach (don't parse what we don't understand / can't use).

In the current code, the parsing & emitting is the only thing that ExoPlayer is doing, and so emitting whatever we find seems reasonable.

[nift4]

Makes sense.

If ExoPlayer was also implementing the handling of this data

That would be nice, but for it to work in offload we'd need an implementation based on DynamicsProcessing and for devices below Android 9 there'd need to be an AudioProcessor based one (if implementing the full spec, it would need a dynamic range compression software implementation, which is hard to do in a performant way in Java). In addition there's 7 ways I know of ReplayGain information can be stored in just MP3 files alone simply due to the long history of both (ID3 TXXX REPLAYGAIN_* frames (modern software), ID3 RVA2/XRVA frames (ID3v2.4), ID3 RVAD/RVA (ID3v2.3), ID3 comment frame (mpg123), ID3 RGAD frame (2002 ReplayGain spec), Xing header (LAME) - this PR, and APE tags (mp3gain)), it's an incredibly complex thing to do and probably won't ever happen (if there is interest besides all that, I'm happy to work on it though).

Today there aren't even all the pieces for an app side implementation alone, as LAME parsing (this PR), APE tag parsing (plan to code and upstream later), and disabling gapless if required (discussed in #2855) are required, so I'm focusing on that for now.