[FFmpeg-devel] [PATCH 1/3] avcodec: add ff_lock/unlock_avcodec functions.

[Michael Niedermayer]

On Tue, Nov 27, 2012 at 09:59:30PM +0100, Reimar Döffinger wrote:
> Will be used in future patches, together with the
> variable that allows checking whether the lock is held.
> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> ---
>  libavcodec/internal.h |    4 +++
>  libavcodec/utils.c    |   96 +++++++++++++++++++++----------------------------
>  2 files changed, 44 insertions(+), 56 deletions(-)
> 
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 17ca8bb..f25215c 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -126,6 +126,10 @@ void ff_init_buffer_info(AVCodecContext *s, AVFrame *frame);
>   */
>  void ff_packet_free_side_data(AVPacket *pkt);
>  
> +extern volatile int ff_avcodec_locked;
> +int ff_lock_avcodec(AVCodecContext *log_ctx);
> +int ff_unlock_avcodec(void);
> +
>  int avpriv_lock_avformat(void);
>  int avpriv_unlock_avformat(void);
>  
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 32bda51..f7405de 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -47,6 +47,7 @@
>  #include <limits.h>
>  #include <float.h>
>  
> +volatile int ff_avcodec_locked;
>  static int volatile entangled_thread_counter = 0;
>  static int (*ff_lockmgr_cb)(void **mutex, enum AVLockOp op);
>  static void *codec_mutex;
> @@ -810,20 +811,11 @@ int attribute_align_arg ff_codec_open2_recursive(AVCodecContext *avctx, const AV
>  {
>      int ret = 0;
>  
> -    entangled_thread_counter--;
> -    /* Release any user-supplied mutex. */
> -    if (ff_lockmgr_cb) {
> -        (*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE);
> -    }
> +    ff_unlock_avcodec();
>  
>      ret = avcodec_open2(avctx, codec, options);
>  
> -    /* If there is a user-supplied mutex locking routine, call it. */
> -    if (ff_lockmgr_cb) {
> -        if ((*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN))
> -            return -1;
> -    }
> -    entangled_thread_counter++;
> +    ff_lock_avcodec(avctx);
>      return ret;
>  }

no more return code checking


>  
> @@ -853,18 +845,9 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (options)
>          av_dict_copy(&tmp, *options, 0);
>  
> -    /* If there is a user-supplied mutex locking routine, call it. */
> -    if (ff_lockmgr_cb) {
> -        if ((ret = (*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN)) < 0)
> -            return ret;
> -    }
> -
> -    entangled_thread_counter++;
> -    if (entangled_thread_counter != 1) {
> -        av_log(avctx, AV_LOG_ERROR, "Insufficient thread locking around avcodec_open/close()\n");
> -        ret = AVERROR(EINVAL);
> +    ret = ff_lock_avcodec(avctx);
> +    if (ret < 0)
>          goto end;
> -    }

crashes due to attempted unlocking later of a lock that wasnt obtained


>  
>      avctx->internal = av_mallocz(sizeof(AVCodecInternal));
>      if (!avctx->internal) {
> @@ -1103,12 +1086,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>          }
>      }
>  end:
> -    entangled_thread_counter--;
> -
> -    /* Release any user-supplied mutex. */
> -    if (ff_lockmgr_cb) {
> -        (*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE);
> -    }
> +    ff_unlock_avcodec();
>      if (options) {
>          av_dict_free(options);
>          *options = tmp;
> @@ -1921,37 +1899,19 @@ av_cold int ff_codec_close_recursive(AVCodecContext *avctx)
>  {
>      int ret = 0;
>  
> -    entangled_thread_counter--;
> -    /* Release any user-supplied mutex. */
> -    if (ff_lockmgr_cb) {
> -        (*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE);
> -    }
> +    ff_unlock_avcodec();
>  
>      ret = avcodec_close(avctx);
>  
> -    /* If there is a user-supplied mutex locking routine, call it. */
> -    if (ff_lockmgr_cb) {
> -        if ((*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN))
> -            return -1;
> -    }
> -    entangled_thread_counter++;
> +    ff_lock_avcodec(NULL);
>      return ret;
>  }
>  
>  av_cold int avcodec_close(AVCodecContext *avctx)
>  {
> -    /* If there is a user-supplied mutex locking routine, call it. */
> -    if (ff_lockmgr_cb) {
> -        if ((*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN))
> -            return -1;
> -    }
> -
> -    entangled_thread_counter++;
> -    if (entangled_thread_counter != 1) {
> -        av_log(avctx, AV_LOG_ERROR, "insufficient thread locking around avcodec_open/close()\n");
> -        entangled_thread_counter--;
> -        return -1;
> -    }
> +    int ret = ff_lock_avcodec(avctx);
> +    if (ret < 0)
> +        return ret;
>  
>      if (avcodec_is_open(avctx)) {
>          if (HAVE_THREADS && avctx->internal->frame_thread_encoder && avctx->thread_count > 1) {
> @@ -1979,12 +1939,8 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>          av_freep(&avctx->extradata);
>      avctx->codec = NULL;
>      avctx->active_thread_type = 0;
> -    entangled_thread_counter--;
>  
> -    /* Release any user-supplied mutex. */
> -    if (ff_lockmgr_cb) {
> -        (*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE);
> -    }
> +    ff_unlock_avcodec();
>      return 0;
>  }
>  
> @@ -2640,6 +2596,34 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>      return 0;
>  }
>  
> +int ff_lock_avcodec(AVCodecContext *log_ctx)
> +{
> +    if (ff_lockmgr_cb) {
> +        if ((*ff_lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN))
> +            return -1;
> +    }
> +    entangled_thread_counter++;
> +    if (entangled_thread_counter != 1) {
> +        av_log(log_ctx, AV_LOG_ERROR, "Insufficient thread locking around avcodec_open/close()\n");
> +        return AVERROR(EINVAL);
> +    }

this fails to update entangled_thread_counter on the error path


> +    av_assert0(!ff_avcodec_locked);
> +    ff_avcodec_locked = 1;
> +    return 0;
> +}
> +
> +int ff_unlock_avcodec(void)
> +{
> +    av_assert0(ff_avcodec_locked);

these asserts fail alot

iam trying to fix these things ATM

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121206/541382c0/attachment.asc>