-
Notifications
You must be signed in to change notification settings - Fork 417
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
Dts direct passthrough support #335
Dts direct passthrough support #335
Conversation
Resolved some of the conflicts in the readme files. Hope it's correct. Thanks. |
@tianyif Hi, thanks for reviewing this pull request. Kindly let me know if there is anything I can do to help move this along. Thank you. |
@christosts Have been waiting on a response from @tianyif for some time now. Am not sure how we can move forward on this pull requets. Please let me know if there is anything I need to do. Thanks. |
Hi @cedricxperi, sorry for the late reply. I was on the other stuffs last week, and I'm starting reviewing this now. |
@tianyif No Worries. Thanks for reviewing this! |
@tianyif Just FYI. I am in Asian (GMT + 8) timezone. Thanks. |
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/common/src/main/java/androidx/media3/common/MimeTypes.java
Outdated
Show resolved
Hide resolved
libraries/common/src/main/java/androidx/media3/common/MimeTypes.java
Outdated
Show resolved
Hide resolved
if (channelCount > maxChannelCount) { | ||
if (format.sampleMimeType == MimeTypes.AUDIO_DTS_X) { | ||
if (channelCount > 10) { | ||
// To fix wrong reporting from device. ChannelCount is reported as 8 for DTS:X P2 |
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.
Could you please explain this fix a bit? Trying to understand this and please let me know if there is anything wrong. Here is my understanding flow:
- The method where this piece of logic is at is
getEncodingAndConfigForPassthrough(Format format)
, by which we query if the passedformat
is supported. - According to the code and comment, when it is a DTS:X P2 stream, the
format
passed-in may have a wrong ChannelCount=8 for some devices, then what is the expected channel count here? Here the logic allows any channel count <= 10 reported with the passed-informat
to be supported, does it mean "for DTS:X P2, the channel count reported can be arbitrary"?
If the last question above is kind of a "yes", would it be possible to consider using getMaxSupportedChannelCountForPassthrough(encoding, sample)
? The benefit of this is, for API >= 29, we can query directly from the platform if a channel count is supported for an encoding. Here is an example of the usage for Dolby object-based surround sound.
Please let me know how you think :)
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.
The answer is yes. Ok, tomorrow I will try and see if I can use getMaxSupportedChannelCountForPassthrough :)
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 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.
@tianyif I mis-intepreted the meaning of arbitrary to mean whether channelcount can be any value when using direct passthrough mode.
In this case, the platform has mistakenly reported max channel count as 8 for DTS:X P2. The correct maximum channel count should be 10. Therefore, I think is it better to stick to the original code that fixes this platform mistake. Please see commit 61a6b99. Thanks
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.
Is there a issue on buganizer or any other issue ID we can add as a comment as to this? It's not uncommon we go back to pieces of code that have assumptions like this one, and having the ability to get more context helps.
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.
Let's clarify this -
the platform has mistakenly reported max channel count as 8 for DTS:X P2
Does the "max channel count" above refer to the maxChannelCount
in the method? That's the maxChannelCount
property of the AudioCapabilities
class. Looking at how we construct AudioCapabilities
, I saw we just used DEFAULT_MAX_CHANNEL_COUNT=8
for most of the cases (only HDMI connection case can lead to a different value). Looks like the value of 8 becomes insufficient, and we should consider to refresh this value or add some flexibility to construct AudioCapabilities
.
Is this corresponding to the issue you mentioned?
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.
@tianyif Yes. It refers to maxChannelCount in the method. I mistakenly assumed that this came from the platform. If DEFAULT_MAX_CHANNEL_COUNT can be increased to 10, then this issue would be resolved. Thanks.
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 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.
case 10: | ||
if (Util.SDK_INT > 31) { | ||
return AudioFormat.CHANNEL_OUT_5POINT1POINT4; | ||
} else { | ||
// This is used by DTS:X P2 with Direct Passthrough Playback. | ||
// Specifying the audio format as 7.1 for 10 channels does not affect the playback. | ||
return AudioFormat.CHANNEL_OUT_7POINT1_SURROUND; | ||
} |
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.
This change conflicts with an ongoing pull request from Dolby in which the case 10 will always return 5.1.4. I will consult with the platform team today what can happen (for SDK <= 31) when creating AudioTrack
with channel count 5.1.4 for DTS:X P2 stream, or with channel count 7.1 for DD+JOC stream.
If there is any potential problem with that, I'm afraid that we should refactor this method getAudioTrackChannelConfig
first to take the mime type into consideration.
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.
Creating an AudioTrack for DTS:X P2 with AudioFormat.CHANNEL_OUT_5POINT1POINT4 (taking the integer value) will fail on MTK platforms. That is why I switched it to AudioFormat.CHANNEL_OUT_7POINT1_SURROUND for API <=31. :)
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.
Acknowledged, we will discuss internally tomorrow on the potential refactorization of getAudioTrackChannelConfig
. Thanks!
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.
@tianyif Any updates on this? Thanks.
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.
We got a hint from the audio framework team that the height channel masks were added in SDK 32 (e.g. CHANNEL_OUT_TOP_BACK_LEFT), so we are now reaching to Dolby to see AudioTrack creation with 5.1.4 will also fail for them when API <= 31. If that's the case, the conflicts won't be there. But we are also waiting for a concrete answer from audio framework team as well.
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/common/src/main/java/androidx/media3/common/util/Util.java
Outdated
Show resolved
Hide resolved
libraries/common/src/main/java/androidx/media3/common/util/Util.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Outdated
Show resolved
Hide resolved
libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioCapabilities.java
Show resolved
Hide resolved
1a77a92
to
730cfec
Compare
I already merged the pull request internally. And the change will be available next time new commits being pushed to |
Resolved Merge Conflict during cherrypicking PiperOrigin-RevId: 535255453 (cherry picked from commit c3dd88d)
This adds DTS Audio Support for Direct Passthrough playback. This is required for Tunnel mode playback on Android TVs that supports tunnel mode playback (e.g. MTK and Realtek based TVs). Test streams will be provided during the review if required.
Thanks.