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

Do not flush eac3(joc) decoder on reuse #1346

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dwhea
Copy link
Contributor

@dwhea dwhea commented May 6, 2024

eac3(joc) decoders do not need to be flushed to be reused for the next compatible track. This change allows for gapless playback on devices with Dolby decoders that require internal re-initialization on flush.

eac3(joc) decoders do not need to be flushed to be reused for the next
compatible track. This change allows for gapless playback on devices
with Dolby decoders that require internal re-initialization on flush.
@microkatz
Copy link
Contributor

Hello @dwhea,

Thank you for creating this pull request! Just to confirm, may I ask if you have any data backing up that the eac3(joc) has independent samples (like xHE-AAC codec)?

@mzchan1
Copy link

mzchan1 commented May 16, 2024

Hi @microkatz,

To respond to your query, eac3-joc frames are structured such that each frame represents an independent time slice (e.g. 32ms of audio) that is encoded such that it does not depend on other frames, and is packaged as a single access unit in container formats such as MP4. eac3-joc is encoded via the Enhanced AC-3 bitstream format which is documented here: https://www.etsi.org/deliver/etsi_ts/102300_102399/102366/01.03.01_60/ts_102366v010301p.pdf

The core problem this pull request solves is to avoid issuing a flush event to the eac3/eac3-joc MediaCodec component under the following playback conditions:

  1. Playing back a playlist containing a series of media assets intended to seamlessly transition from the end of one media asset to the beginning of the next media asset without introducing audible gaps or other audio artifacts.
  2. During streaming audio playback (e.g. via DASH) the switching of representations within an adaptation set does not introduce audible gaps or other audio artifacts, even if the asset switches between eac3 and eac3-joc audio formats.

Please let me know if you have any further queries.

Thanks.

@dwhea
Copy link
Contributor Author

dwhea commented May 22, 2024

Thanks @mzchan1 for responding.

@microkatz
Copy link
Contributor

Hi @dwhea,

Sorry for the delay. Would it be possible to open a new PR from an individual-owned fork? We can't push changes to organization-owned forks like this one unless we have collaborator access. If that's not possible then we can still merge this PR but it will result in an 'evil' merge. See more info here: https://github.com/androidx/media/blob/release/CONTRIBUTING.md#push-access-to-pr-branches

@dwhea
Copy link
Contributor Author

dwhea commented Aug 20, 2024

Hi @microkatz , no problem. We're not able to open PR from individual-owned fork, so merging as an "evil merge" is OK for us. This was discussed between us two parties around January 2024 and we came to that decision then.

@@ -479,6 +479,17 @@ public DecoderReuseEvaluation canReuseCodec(Format oldFormat, Format newFormat)
}
}

// For eac3 and eac3-joc formats, adaptation is possible without reconfiguration or flushing.
if (discardReasons == 0 && (MimeTypes.AUDIO_E_AC3_JOC.equals(mimeType)
Copy link
Contributor

@microkatz microkatz Aug 21, 2024

Choose a reason for hiding this comment

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

@dwhea
Do Dolby Audio decoders use any initalization data provided by the stream? Just checking if this check needs to occur after the initialization data comparison right below.

Copy link
Contributor Author

@dwhea dwhea Aug 23, 2024

Choose a reason for hiding this comment

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

There's no initialization data for Dolby codecs.

@microkatz
Copy link
Contributor

Hi @microkatz , no problem. We're not able to open PR from individual-owned fork, so merging as an "evil merge" is OK for us. This was discussed between us two parties around January 2024 and we came to that decision then.

If you are not opening a PR from an individual-owned fork then it still easier for us if given collaborator access. In that way we can submit commits to this branch during the review process.

// For eac3 and eac3-joc formats, adaptation is possible without reconfiguration or flushing.
if (discardReasons == 0 && (MimeTypes.AUDIO_E_AC3_JOC.equals(mimeType)
|| MimeTypes.AUDIO_E_AC3.equals(mimeType))) {
return new DecoderReuseEvaluation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to add a unit test to the MediaCodecInfoTest file that given a call of canReuseCodec for dolby content of the same mimetype that REUSE_RESULT_YES_WITHOUT_RECONFIGURATION is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we will add those tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I force pushed a commit to DolbyLaboratories@ade6e4f which rebased to Google upstream main branch, and added the requested tests.

I'm not sure how to update this pull request with those changes. Please advise if I need to do anything, or whether you can pull that new commit into this pull request (or create a new pull request).

Copy link
Contributor

@microkatz microkatz Aug 27, 2024

Choose a reason for hiding this comment

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

I believe that you need to make the commits to your topic branch, DolbyLaboratories:dlb/ddp-gapless, and then rebase the topic branch back onto main. With a git --force push command that should update the PR with your requested commits and update the PR's base commit as main head.

Without the collaborator access I'm unable to do this for you.

I also made a comment in the referenced commit as well.
DolbyLaboratories@ade6e4f#r145885998

@benlancaster
Copy link

benlancaster commented Sep 5, 2024

I'm somewhat surprised by this change. My team has observed several cases on many devices where E-AC-3 codecs need not only a flush but also a reconfiguration when transitioning between two E-AC-3 tracks of differing bitrates, or E-AC-3 to E-AC-3+JOC.

@microkatz
Copy link
Contributor

@benlancaster

I'm somewhat surprised by this change. My team has observed several cases on many devices where E-AC-3 codecs need not only a flush but also a reconfiguration when transitioning between two E-AC-3 tracks of differing bitrates, or E-AC-3 to E-AC-3+JOC.

Thanks for the query!

Although not expressly noted in the description, if you check out the proposed commit, you'll see that the new conditional check to reuse the decoder(ln: 482) occurs after the checks on sampleMimetype(ln: 414) and sampleRate(ln: 451). Does this help alleviate your concerns?

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.

4 participants