add support in mediacodecaudiorenderer for 24bit pcm to float #3635
Conversation
There was a problem hiding this comment.
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.
|
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
force-pushed
the
dev-v2_24bitpcm
branch
from
be0d173 to
821ea0e
Compare
|
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. |
|
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! |
|
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?
etcetera |
| @@ -0,0 +1,182 @@ | |||
| package com.google.android.exoplayer2.audio; | |||
There was a problem hiding this comment.
Please could you add an AOSP header? (Just copy from another file and replace the year with 2018.)
| } | ||
| @C.Encoding int encoding = inputEncoding; | ||
| boolean processingEnabled = isInputPcm && inputEncoding != C.ENCODING_PCM_FLOAT; | ||
| if (processingEnabled) { | ||
| AudioProcessor[] activeAudioProcessors = shouldUpResPCMAudio ? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did the rename and changed the local activeAudioProcessors line to availableAudioProcessors
| if (isInputPcm) { | ||
| pcmFrameSize = Util.getPcmFrameSize(inputEncoding, channelCount); | ||
| pcmFrameSize = Util.getPcmFrameSize(shouldUpResPCMAudio |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, I think that was a mixing of changes that got left in and caused discontinuity errors. Fixed.
| buffer.put((byte) (bits & 0xff)); | ||
| buffer.put((byte) ((bits >> 8) & 0xff)); | ||
| buffer.put((byte) ((bits >> 16) & 0xff)); | ||
| buffer.put((byte) ((bits >> 24) & 0xff)); |
There was a problem hiding this comment.
Does buffer.putLong(bits) work here?
| if (isInputPcm) { | ||
| pcmFrameSize = Util.getPcmFrameSize(inputEncoding, channelCount); | ||
| pcmFrameSize = Util.getPcmFrameSize(shouldUpResPCMAudio | ||
| ? C.ENCODING_PCM_FLOAT : inputEncoding, channelCount); | ||
| } | ||
| @C.Encoding int encoding = inputEncoding; | ||
| boolean processingEnabled = isInputPcm && inputEncoding != C.ENCODING_PCM_FLOAT; |
There was a problem hiding this comment.
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?
|
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. |
|
Changes made. Thanks! |
Kept FloatResampleAudioProcessor it's own class in case it can be leveraged/upgraded in the future. This has to be explicitly enabled for use.