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

ExoPlayer still wrongly decode some MP3 file #1480

Closed
1 task
Tolriq opened this issue Jun 21, 2024 · 8 comments
Closed
1 task

ExoPlayer still wrongly decode some MP3 file #1480

Tolriq opened this issue Jun 21, 2024 · 8 comments
Assignees
Labels

Comments

@Tolriq
Copy link
Contributor

Tolriq commented Jun 21, 2024

Version

Media3 pre-release (alpha, beta or RC not in this list)

More version details

1.4.0 Beta 1

Devices that reproduce the issue

All

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

Play the attached files, they should support gapless but it's not working and logs shows :

internalError [eventTime=4684.06, mediaPos=129.00, window=0, period=0, loadError
                                                                               androidx.media3.common.ParserException: Searched too many bytes.{contentIsMalformed=true, dataType=1}
                                                                                   at androidx.media3.extractor.mp3.Mp3Extractor.synchronize(Mp3Extractor.java:412)
                                                                                   at androidx.media3.extractor.mp3.Mp3Extractor.readInternal(Mp3Extractor.java:281)
                                                                                   at androidx.media3.extractor.mp3.Mp3Extractor.read(Mp3Extractor.java:254)
                                                                                   at androidx.media3.exoplayer.source.BundledExtractorsAdapter.read(BundledExtractorsAdapter.java:147)
                                                                                   at androidx.media3.exoplayer.source.ProgressiveMediaPeriod$ExtractingLoadable.load(ProgressiveMediaPeriod.java:1074)
                                                                                   at androidx.media3.exoplayer.upstream.Loader$LoadTask.run(Loader.java:421)
                                                                                   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
                                                                                   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
                                                                                   at java.lang.Thread.run(Thread.java:1012)
                                                                             ]

This is probably a follow up to #1376 as the user report that in the previous version (so before the fix from that issue) the duration was wrongly reported. Now the duration is good but there's still some decoding issue somewhere.

Expected result

No error in logs and gapless working

Actual result

Does not gapless and error in logs:

https://github.com/androidx/media/issues/1376

Media

Downloads.zip

Bug Report

@Tolriq
Copy link
Contributor Author

Tolriq commented Jun 21, 2024

@icbaker This is most probably a follow up to your fixes from #1376.

Maybe different root issue, but they did report wrong duration before and now report correct duration, but gapless still does not work and there's error logged.

@tonihei tonihei assigned tonihei and icbaker and unassigned tonihei Jun 24, 2024
@icbaker
Copy link
Collaborator

icbaker commented Jun 28, 2024

Thanks for the report. I can reproduce the issue by playing the 01 - Since I Left You.mp3 file in the demo app built from the tip of main and waiting for the loading position to reach the end of the file (this is when the stack trace is first logged). Playback then fails when the playback position reaches the end of the file.

I tried reverting 5b3066f and I see the same behaviour.

I built the demo app at 1.3.1 and I see the same behaviour there (though with the expected incorrect duration), so I don't think this is a regression. Did the user report this file working correctly previously? On what media3 version?

@Tolriq
Copy link
Contributor Author

Tolriq commented Jun 28, 2024

Did not say it's a regression, just that on those files the duration was also wrong before that patch by a few seconds and it's now the correct one.

So it's possible the issue is tied like you fixed most of the stuff and it shows proper durations but there's still a wrong calculation leading to the app trying to read too much at the end by not using the updated calculation and so error and so break Gapless.

@icbaker
Copy link
Collaborator

icbaker commented Jul 31, 2024

#1563 points out that this is a regression since 1.2.1, and it is (like #1376) due to using "constant bitrate seeking" for files with an Info frame, instead of the (less accurate) table-of-contents in the Info frame. The ConstantBitrateSeeker currently always indicates the "last data position" is unknown:

public long getDataEndPosition() {
return C.INDEX_UNSET;
}

I've fixed this by changing this method to return inputLength if it's known, instead of always indicating that the end position is unknown. This ensures that we stop searching for MP3 sync bytes once we get past the length indicated by the Info header, and resolves the error.

I've sent this change for internal review.

copybara-service bot pushed a commit that referenced this issue Aug 2, 2024
This is needed to correctly handle files with trailing non-MP3 data
(which is indicated by the length in the `Info` frame being shorter than
the overall length of the file).

The test file was generated by appending 150kB of `DEADBEEF` onto the
end of `test-cbr-info-header.mp3`, and the test asserts that the
extracted samples are identical.

Issue: #1480

#cherrypick

PiperOrigin-RevId: 658727595
@icbaker
Copy link
Collaborator

icbaker commented Aug 2, 2024

Should be fixed by the commit linked above.

@icbaker icbaker closed this as completed Aug 2, 2024
@Tolriq
Copy link
Contributor Author

Tolriq commented Aug 3, 2024

Thanks, is 1.5 alpha 1 planned relatively soon or should I just backport on my copy ?

@icbaker
Copy link
Collaborator

icbaker commented Aug 5, 2024

I plan to include b09cea9 in 1.4.1 which should be released this month (August). It will also be included in 1.5.0-alpha01, which atm is due to be released towards the beginning of next month (September).

@Tolriq
Copy link
Contributor Author

Tolriq commented Aug 5, 2024

Perfect thanks.

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

No branches or pull requests

3 participants