add support in mediacodecaudiorenderer for 24bit pcm to float by drhill · Pull Request #3635 · google/ExoPlayer

drhill · 2017-12-23T18:30:51Z

Kept FloatResampleAudioProcessor it's own class in case it can be leveraged/upgraded in the future. This has to be explicitly enabled for use.

andrewlewis

Thanks for the PR!

Would it make sense for FloatResamplingAudioProcessor to support 32-bit PCM to float conversion (in addition to 24-bit)?
I think we should try to move the resampling step into DefaultAudioSink and leave MediaCodecAudioRenderer unmodified, so that other audio renderers could also benefit from this functionality. This would also remove the need to input/output/drain the audio processor in the renderer. I suggest we add a constructor argument enableResamplingToFloat in DefaultAudioSink and apply the FloatResamplingAudioProcessor as a special case. This may be a bit tricky as we will need to disable the int PCM processors under the right circumstances. If that sounds reasonable I can take a look at doing that part of the change.

drhill · 2018-01-06T04:30:15Z

lol... isn't that what I had a few months ago? Both 32-bit pcm to float conversion (I removed as I have no media of that type but it is certainly a good idea) and the floatresample code in the AudioSink (much older code now). I'll try to give it a look this weekend. Thanks!

…n defaultaudiosync to use that resample when audio input is 24/32bit pcm and the new flag is enabled

drhill · 2018-01-06T05:30:25Z

Ok, got it started though tt looks a little sloppy having the hiResProcessors and the regular ones. 24bit pcm audio converts to 32 float fine. DTS, DTS-HD, DD, TrueHD, and 16 bit PCM all seem unchanged. Didn't test 32bit int to 32bit float as I don't have video samples of that.

andrewlewis · 2018-01-06T10:01:26Z

Yes, this is doing float to integer conversion in the audio sink as you had a while back. I thought before that it wasn't worth the complexity to convert 24-bit and 32-bit integer input to float, but actually it is needed to make this complete. Sorry for having to split this up and iterate.

I don't think it's so bad having the int -> float processor separate from the other ones. Does it ever make sense to use the processors together? Going via 16-bit makes it not worth doing float output.

Had a quick look and this looks good. I will take a proper look next week and hopefully we can get it submitted soon. Thanks!

drhill · 2018-01-06T12:54:06Z

No problem.

I think the separate processors makes sense. Maybe add a layer of extraction for each processor which encapsulates the current 16 bit processor with any float processor and have the encapsulating class direct the inputs to the proper processor or just do nothing if the input isn't supported?

........./ 16BitResampler
Resampler
.........\ FloatResampler

....................../ 16bitChannelMapperProcessor
ChannelMapperProcessor
......................\ pass through untouched non-16bit data

....................../ 16bitTrimmingAudioProcessor
TrimmingAudioProcessor
......................\ pass through untouched non-16bit data

etcetera
Editing formatting

andrewlewis

Looks good. Just some minor things to fix up then we can merge this. Thanks again!

andrewlewis · 2018-01-12T12:05:06Z

@@ -0,0 +1,182 @@
+package com.google.android.exoplayer2.audio;


Please could you add an AOSP header? (Just copy from another file and replace the year with 2018.)

andrewlewis · 2018-01-12T12:14:17Z

    @C.Encoding int encoding = inputEncoding;
    boolean processingEnabled = isInputPcm && inputEncoding != C.ENCODING_PCM_FLOAT;
    if (processingEnabled) {
+      AudioProcessor[] activeAudioProcessors = shouldUpResPCMAudio ?


The "active" part of the name may be misleading because these audio processors won't necessarily all end up being active (in the terminology of AudioProcessor.isActive()).

How about renaming the final fields to something like toIntPcmAvailableAudioProcessors and toFloatPcmAvailableAudioProcessors? The names are a bit long but hopefully clearer. Or, if not doing that rename, this line could be AudioProcessor[] availableAudioProcessors = shouldUpResPCMAudio ? hiResAvailableAudioProcessors : this.availableAudioProcessors. Same applies for other occurrence of activeAudioProcessors.

Did the rename and changed the local activeAudioProcessors line to availableAudioProcessors

andrewlewis · 2018-01-12T13:03:46Z

+     (inputEncoding == C.ENCODING_PCM_24BIT || inputEncoding == C.ENCODING_PCM_32BIT);
    if (isInputPcm) {
-      pcmFrameSize = Util.getPcmFrameSize(inputEncoding, channelCount);
+      pcmFrameSize = Util.getPcmFrameSize(shouldUpResPCMAudio


pcmFrameSize is used to calculate the number of submitted frames (not the number of output frames), so shouldn't this be left as it was?

Yes, I think that was a mixing of changes that got left in and caused discontinuity errors. Fixed.

andrewlewis · 2018-01-12T13:12:34Z

+    buffer.put((byte) (bits & 0xff));
+    buffer.put((byte) ((bits >> 8) & 0xff));
+    buffer.put((byte) ((bits >> 16) & 0xff));
+    buffer.put((byte) ((bits >> 24) & 0xff));


Does buffer.putLong(bits) work here?

putInt works

andrewlewis · 2018-01-12T13:13:37Z

+       ? C.ENCODING_PCM_FLOAT : inputEncoding, channelCount);
    }
    @C.Encoding int encoding = inputEncoding;
    boolean processingEnabled = isInputPcm && inputEncoding != C.ENCODING_PCM_FLOAT;


For the 24-bit integer PCM -> float path we can't apply playback parameters, but processingEnabled gets set to true so setPlaybackParameters will try to apply the playback parameters. For now shall we just add another flag canApplyPlaybackParams which is processingEnabled && !shouldUpResPCMAudio and change the check in setPlaybackParameters to use that?

andrewlewis · 2018-01-12T13:18:55Z

I suggest we go with separate available audio processors references for this change as it isn't much code, and have a think about doing #3635 (comment) later on.

drhill · 2018-01-14T16:03:18Z

Changes made. Thanks!

googlebot added the cla: yes label Dec 23, 2017

ojw28 assigned andrewlewis Jan 2, 2018

andrewlewis reviewed Jan 2, 2018

View reviewed changes

drhill added 2 commits January 5, 2018 23:31

add support in mediacodecaudiorenderer for 24bit pcm to float

ca0c090

moved floatresample into defaultaudiosink and added new constructor i…

821ea0e

…n defaultaudiosync to use that resample when audio input is 24/32bit pcm and the new flag is enabled

drhill force-pushed the dev-v2_24bitpcm branch from be0d173 to 821ea0e Compare January 6, 2018 05:26

andrewlewis reviewed Jan 12, 2018

View reviewed changes

code review changes and fix for discontinuity

aaf469c

andrewlewis added the should merge label Jan 15, 2018

ojw28 merged commit 4d2e0bf into google:dev-v2 Jan 24, 2018

drhill deleted the dev-v2_24bitpcm branch January 24, 2018 22:31

google locked and limited conversation to collaborators Jun 29, 2018

		@@ -0,0 +1,182 @@
		package com.google.android.exoplayer2.audio;

Conversation

drhill commented Dec 23, 2017

Uh oh!

andrewlewis left a comment

Choose a reason for hiding this comment

Uh oh!

drhill commented Jan 6, 2018

Uh oh!

drhill commented Jan 6, 2018

Uh oh!

andrewlewis commented Jan 6, 2018

Uh oh!

drhill commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlewis commented Jan 12, 2018

Uh oh!

drhill commented Jan 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drhill commented Jan 6, 2018 •

edited

Loading