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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
