-
Notifications
You must be signed in to change notification settings - Fork 361
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
Fix DTS Express Audio Buffer Underflow Issue. #650
Fix DTS Express Audio Buffer Underflow Issue. #650
Conversation
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
@@ -232,7 +254,20 @@ protected int getPassthroughBufferSizeInBytes(@C.Encoding int encoding, int bitr | |||
int bufferSizeUs = passthroughBufferDurationUs; | |||
if (encoding == C.ENCODING_AC3) { | |||
bufferSizeUs *= ac3BufferMultiplicationFactor; | |||
} else if ((DtsUtil.getCurrentMimeType().contentEquals(MimeTypes.AUDIO_DTS_EXPRESS) && (bitrate |
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 add tests for the DTS Express case? DefaultAudioTrackBufferSizeProviderAC3Test
can be a good example.
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.
Done.
...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java
Outdated
Show resolved
Hide resolved
...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java
Outdated
Show resolved
Hide resolved
* Default multiplication factor to apply to DTS Express passthrough buffer to avoid underruns | ||
* on some devices (e.g., Xiaomi A2 TV). | ||
*/ | ||
private static final int DTSE_BUFFER_MULTIPLICATION_FACTOR = 4; |
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.
nit: How do you think of changing the naming to DTSHD_BUFFER_MULTIPLICATION_FACTOR
?
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.
Good idea. Done.
...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java
Outdated
Show resolved
Hide resolved
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.
Thanks for the changes! I only left some nit comments, and after addressing them I will bring this PR to the internal review stage.
/** | ||
* Sets the multiplication factor to apply to the passthrough buffer for DTS-HD (DTS Express) | ||
* to avoid underruns on some devices (e.g., Xiaomi A2 TV). Default is | ||
* {@value #DTSHD_BUFFER_MULTIPLICATION_FACTOR}. |
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.
nit: {@link #DTSHD_BUFFER_MULTIPLICATION_FACTOR}
instead of @value
.
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.
Done
@@ -232,7 +258,13 @@ protected int getPassthroughBufferSizeInBytes(@C.Encoding int encoding, int bitr | |||
int bufferSizeUs = passthroughBufferDurationUs; | |||
if (encoding == C.ENCODING_AC3) { | |||
bufferSizeUs *= ac3BufferMultiplicationFactor; | |||
} else if (encoding == C.ENCODING_DTS_HD) { | |||
// DTSHD (DTS Express) for streaming uses a frame size (number of audio samples per channel |
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.
super-nit: DTS-HD (a dash "-" in between)?
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.
Done
@@ -232,7 +258,13 @@ protected int getPassthroughBufferSizeInBytes(@C.Encoding int encoding, int bitr | |||
int bufferSizeUs = passthroughBufferDurationUs; | |||
if (encoding == C.ENCODING_AC3) { | |||
bufferSizeUs *= ac3BufferMultiplicationFactor; | |||
} else if (encoding == C.ENCODING_DTS_HD) { | |||
// DTSHD (DTS Express) for streaming uses a frame size (number of audio samples per channel | |||
// per frame of 4096. This requires a higher multiple for the buffersize computation. |
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.
nit: A missing closing )
after the second word "frame"? The corresponding open (
is in the last line.
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.
Done
private static final int DTSHD_BUFFER_MULTIPLICATION_FACTOR = 4; | ||
|
||
/** | ||
* A builder to create {@link DefaultAudioTrackBufferSizeProvider} instances. |
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.
nit: Since there is no actual required diff at lines 71-74, could you please revert these lines back to the original state?
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.
Done
97e2bb0
to
fa9adcd
Compare
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks! |
fa9adcd
to
a4b4ff1
Compare
@cedricxperi Sorry for the delay on this PR. I was just back from the holiday :) Quick question for double check: Does this buffer underrun issues apply to all devices or only applies to some of them like Xiaomi A2 TV? I'm asking to see if we have to mention the devices in the comment lines. |
Hi @tianyif no worries. This will happen for all devices as it is not a processor speed issue. Thanks |
a4b4ff1
to
492048c
Compare
PiperOrigin-RevId: 572864175 (cherry picked from commit 2421ba4)
Problem Description
DASH DTS Express audio playback results in buffer underrun issues. This can be heard as regular blips in the output.
Solution
DTS Express audio for streaming, uses a frame size (number of audio samples per channel per frame) of 4096. This requires a higher multiple for the buffersize computation.