-
Notifications
You must be signed in to change notification settings - Fork 404
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
base: main
Are you sure you want to change the base?
Conversation
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.
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)? |
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:
Please let me know if you have any further queries. Thanks. |
Thanks @mzchan1 for responding. |
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 |
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
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? |
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.