Add AC-4 Level-4 ISO base media file format support#1265
Add AC-4 Level-4 ISO base media file format support#1265ybai001 wants to merge 8 commits intoandroidx:mainfrom
Conversation
Merge latest code from androidx/media main branch
…ries/media into dlb/ac4-level4/dev
…ries/media into dlb/ac4-level4/dev
| // Using first presentation (default presentation) channel count | ||
| int presentationIndex = 0; | ||
| Ac4Presentation ac4Presentation = | ||
| Objects.requireNonNull(ac4Presentations.get(presentationIndex)); |
There was a problem hiding this comment.
Why are we looping over all presentationIndex above and storing it in a Map when all we require is the ac4Presentation for the index 0?
Is it to verify if the file doesn't have bad data? Even in that case storing it in a Map seems an overkill, when we don't intend to use the map for anything else but index 0.
There was a problem hiding this comment.
@rohitjoins,
This AC4 parser implementation is similar as the one in Android Framework: https://cs.android.com/android/platform/superproject/main/+/main:frameworks/av/media/module/extractors/mp4/AC4Parser.cpp;drc=2e87c835abea90b0cf61b42dc9aa8a882f2ef5aa;l=292. It parses all the information for all presentations because there is API https://developer.android.com/reference/android/media/MediaExtractor#getAudioPresentations(int) to fetch the presentations info for appointed audio track.
Since AndroidX Media extractor doesn't support AudioPresentation, I know this loop is redundant now.
But I know Android framework MPEGExtractor was deprecated and it will be replaced by AndroidX Media extractor. Although AndroidX Media doesn't support AudioPresentation now but I believe it will support it sooner or later. That's why I use a Map to store all audio presentation info and keep this logic in my implementation.
If one audio track includes multiple audio presentations, and if application doesn't set which presentation should be played, the Dolby AC-4 decoder will decode the default presentation (the first presentation).
There was a problem hiding this comment.
I would suggest to remove any logic which will be used in the future as it can be added when required. For now we should only parse and use the first presentation.
There was a problem hiding this comment.
@rohitjoins, I have updated the code as your suggestion.
There was a problem hiding this comment.
Hi @sqshet-dlb,
I’m currently making some updates to the PR, such as adding more documentation and refactoring some parts into new methods before sending it for internal review. I’ll keep you updated on the progress and aim to merge it soon.
Hi, @rohitjoins, as you mentioned on July 2nd,
I'll take a look and send this PR for internal review.
It's nearly 3 months. Could you help to explain why this pull request has not been sent for internal review? Could you give me a deadline? This is very important for us since it blocks several other contributions as @sqshet-dlb said.
There was a problem hiding this comment.
Hi, @rohitjoins
Could you help to provide us any update about this contribution?
Thanks.
There was a problem hiding this comment.
@rohitjoins , there is a merge conflict with IAMF code. Do I need to resolve this conflict? Because you said you would send this for internal code review, I'm not sure whether I should modify code again. Thanks.
There was a problem hiding this comment.
Hey @ybai001,
No need to update this PR as I already do it for the internal version. If no blockers come up in review, this should be merged this week.
There was a problem hiding this comment.
@rohitjoins, thanks for the quick feedback. :)
PiperOrigin-RevId: 696157037
Hi, @rohitjoins I truly appreciate your effort on this pull request. It's almost one year! :) I understood this was a big change in extractor and it was related to Dolby specific format that you were not familiar with. I have verified it on my test device based on the lastest main branch code. It can work. So thanks again! |
PiperOrigin-RevId: 696157037 (cherry picked from commit 74611bb)
PiperOrigin-RevId: 696157037 (cherry picked from commit 74611bb)
PiperOrigin-RevId: 696157037 (cherry picked from commit 74611bb)
3 participants
This pull request is same as #875. Since 875 is broken due to too many commit numbers, I re-created this new one.
========================
AC-4 Level-4 (Object Based Audio) is a Dolby official audio stream type for Atmos Music. This code update enables Android Media3 support local MP4 and MPEG-DASH AC-4 Level-4 playback.