Rethink handling of "unseekable" mp3 files

[rustyshelf]

Using ExoPlayer v2.2.0 and trying to play this file: sample.mp3 it reports as un-seekable. However this same file seeks fine in the Android MediaPlayer and various other players as well.

Doing an afinfo on the macOS command line doesn't seem to turn up anything interesting about the file:

File:           Feminist Fight Club.mp3
File type ID:   MPG3
Num Tracks:     1
----
Data format:     2 ch,  44100 Hz, '.mp3' (0x00000000) 0 bits/channel, 0 bytes/packet, 1152 frames/packet, 0 bytes/frame
                no channel layout.
estimated duration: 3887.560687 sec
audio bytes: 62201388
audio packets: 148821
bit rate: 128000 bits per second
packet size upper bound: 1052
maximum packet size: 417
audio data file offset: 417
optimized
----

This seems like a bug, but I'm unsure about where to hunt for whats going wrong?

Edit: Just to rule out our implementation I tried it in the sample project as well, same result, it disables the seek bar and buttons.

[AquilesCanta]

Have you seen #1299?

[rustyshelf]

@AquilesCanta I have now (I did try and search for similar issues but didn't find any), my vote @ojw28 would be to assume constant bit rate and allow seeking, or at the very least have this as a configuration parameter. We're working on a new version of our app at the moment and not being able to seek (or even resume from the middle of) files like this would be a deal breaker :(

[ojw28]

Are you in control of the media being played? If not then where is it typically coming from?

We're unlikely to add support for this case. As per #1299 it's unlikely MediaPlayer is doing a particularly good job of allowing seeking (there's likely a significant trade-off of one kind or another involved).

[rustyshelf]

@ojw28 we make a podcasting app, so we have 0 control over the media. Also this file seeks just fine on both iOS (using AVPlayer or AVAudioEngine) as well as on Android using MediaPlayer or just an Extractor. I understand the reasoning, but I have yet to see any of the previous solutions get seeking wrong in a CBR file. Pretty much all podcasts are CBR, VBR is extremely rare (< 1%). I get this could be specific to our case, but still, it would mean we'd have to fork this project and maintain a fork just to get this behaviour.

Won't somebody please think of the children ;)

[rustyshelf]

@ojw28 curious to know if this is something you'd consider, or whether your intention is to keep it as is. If we were to make this change ourselves I'd be curious on your opinion as to the best place to make it, and whether you'd consider a pull request for it or not?

[ojw28]

I think it's quite problematic to turn it on blindly, particularly with respect to playlist support. We could probably have it as an option that you can enable on the extractor. You'd need to pass your own ExtractorsFactory to ExtractorMediaSource that sets the option, if we were to go down that route.

[rustyshelf]

@ojw28 completely agree that it shouldn't be on by default. I guess my question was around where to add the API. For example the player could have a seekToApproximatePosition() was my thinking. You'd call this if isSeekable is false and you don't mind imprecise seeking. I also feel that seek should be changed to not attempt to seek unseekable files, since it just resets the position. That could be confusing to document and explain though so I like the ExtractorsFactory option. We'd like to implement this fairly soon so any other guidance you might have would be great.

[rustyshelf]

Ok after looking at this a bit more, it seems like the simplest change is to alter the Mp3Extractor to use the ConstantBitrateSeeker if no other seeker can be set up (eg seeker == null). We made this change and tested it and now all our test MP3's seek great (including the one referenced above).

The only minor downside is that this involves making our own ExtractorFactory and also copying the entire source for Mp3Extractor and all the various seekers (XingSeeker, VbrSeeker, ConstantBitrateSeeker) because they are all private. This is despite us not wanting to modify any of them. It's not a huge deal, but having to keep the these up to date with any changes is not ideal. If this could ever be made into some kind of preference (off by default) that would be amazing :)

[ojw28]

If you look at FragmentedMp4Extractor you'll see there are some flags that can be passed to the constructor to enable different non-standard behaviors. The no-arg constructor passes 0 (i.e. no flags). You could do something similar in Mp3Extractor to allow using ConstantBitrateSeeker if no other seeker can be configured (FLAG_ENABLE_CONSTANT_BITRATE_SEEKING or something). Initially there will only be one flag defined, but let's use the same style for consistency and so that we can add other flags in the future.

I think we'd be happy to accept a pull request that does this, if you feel like putting one together. Once that's submitted all you'll have to do in your own code is to create your own ExtractorFactory, but you can do that with very little code so this should be acceptable I think.