[FFmpeg-devel] [PATCH 12/13] avcodec/omx: Check initializing mutexes/conditions

[Andreas Rheinhardt]

Steve Lhomme:
> On 2021-09-02 17:41, Andreas Rheinhardt wrote:
>> The earlier code did not properly check these initializations:
>> It only recorded whether the part of init where these initializations
>> are has been reached, but it did not check whether the initializations
>> were successful, although destroying them would be undefined behaviour
>> if they had not been initialized successfully.
>> Furthermore cleanup() always locked a mutex regardless of whether there
>> was any attempt to initialize these mutexes at all.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>> This is mostly untested: I only tested whether it compiles.
>>
>>   libavcodec/omx.c | 34 +++++++++++++++++-----------------
>>   1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
>> index 9597c60057..7086ddd3a4 100644
>> --- a/libavcodec/omx.c
>> +++ b/libavcodec/omx.c
>> @@ -43,6 +43,7 @@
>>   #include "avcodec.h"
>>   #include "h264.h"
>>   #include "internal.h"
>> +#include "pthread_internal.h"
>>     #ifdef OMX_SKIP64BIT
>>   static OMX_TICKS to_omx_ticks(int64_t value)
>> @@ -218,7 +219,7 @@ typedef struct OMXCodecContext {
>>       OMX_STATETYPE state;
>>       OMX_ERRORTYPE error;
>>   -    int mutex_cond_inited;
>> +    unsigned mutex_cond_inited_cnt;
>>         int eos_sent, got_eos;
>>   @@ -229,6 +230,12 @@ typedef struct OMXCodecContext {
>>       int profile;
>>   } OMXCodecContext;
>>   +#define NB_MUTEX_CONDS 6
>> +#define OFF(field) offsetof(OMXCodecContext, field)
>> +DEFINE_OFFSET_ARRAY(OMXCodecContext, omx_codec_context,
>> mutex_cond_inited_cnt,
>> +                    (OFF(input_mutex), OFF(output_mutex),
>> OFF(state_mutex)),
>> +                    (OFF(input_cond),  OFF(output_cond), 
>> OFF(state_cond)));
>> +
>>   static void append_buffer(pthread_mutex_t *mutex, pthread_cond_t *cond,
>>                             int* array_size, OMX_BUFFERHEADERTYPE
>> **array,
>>                             OMX_BUFFERHEADERTYPE *buffer)
>> @@ -591,6 +598,9 @@ static av_cold void cleanup(OMXCodecContext *s)
>>   {
>>       int i, executing;
>>   +    /* If the mutexes/condition variables have not been properly
>> initialized,
>> +     * nothing has been initialized and locking the mutex might be
>> unsafe. */
>> +    if (s->mutex_cond_inited_cnt == NB_MUTEX_CONDS) {
>>       pthread_mutex_lock(&s->state_mutex);
>>       executing = s->state == OMX_StateExecuting;
>>       pthread_mutex_unlock(&s->state_mutex);
>> @@ -620,20 +630,13 @@ static av_cold void cleanup(OMXCodecContext *s)
>>         omx_deinit(s->omx_context);
>>       s->omx_context = NULL;
>> -    if (s->mutex_cond_inited) {
>> -        pthread_cond_destroy(&s->state_cond);
>> -        pthread_mutex_destroy(&s->state_mutex);
>> -        pthread_cond_destroy(&s->input_cond);
>> -        pthread_mutex_destroy(&s->input_mutex);
>> -        pthread_cond_destroy(&s->output_cond);
>> -        pthread_mutex_destroy(&s->output_mutex);
>> -        s->mutex_cond_inited = 0;
>> -    }
>>       av_freep(&s->in_buffer_headers);
>>       av_freep(&s->out_buffer_headers);
>>       av_freep(&s->free_in_buffers);
>>       av_freep(&s->done_out_buffers);
>>       av_freep(&s->output_buf);
>> +    }
>> +    ff_pthread_free(s, omx_codec_context_offsets);
>>   }
>>     static av_cold int omx_encode_init(AVCodecContext *avctx)
>> @@ -644,17 +647,14 @@ static av_cold int
>> omx_encode_init(AVCodecContext *avctx)
>>       OMX_BUFFERHEADERTYPE *buffer;
>>       OMX_ERRORTYPE err;
>>   +    /* cleanup relies on the mutexes/conditions being initialized
>> first. */
>> +    ret = ff_pthread_init(s, omx_codec_context_offsets);
>> +    if (ret < 0)
>> +        return ret;
>>       s->omx_context = omx_init(avctx, s->libname, s->libprefix);
>>       if (!s->omx_context)
> 
> Shouldn't you call ff_pthread_free() here ?
> 
These encoders have the FF_CODEC_CAP_INIT_CLEANUP set which means that
omx_encode_end() will be called automatically if omx_encode_init()
fails. (This is why the current code always locked the state_mutex
regardless of whether its initialization has ever been reached.)

- Andreas