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.

Sign up for GitHub

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

Jump to bottom

Fix DTS Express Audio Buffer Underflow Issue. #650

Merged

copybara-service merged 8 commits into androidx:main from cedricxperi:dts-lbr-buffer-underflow-fix

Oct 12, 2023

Contributor

cedricxperi commented Sep 13, 2023

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.

Contributor Author

cedricxperi commented Sep 13, 2023 •

edited

Loading

@tianyif This is the same pull request as #450 which you were assigned in June. I re-applied the changes to the latest main branch. Kindly have a look. Thanks.

oceanjules assigned tianyif

tianyif reviewed

View reviewed changes

libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java Outdated Show resolved Hide resolved

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

                   int bufferSizeUs = passthroughBufferDurationUs;
                   if (encoding == C.ENCODING_AC3) {
                     bufferSizeUs *= ac3BufferMultiplicationFactor;
+                  } else if ((DtsUtil.getCurrentMimeType().contentEquals(MimeTypes.AUDIO_DTS_EXPRESS) && (bitrate

Contributor

tianyif Sep 15, 2023

Could you please add tests for the DTS Express case? DefaultAudioTrackBufferSizeProviderAC3Test can be a good example.

Contributor Author

cedricxperi Sep 18, 2023

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

tianyif reviewed

View reviewed changes

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

+                 * 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;

Contributor

tianyif Sep 18, 2023

nit: How do you think of changing the naming to DTSHD_BUFFER_MULTIPLICATION_FACTOR?

Contributor Author

cedricxperi Sep 18, 2023

Good idea. Done.

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated Show resolved Hide resolved

tianyif reviewed

View reviewed changes

Contributor

tianyif left a comment

Thanks for the changes! I only left some nit comments, and after addressing them I will bring this PR to the internal review stage.

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

+                  /**
+                   * 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}.

Contributor

tianyif Sep 18, 2023

nit: {@link #DTSHD_BUFFER_MULTIPLICATION_FACTOR} instead of @value.

Contributor Author

cedricxperi Sep 18, 2023

Done

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

                   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

Contributor

tianyif Sep 18, 2023

super-nit: DTS-HD (a dash "-" in between)?

Contributor Author

cedricxperi Sep 18, 2023

Done

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

                   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.

Contributor

tianyif Sep 18, 2023

nit: A missing closing ) after the second word "frame"? The corresponding open ( is in the last line.

Contributor Author

cedricxperi Sep 18, 2023

Done

...layer/src/main/java/androidx/media3/exoplayer/audio/DefaultAudioTrackBufferSizeProvider.java Outdated

+                private static final int DTSHD_BUFFER_MULTIPLICATION_FACTOR = 4;
+                /**
+                 * A builder to create {@link DefaultAudioTrackBufferSizeProvider} instances.

Contributor

tianyif Sep 18, 2023

nit: Since there is no actual required diff at lines 71-74, could you please revert these lines back to the original state?

Contributor Author

cedricxperi Sep 18, 2023

Done

tianyif force-pushed the dts-lbr-buffer-underflow-fix branch from 97e2bb0 to fa9adcd Compare

September 18, 2023 12:14

Contributor

tianyif commented Sep 18, 2023

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!

tianyif added the should merge label

tianyif force-pushed the dts-lbr-buffer-underflow-fix branch from fa9adcd to a4b4ff1 Compare

October 11, 2023 16:37

Contributor

tianyif commented Oct 11, 2023 •

edited

Loading

@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.

Contributor Author

cedricxperi commented Oct 11, 2023

@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

cedricxperi and others added 8 commits

October 12, 2023 09:56


          Fix DTS Express Audio Buffer Underflow Issue.

25acaa1


          Remove current mimetype state and add DTSHD test cases

7edfe58


          Rename DTSE to DTSHD

24d03b0


          Update nits

0f200ba


          Format with google-java-format

215f663


          Rebase and resolve conflict in the release notes

49f45c4


          Update the year in the copyright

834b61d


          Modify the comment lines

492048c

tianyif force-pushed the dts-lbr-buffer-underflow-fix branch from a4b4ff1 to 492048c Compare

October 12, 2023 09:56

copybara-service bot merged commit 2421ba4 into androidx:main

1 check passed

rohitjoins pushed a commit that referenced this pull request


          Merge pull request #650 from cedricxperi:dts-lbr-buffer-underflow-fix

c7186c9

PiperOrigin-RevId: 572864175
(cherry picked from commit 2421ba4)

androidx locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels