[FFmpeg-devel] [PATCH 4/4] avcodec/mediacodecenc: add async mode support

[Zhao Zhili]

> On Nov 27, 2024, at 14:37, Anton Khirnov <anton at khirnov.net> wrote:
> 
> Quoting Zhao Zhili (2024-11-06 13:31:34)
>> @@ -78,6 +80,17 @@ typedef struct MediaCodecEncContext {
>>     int extract_extradata;
>>     // Ref. MediaFormat KEY_OPERATING_RATE
>>     int operating_rate;
>> +    int async_mode;
>> +
>> +    AVMutex input_mutex;
>> +    AVCond input_cond;
>> +    AVFifo *input_index;
>> +
>> +    AVMutex output_mutex;
>> +    AVCond output_cond;
>> +    int encode_status;
>> +    AVFifo *output_index;
>> +    AVFifo *output_buf_info;
> 
> Those seem to always be written and read together, so why not merge them
> into one FIFO?

OK. Will add a struct to hold these two information.

> 
>> +static void on_input_available(FFAMediaCodec *codec, void *userdata,
>> +                               int32_t index)
>> +{
>> +    AVCodecContext *avctx = userdata;
>> +    MediaCodecEncContext *s = avctx->priv_data;
>> +
>> +    ff_mutex_lock(&s->input_mutex);
>> +
>> +    av_fifo_write(s->input_index, &index, 1);
> 
> The fifo is dynamic, so this can fail.

OK.

> 
>> +
>> +    ff_mutex_unlock(&s->input_mutex);
>> +    ff_cond_signal(&s->input_cond);
> 
> This looks wrong - you should signal while holding the mutex, otherwise
> the consumer may miss the signal. Same for the other two signalling
> sites.

How? The Linux Programming Interface 30.2.2:

We conclude with one final observation about the use of pthread_cond_signal()
(and pthread_cond_broadcast()). In the producer code shown earlier, we called
pthread_mutex_unlock(), and then called pthread_cond_signal(); that is, we first unlocked
the mutex associated with the shared variable, and then signaled the corresponding
condition variable. We could have reversed these two steps; SUSv3 permits them to
be done in either order.

> 
>> @@ -395,17 +569,62 @@ bailout:
>>     return ret;
>> }
>> 
>> +static int mediacodec_get_output_index(AVCodecContext *avctx, ssize_t *index,
>> +                                       FFAMediaCodecBufferInfo *out_info)
>> +{
>> +    MediaCodecEncContext *s = avctx->priv_data;
>> +    FFAMediaCodec *codec = s->codec;
>> +    int64_t timeout_us = s->eof_sent ? OUTPUT_DEQUEUE_TIMEOUT_US : 0;
>> +    int n;
>> +    int ret;
>> +
>> +    if (!s->async_mode) {
>> +        *index = ff_AMediaCodec_dequeueOutputBuffer(codec, out_info, timeout_us);
>> +        return 0;
>> +    }
>> +
>> +    ff_mutex_lock(&s->output_mutex);
>> +
>> +    n = -1;
>> +    while (n < 0 && !s->encode_status) {
>> +        if (av_fifo_can_read(s->output_index)) {
>> +            av_fifo_read(s->output_index, &n, 1);
>> +            av_fifo_read(s->output_buf_info, out_info, 1);
> 
> It's simpler and API-compliant to just call av_fifo_read() and check
> whether it failed.

OK.

> 
> -- 
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".