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

Player moves to next item in playlist before completing previous item. #1904

Closed
1 task
akshdeep-singh opened this issue Nov 19, 2024 · 3 comments
Closed
1 task
Assignees

Comments

@akshdeep-singh
Copy link

Version

Media3 1.4.1

More version details

No response

Devices that reproduce the issue

Pixel 8 running Android 15
Emulator Pixel 5 Android 14

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

  1. Play the issue causing mp3 file
  2. Wait until reached 1:40 (1 min 40 sec) position

Expected result

The media plays successfully

Actual result

Either the player starts playing next item in the playlist, or the sound stops but position is increasing.
No Error shown by player.
Following warnings are there:

6038-6103  CCodecConfig            I  query failed after returning 7 values (BAD_INDEX)
6038-6076  MediaCodec              D  keep callback message for reclaim
6038-6103  Codec2Client            W  query -- param skipped: index = 1342179345.
6038-6103  Codec2Client            W  query -- param skipped: index = 2415921170.

I am observing this issue with more mp3 files also but at different positions.
The issue is not observed by other softwares, such as Windows Media Player, Chrome Browser Player.

Media

MP3 file link

Bug Report

@icbaker
Copy link
Collaborator

icbaker commented Nov 22, 2024

This is happening because the file has a VBRI header where the table of contents (ToC) only covers 2255081 bytes, while the file on disk is 13605001 bytes long.

When I play this file in ExoPlayer I see a warning logged about this:

VbriSeeker              androidx.media3.demo.main            W  VBRI data size mismatch: 13605001, 2255081

The result of this that we indicate the frame just before byte 2255081 is the last sample, and so playback stops at that point. With some extra logging I see dataEndPosition=2255081, extractorInput.getPeekPosition()=2255491 at these lines:

if (peekEndOfStreamOrHeader(extractorInput)) {
return RESULT_END_OF_INPUT;
}

private boolean peekEndOfStreamOrHeader(ExtractorInput extractorInput) throws IOException {
if (seeker != null) {
long dataEndPosition = seeker.getDataEndPosition();
if (dataEndPosition != C.INDEX_UNSET
&& extractorInput.getPeekPosition() > dataEndPosition - 4) {
return true;


We are currently assuming that the ToC in the VBRI frame covers the whole file. This is perhaps an unreasonable assumption.

The VBRI frame also has a bytes field to indicate the amount of MP3 data in the file (which we don't use atm). This should be the number of MP3 bytes, not including the VBRI frame itself.

In your file, the VBRI frame has bytes=13603849, so based on that we'd expect the end of the MP3 data to be at:

  vbriFrameStart + vbriFrameSize + bytes
= 1024 + 522 + 13603849
= 13605395

This seems incorrect, because the file on disk is only 13605001 bytes long!

With a bit of guesswork, and noting that the VBRI frame also has a frameSize=128 field (which is meant to be the average frame size of the rest of the file), we can re-run the calculation assuming whatever populated the bytes field incorrectly assumed the VBRI frame itself was 128 bytes long:

  vbriFrameStart + avgFrameSize + bytes
= 1024 + 128 + 13603849
= 13605001

And this now exactly matches the file on disk.

Aside: We can also derive the MP3 data length from the VBRI frame size & count values: numFrames * frameSize = 2882048. This is clearly also incorrect in this file (but roughly consistent with the ToC).

Summary:

This file has a strange VBRI frame, with a ToC that only covers the first ~1:40 of the file, an incorrect average frameSize value, and an incorrect bytes value which assumes the VBRI frame itself is 128 bytes long (when it's actually 522 bytes long) - meaning it's wrong by 394 bytes.


If we switch to using the bytes field from the VBRI header then other parts of ExoPlayer's MP3 machinery handle the 394 length error fairly gracefully and playback continues for the whole duration of the file. I'll send a change for this.

@icbaker
Copy link
Collaborator

icbaker commented Nov 22, 2024

This file has a strange VBRI frame, with a ToC that only covers the first ~1:40 of the file

This also results in seeking being broken/nonsensical for this file, because the VBRI ToC assumes that each of the N entries covers an Nth of the file. The result of this is that (with the bytes fix applied), when I play your file and seek near the end, playback continues well past the 9:48 'duration' - because the "seek near the end" actually resulted in us seeking near the end of the first 2255081 bytes, so there's lots more MP3 data left to consume.

copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
This was hand-crafted with a 4-entry ToC by modifying
`bear-vbr-xing-header.mp3` in a hex editor.

The output difference from 117 samples to 116 samples is due to the
calculation in `VbriSeeker` assuming that the ToC includes the VBRI
frame itself, which I don't think is correct (fix is in a follow-up
change).

Issue: #1904

#cherrypick

PiperOrigin-RevId: 700254516
copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
The current code assumes that the first Table of Contents segment
includes the `VBRI` frame, but I don't think this is correct and it
should only include real/audible MP3 ata - so this change updates the
logic to assume the first ToC segment starts at the frame **after** the
`VBRI` frame.

Issue: #1904

#cherrypick

PiperOrigin-RevId: 700269811
copybara-service bot pushed a commit that referenced this issue Nov 26, 2024
The previous code assumed that the `VBRI` Table of Contents (ToC)
covers all the MP3 data in the file. In a file with an invalid VBRI ToC
where this isn't the case, this results in playback silently stopping
mid-playback (and either advancing to the next item, or continuing to
count up the playback clock forever). This change considers the `bytes`
field to determine the end of the MP3 data, in addition to deriving it
from the ToC. If they disagree we log a warning and take the max value.
This is because we handle accidentally reading non-MP3 data at the end
(or hitting EoF) better than stopping reading valid MP3 data partway
through.

Issue: #1904

#cherrypick

PiperOrigin-RevId: 700319250
@icbaker
Copy link
Collaborator

icbaker commented Nov 26, 2024

This invalid file is now handled more gracefully after 46578ee

@icbaker icbaker closed this as completed Nov 26, 2024
shahdDaghash pushed a commit that referenced this issue Dec 18, 2024
This was hand-crafted with a 4-entry ToC by modifying
`bear-vbr-xing-header.mp3` in a hex editor.

The output difference from 117 samples to 116 samples is due to the
calculation in `VbriSeeker` assuming that the ToC includes the VBRI
frame itself, which I don't think is correct (fix is in a follow-up
change).

Issue: #1904

#cherrypick

PiperOrigin-RevId: 700254516
(cherry picked from commit 3eb36d6)
shahdDaghash pushed a commit that referenced this issue Dec 18, 2024
The current code assumes that the first Table of Contents segment
includes the `VBRI` frame, but I don't think this is correct and it
should only include real/audible MP3 ata - so this change updates the
logic to assume the first ToC segment starts at the frame **after** the
`VBRI` frame.

Issue: #1904

PiperOrigin-RevId: 700269811
(cherry picked from commit f257e55)
shahdDaghash pushed a commit that referenced this issue Dec 18, 2024
The previous code assumed that the `VBRI` Table of Contents (ToC)
covers all the MP3 data in the file. In a file with an invalid VBRI ToC
where this isn't the case, this results in playback silently stopping
mid-playback (and either advancing to the next item, or continuing to
count up the playback clock forever). This change considers the `bytes`
field to determine the end of the MP3 data, in addition to deriving it
from the ToC. If they disagree we log a warning and take the max value.
This is because we handle accidentally reading non-MP3 data at the end
(or hitting EoF) better than stopping reading valid MP3 data partway
through.

Issue: #1904

#cherrypick

PiperOrigin-RevId: 700319250
(cherry picked from commit 46578ee)
@androidx androidx locked and limited conversation to collaborators Jan 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants