valbok / QtAVPlayer

Free and open-source Qt Media Player library based on FFmpeg, for Linux, Windows, macOS, iOS and Android.
MIT License
310 stars 61 forks source link

QAVAudioFrame::data() reinitializes the resampler over and over again #484

Closed pasveer closed 1 week ago

pasveer commented 5 months ago

In the file qavaudioframe.cpp there is a function named QAVAudioFrame::data().

It uses the resampler swr_convert() (from swresample.h) in case the input stream's features (channel count, sample rate or format) does not match the desired output format (as returned by QAVAudioFrame::format()).

If this is the case, the bool needsConvert is true and then it is checked whether a (new) instance of the resampler need to be created, by calling swr_alloc_set_opts(). Usually this happens only once when a playback is started, and subsequent calls to swr_convert() does the actual resampling. It is important the instance stays alive for proper resampling.

But because the resampler-state variables d->swr_ctx, d->inSampleRate and d->outAudioFormat are members of the current QAVAudioFrame, this essential resampler-state data is not available anymore on the next cycle, when the next QAVAudioFrame arrives. As a result swr_alloc_set_opts() is called again and again, and proper resampling of the audio fails...

Solution: I think that the variables swr_ctx, inSampleRate and outAudioFormat should be part of QAVAudioOutputPrivate instead, where housekeeping is done per audio output. After all, each (imagine you have more than one) output device -- or any other samples-consumer -- have different needs.

My quick and dirty fix: QAVAudioFrame::data() is called from QAVAudioOutputPrivate::readData().

I created this struct:

typedef struct
{
    int d_inSampleRate;
    QAVAudioFormat d_outAudioFormat;
    /*SwrContext*/ void * d_swr_ctx;
} resampler_context_t;

This struct is a member of QAVAudioOutputPrivate and I pass a reference to my slightly modified QAVAudioFrame::data() --> QAVAudioFrame::data(resampler_context_t & resampler_context).

Now, resampler_context is used to store the resampler-state so it won't get lost. Now swr_alloc_set_opts() is called once with the desired parameters and after that, on every next call, the same instance of the resampler is used and resampling works well.

Like I said, my solution is really an ugly patch. Notice that I use a void-pointer as QAVAudioOutputPrivate doesn't need to know what SwrContext looks like. There must be a cleaner way that follows the design of QtAVPlayer.

Finally, there is a strange line in the code where outCount is calculated. This line should simply be: int outCount = frame->nb_samples; av_samples_get_buffer_size takes care of the math to calculate the buffer size.

valbok commented 1 month ago

Finally, there is a strange line in the code where outCount is calculated. This line should simply be: int outCount = frame->nb_samples; av_samples_get_buffer_size takes care of the math to calculate the buffer size.

Interesting but this was "inspired" by ffplay.c impl:

        int out_count = (int64_t)wanted_nb_samples * is->audio_tgt.freq / af->frame->sample_rate + 256;
        int out_size  = av_samples_get_buffer_size(NULL, is->audio_tgt.ch_layout.nb_channels, out_count, is->audio_tgt.fmt, 0);
valbok commented 1 week ago

Thanks @pasveer, just merged the fix to reuse SwrContext, could you please check if it works for you? Thanks again

PS Sorry for long delay.