Skip to content

Better support for PCM audio formats around DefaultAudioSink #2445

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nift4
Copy link

@nift4 nift4 commented May 19, 2025

This change:

  • abstracts away the fact that SonicAudioProcessor does not support float PCM from the audio sink to the audio processor chain (easier to replace)
  • makes the audio sink allow custom audio processors with any output format
  • adds a new mode to DefaultAudioSink that allows any supported linear PCM format to be output into the AudioTrack

Issue: google/ExoPlayer#10516
Issue: #1931

Copy link
Contributor

@ivanbuper ivanbuper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nift4, thanks for your PR. From what I can see, the PR seems to be doing a few different things:

  1. Modifying DefaultAudioSink to allow an arbitrary PCM encoding (including changes in DefaultRenderersFactory).
  2. Adding support in the codebase for PCM_DOUBLE and DSD.

These two items seem to be independent changes. Can you please split them out onto separate PRs to ease the review process? Thanks!

This change:
- abstracts away the fact that SonicAudioProcessor does not support float PCM
  from the audio sink to the audio processor chain (easier to replace)
- makes the audio sink allow custom audio processors with any output format
- adds a new mode to DefaultAudioSink that allows any supported linear PCM format
  to be output into the AudioTrack

Issue: google/ExoPlayer#10516
Issue: androidx#1931
@nift4 nift4 force-pushed the outputformatext branch from 77bccf6 to 9231816 Compare June 3, 2025 11:24
@nift4
Copy link
Author

nift4 commented Jun 3, 2025

Hi @ivanbuper, no problem, I have split the PR into this one and #2500. Thanks in advance for the review :)

@ivanbuper
Copy link
Contributor

@microkatz Would you mind taking a look at this one? I can do #2500. Thanks!

@ivanbuper ivanbuper requested review from microkatz and ivanbuper and removed request for ivanbuper June 3, 2025 12:22
@tonihei
Copy link
Collaborator

tonihei commented Jun 3, 2025

@microkatz (or whoever will look into this one): there is also the internal draft cl/592161260 that is roughly the same as this PR, so we should check where they differ and what makes most sense for the API.

@nift4: Some question based on a quick look:

  • Our internal draft so far just had setEnableHighResolutionPcmOutput instead of setEnableFloatOutput. Are the additional options provided by this PR needed for your integration or just meant to provide all the options?
  • The internal draft also specifically checks whether the output path actually supports the PCM version directly (which is different from the version in this PR that always claims all PCM versions are supported on newer API versions). Is this to enable specific audio processors with a different PCM version? Otherwise, if the actual output path doesn't support a certain PCM bit depth, it will need to be transcoded again to avoid unexpected crashes.
  • And last question: there doesn't seem to be a change in the MediaCodecAudioRenderer logic that determines the PCM version produced by the codec. I think this means that the codec will always produce float PCM at the moment and the new paths are not actually used unless you play PCM (without decoding) directly?

@nift4
Copy link
Author

nift4 commented Jun 3, 2025

Hi @tonihei ,
1.
I just deprecated setEnableFloatOutput but kept it for backwards compatibility. I don't see any need for a float output filter in the longer term, and if backward compat of unstable API, is not of concern, removing is totally fine for me.
2.
Audioflinger can do the transcoding in optimized native code (compared to ART AOT java code) no matter if actual output device supports it or not, so if API level (hence system-side audioflinger) supports the PCM mode, it should always work, or am I missing something? Which function was actually used for checking (because the 'getDirectPlaybackSupport' check will cause false negatives for normal AudioTrack usage as some devices do not support direct PCM at all - for example Pixel 7a)?
3.
MediaCodec integration is pending since 3 months, #2218 I would appreciate review :) I tested this PR together with #2218 for expected functionality and it works fine.

@nift4
Copy link
Author

nift4 commented Jun 3, 2025

Oh I just realized I missed a question,

Is this to enable specific audio processors with a different PCM version?

No, the idea is that audio sink does least possible work. For example, imagine following scenario:

  • Audio output device supports int32 only (an audio device in android only ever has one format sent to it, and this never changes until device reboot or device eject, with the exception of USB devices)
  • Song is int24, sink will not output int24 because device doesn't support it
  • So sink falls back to int16 and converts in java
  • But because device only supports int32 audioflinger will convert a second time which wastes resources

always avoiding work in audio sink and letting audioflinger do the work avoids these without additional effort in coding, and will provide best battery life because conversion will be done in native code

@nift4 nift4 requested a review from ivanbuper June 4, 2025 11:41
@nift4
Copy link
Author

nift4 commented Jun 4, 2025

Sorry for the review request, it seems to have requested it from the wrong person. It seems like I don't have permission to assign / request microkatz

@tonihei tonihei assigned microkatz and unassigned ivanbuper Jun 4, 2025
@tonihei tonihei removed the request for review from ivanbuper June 4, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants