Skip to content

Handle gapless MP3 Xing/Info durations#3198

Open
Tolriq wants to merge 6 commits into
androidx:mainfrom
Tolriq:mp3_gapless
Open

Handle gapless MP3 Xing/Info durations#3198
Tolriq wants to merge 6 commits into
androidx:mainfrom
Tolriq:mp3_gapless

Conversation

@Tolriq

@Tolriq Tolriq commented May 5, 2026

Copy link
Copy Markdown
Contributor

LAME Xing/Info headers can include encoder delay and padding. Mp3Extractor already propagates those values into Format so decoded audio is rendered gaplessly, but the SeekMap duration was still computed from the untrimmed MPEG frame count. For CBR Info files this can expose a source duration that is longer than the samples that will actually be played, which can break gapless transitions.

Split the Xing/Info duration into two concepts: raw duration from the frame count, used for bitrate and byte-position calculations, and gapless duration with encoder delay and padding removed, exposed from SeekMap.

Keep CBR average-bitrate derivation on the raw duration to avoid changing byte-position seeking. When a CBR seeker is given an explicit gapless duration, map seeks at the advertised end of the stream to the raw data end so the SeekMap endpoint contract is preserved.

Add focused tests for raw vs gapless duration calculation, CBR seek endpoint handling, and Info-frame duration/bitrate behavior, and update affected extractor dumps.

Fixes #3183.

@Tolriq Tolriq mentioned this pull request May 5, 2026
1 task
@icbaker icbaker self-assigned this May 6, 2026
@icbaker icbaker self-requested a review May 6, 2026 15:56
@icbaker

icbaker commented May 6, 2026

Copy link
Copy Markdown
Collaborator

GitHub isn't letting me add review comments for some reason, so here's a quick bit of initial feedback


The part of this PR related to the LAME_TO_DECODED_PCM_TRIM_OFFSET_SAMPLES constant & its usages seem unrelated to the duration vs raw duration split. Please can you send this as a separate PR?


In the bear-vbr-xing-header-no-toc.mp3.cbr-seeking-always.0.dump dump file, I'd expect getPosition(DURATION) to return timeUs=DURATION (concretely: getPosition(2783979) = [[timeUs=2783979, position=38396]]). But it seems timeUs is still returning the 'raw' duration (same for many other dump files).

@Tolriq

Tolriq commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

Yeah sorry I did not fully verify that part, I completely underestimated the complete mess that is mp3 seeking and gapless before going into this.

I've moved the lame part in another PR, but used AI to rebase it on main to not depend on this one. It's probably best to not really look at the second one deeply until this is one is done or not.

Comment thread libraries/extractor/src/main/java/androidx/media3/extractor/mp3/Seeker.java Outdated
@Tolriq

Tolriq commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Rebased after the merge of the other one.

Comment thread libraries/extractor/src/main/java/androidx/media3/extractor/mp3/IndexSeeker.java Outdated
Comment thread libraries/extractor/src/main/java/androidx/media3/extractor/mp3/Mp3Extractor.java Outdated
Comment thread libraries/extractor/src/main/java/androidx/media3/extractor/mp3/IndexSeeker.java Outdated
@Tolriq Tolriq force-pushed the mp3_gapless branch 2 times, most recently from 396c498 to 5c77d9b Compare June 2, 2026 10:31
@Tolriq

Tolriq commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

So I rebased and run a Codex review and the added back the defensive check due to :

• Yes, this can happen. The earlier checks only prove:

  - durationUs is valid, from frameCount.
  - There is some stream length source, from Info.dataSize or fallbackStreamLength.

  They do not prove audioLength > 0.

  Concrete reachable case:

  audioLength = infoFrame.dataSize - infoFrame.header.frameSize;

  XingFrame.parse() accepts dataSize directly from the Info header without validating it against the MPEG frame size. So a malformed Info header can have dataSize == header.frameSize or smaller. Then durationUs is still valid because it comes from frameCount, but
  computeAverageBitrate(audioLength, durationUs) returns C.RATE_UNSET_INT.

  That is exactly what our regression test creates:

  writeBigEndianInt(fileBytes, infoTagOffset + 12, infoFrameHeader.frameSize);

  So the guard is not hypothetical.

  One possible improvement is to make the code more explicit:

  if (audioLength <= 0) {
    return null;
  }
  int averageBitrate = computeAverageBitrate(audioLength, durationUs);
  if (averageBitrate == C.RATE_UNSET_INT) {
    return null;
  }

  But the current guard is still necessary because computeAverageBitrate can also reject overflow/out-of-range values.

@Tolriq

Tolriq commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@icbaker there's maybe something wrong I got at least one mail with comments but can't find them on Github.

I think everything was addressed in the last update. But Github is a mess.

@icbaker

icbaker commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

I think this comment thread is still open/unresolved: https://github.com/androidx/media/pull/3198/changes#r3339773432

(I agree it's easy to lose comments on github)

@Tolriq

Tolriq commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Okay should be addressed and all the other comment I could find are fixed too hopefully.

Comment thread libraries/extractor/src/main/java/androidx/media3/extractor/mp3/Mp3Extractor.java Outdated
if (shouldFallbackToConstantBitrateSeeking(resultSeeker)) {
// Either we found no seek or VBR info, so we must assume the file is CBR (even without the
// flag(s) being set), or an 'enable CBR seeking flag' is set and we found some seek info, but
// not enough to do 'enhanced' CBR seeking with. In either case, we fall back to CBR seeking

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing this PR I realised this comment is imprecise (and this whole computeSeeker method is a bit of a mess). I'm going to send a clean-up change internally first, then once it's submitted I'll rebase this on top.

copybara-service Bot pushed a commit that referenced this pull request Jun 9, 2026
Consolidate CBR & index-seeking fallback logic and improve control flow
in `computeSeeker` by using early returns and guard clauses.

This change moves the side effect of setting `durationUs` on
`realTrackOutput` to the callsite in `readInternal`, to allow the early
returns.

This came up when I was reviewing Issue: #3198 which modifies this
logic a bit.

PiperOrigin-RevId: 929301753
@icbaker

icbaker commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants