Add AC-4 Level-4 ISO base media file format support #1265
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 , OK, I'll update the code.
There was a problem hiding this comment.
@rohitjoins, I have updated the code as your suggestion.
There was a problem hiding this comment.
Hi, @rohitjoins,
Any update on this PR for internal review? Thanks.
There was a problem hiding this comment.
Hi @rohitjoins,
Just wondering if this CL is good to go. Please let us know if you need any further clarification needed regarding the changes here.
We have few other dependent changes in the pipeline that needs to be pushed up for review once this CL gets merged.
Thanks
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.
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.
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.