media icon indicating copy to clipboard operation
media copied to clipboard

Better support for PCM audio formats around DefaultAudioSink

Open nift4 opened this issue 7 months ago • 6 comments

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: https://github.com/google/ExoPlayer/issues/10516 Issue: https://github.com/androidx/media/issues/1931

nift4 avatar May 19 '25 17:05 nift4

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

nift4 avatar Jun 03 '25 11:06 nift4

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

ivanbuper avatar Jun 03 '25 12:06 ivanbuper

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

tonihei avatar Jun 03 '25 12:06 tonihei

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 avatar Jun 03 '25 12:06 nift4

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 avatar Jun 03 '25 13:06 nift4

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

nift4 avatar Jun 04 '25 11:06 nift4

@microkatz Hi, can you please take a look at this PR, as it has been a while? Thanks in advance!

nift4 avatar Jul 01 '25 17:07 nift4