AudioDecoder: specify desired num_channels #678
Conversation
facebook-github-bot
added
the
CLA Signed
| AudioStreamOptions audioStreamOptions; | ||
| audioStreamOptions.sampleRate = sample_rate; | ||
| audioStreamOptions.numChannels = num_channels; |
There was a problem hiding this comment.
This implicitly converts an optional int64 to an optional int. The same implicit conversion already happens for sample_rate, and for the VideoStreamOptions' fields like height, width, and ffmpegThreadCount.
I'll open an issue to fix / check these all at once.
| } | ||
| convertedAVFrame = convertAudioAVFrameSampleFormatAndSampleRate( | ||
| convertedAVFrame = convertAudioAVFrameSamples( |
There was a problem hiding this comment.
I renamed the function, because it converts too many things to list all of them now. @scotts I also remember that you wanted to align all the sourceStuff to srcStuff - which I'll do as a quick follow-up!
| int desiredSampleRate) { | ||
| int desiredSampleRate, | ||
| const UniqueAVFrame& srcAVFrame, | ||
| int desiredNumChannels) { |
There was a problem hiding this comment.
slightly changed the signature to take an AVFrame instead of the AVCodecContext. This is where we get the source num_channel and source channel layout. It's more accurate to get it from the AVFrame, although they should both always be the same.
There was a problem hiding this comment.
Nit: I prefer for the primary FFmpeg data structures to appear first in a function. I think of these functions as if they are methods on those structures, and that gets reinforced by making them the first parameter. I think I understand the current order, which srcFrame is placed next to desiredNumChannels because you're getting the source number of channels from srcFrame. But given what we're doing, I think of this function as "Create a swrContext from this AVFrame with these desire parameters."
There was a problem hiding this comment.
I agree that for functions that look like methods, we want the FFmpeg struct to be the first. The reason I removed the avCodecContext from the top and now use an AVFrame is precisely because this function shouldn't read as a method.
I think I understand the current order, which srcFrame is placed next to desiredNumChannels because you're getting the source number of channels from srcFrame
Yes, you're correct. Ideally, we shouldn't need to pass the AVFrame at all, we should just pass its AVChannelLayout as sourceChannelLayout. But unfortunately, AVChannelLayout only exists from FFmpeg 5. That's the main reason we're passing an AVFrame: it's so that we can call createSwrContext() uniformly across ffmpeg versions, and contain the preproc #ifdef directives into FFmpegCommon.cpp.
Does that make sense?
Basically: we're passing an AVFrame because of some FFmpeg version constraint, but really this just be "source channel layout", and thus this isn't a method on an AVFrame at all.
Closes #675