Fix an issue in Audio transcoding where target-sample-rate is less than the source-sample-rate #111
Conversation
…less than the source sample-rate, so here we need to do a down-sampling, and I found this fix by another third party, but I did some changes.
| */ | ||
| void renderFrame(@Nullable Frame frame, long presentationTimeNs); | ||
| void renderFrame(@Nullable Frame inputFrame, long presentationTimeNs, MediaFormat sourceMediaFormat, |
There was a problem hiding this comment.
Why do we have to break a public API? Source and target formats are passed in init method, can't we just hold on to them in the renderer? They don't usually change during transcoding.
There was a problem hiding this comment.
No, we don't need to break the renderFrame() method, but yes, they change after init method, so I'll add a new method onMediaFormatChanged() to Renderer interface, and will call it each time the MediaCodec.INFO_OUTPUT_FORMAT_CHANGED is returned
| public void resample(@NonNull ByteBuffer inputBuffer, int inputSampleRate, @NonNull ByteBuffer outputBuffer, | ||
| int outputSampleRate, int channels) { | ||
| super.resample(inputBuffer, inputSampleRate, outputBuffer, outputSampleRate, channels); | ||
| // TODO currently up-sampling not supported because it's need some enhancements, so fallback to PASSTHROUGH approach |
There was a problem hiding this comment.
We probably don't want to do this here. I think we can remove this entire class for now and bring it in later.
| /** | ||
| * An {@link AudioResampler} that downsamples from a higher sample rate to a lower sample rate. | ||
| */ | ||
| public class DownsampleAudioResampler implements AudioResampler { |
There was a problem hiding this comment.
Is this adapted code from a different open source project? In that case we have to keep their copyright notice and add a note that we made modifications (feel free to add your name as author of modifications)
| * @param outputSampleRate the output sample rate | ||
| * @param channels the number of channels | ||
| */ | ||
| void resample(@NonNull final ByteBuffer inputBuffer, int inputSampleRate, @NonNull final ByteBuffer outputBuffer, int outputSampleRate, int channels); |
There was a problem hiding this comment.
I think, factory pattern would work better here to create correct resampler implementation for given source/target formats. We would need instantiate only one class.
| * Resamples audio data. See {@link UpsampleAudioResampler} or | ||
| * {@link DownsampleAudioResampler} for concrete implementations. | ||
| */ | ||
| public interface AudioResampler { |
There was a problem hiding this comment.
For all new classes that are not modified classes from other project please add LinkedIn copyright notice
- Add a copyright notice fot new classes - Changed the logic of getting an updated sourceMediaFormat and targetAudioFormat
| private final AudioResampler audioResampler = new DefaultAudioResampler(); | ||
|
|
||
| @VisibleForTesting | ||
| static int getSampleRate(MediaFormat format) { |
There was a problem hiding this comment.
changed to non-static method
| } | ||
|
|
||
| @VisibleForTesting | ||
| static int getChannels(MediaFormat format) { |
There was a problem hiding this comment.
removed from here
| * @param outputSampleRate the output sample rate | ||
| * @param channels the number of channels | ||
| */ | ||
| void resample(@NonNull final ByteBuffer inputBuffer, int inputSampleRate, @NonNull final ByteBuffer outputBuffer, int outputSampleRate, int channels); |
There was a problem hiding this comment.
I think it would be more flexible to pass MediaFormat for source and target. Resampler implementation would know how to extract information it needs from it. And we would not need to change method signature (breaking change) if we needed to pass in more parameters than we do now.
| */ | ||
| void resample(@NonNull final ByteBuffer inputBuffer, int inputSampleRate, @NonNull final ByteBuffer outputBuffer, int outputSampleRate, int channels); | ||
|
|
||
| AudioResampler DOWNSAMPLE = new DownsampleAudioResampler(); |
There was a problem hiding this comment.
Interface being aware of two of its implementations does not sounds right. We should use factory pattern to instantiate a correct resampler implementation in onMediaFormatChanged method. By default, it should be PassthroughResampler, of course.
|
|
||
| @Override | ||
| public void resample(@NonNull ByteBuffer inputBuffer, int inputSampleRate, @NonNull ByteBuffer outputBuffer, int outputSampleRate, int channels) { | ||
| if (inputSampleRate > outputSampleRate) { |
There was a problem hiding this comment.
This logic can move into onMediaFormatChanged method. That way we would remove this entire class and only one instance of resampler would be created. This would also remove those two constants with implementations from AudioResampler interface.
| */ | ||
| public class DownsampleAudioResampler implements AudioResampler { | ||
|
|
||
| private static float ratio(int remaining, int all) { |
There was a problem hiding this comment.
I guess this was original static and got copied there? Can we make this not static?
| @@ -42,8 +50,35 @@ public PassthroughSoftwareRenderer(@NonNull Encoder encoder, long frameWaitTimeo | |||
| this.frameWaitTimeoutUs = frameWaitTimeoutUs; | |||
| } | |||
|
|
|||
| private int getSampleRate(MediaFormat format) { | |||
There was a problem hiding this comment.
Do we need this method?
| if (sourceAudioFormat == null || targetAudioFormat == null) { | ||
| return; | ||
| } | ||
| int inputSampleRate = getSampleRate(sourceAudioFormat); |
There was a problem hiding this comment.
Can't we use AudioResampler method here? It seems to be doing the same thing.
Fix an issue in Audio transcoding, where the target sample-rate is less than the source sample-rate, so here we need to do a down-sampling, and I found this fix by another third party, but I did some changes.