From patchwork Thu Feb 11 15:57:59 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 25574 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id E37FE44A067 for ; Thu, 11 Feb 2021 18:06:27 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C2BDB68AB17; Thu, 11 Feb 2021 18:06:27 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0AB4C68AB0E for ; Thu, 11 Feb 2021 18:06:22 +0200 (EET) Received: by mail-wm1-f42.google.com with SMTP id m1so6268977wml.2 for ; Thu, 11 Feb 2021 08:06:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=lJfnsaWFtWkOdW5js6FXaudumQgUuPjtfw7hLX4q1t8=; b=M336K/khjSGjGwsPGr892DJvYFA/mpPYSTkAh0a7lfeBi3akUSiaV8Aku6B5CIOkQx oJg89l2CXYxUrGiG70pSEGe+LbYnUu+btFE/NEI/M/0U5KYULUai+UpgXMP1KA5q03Bz GAm7u2WXk5IEPA8VDeD0OjLJK7IVFT7cGRSUGRfstE3xak2i6RffUdLGEBFIYJOF4zUT rWbtFfVbD0pOzHyTv+EQvlqoApWP8BQ1J2ThpSLu6BkgkgCj6l0huRP1FsR0gOhYjVaH jnwG3EMu4ATKAX2Eiw2rn6qlrZfEFkKZrD+LgPFUm8C/elnNdrMMWq5gxbliB82sAUO0 Kamw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=lJfnsaWFtWkOdW5js6FXaudumQgUuPjtfw7hLX4q1t8=; b=s7B9nNlLp/fjd6J/lQ4l03tpSLnZvhOowGpeU8Oj8C8ro2YzqKesDoa7P7qpeKYVSM 7tBuLADwIYh02KE48tOm1bzq87Fg29x7HKvOBRwUCKxFSiOh8cF4EkAgX4MaxrZpsTuK tFwdpb0GRxptoopywzIQr5Rh+yxAaValAsIT08P7qPnNs6LM80C7ipAEPKQR+PVkMhnT avhwhcTqOjBjzr8w5oPc5zs6e5mKBY9VSaANkOojRfv1KLcNRvOasUxIhe3FVczNcBog fEyPJXm5D5RNOSnwOBdXsZKfviWTqAvYVghVgHSv8P4XaZjzSRpLVHDeO9P9OkgBT3I1 B8zA== X-Gm-Message-State: AOAM530GBdhfkCcGYMVKCq+xKKNeOXYCFYEcHd1PX91texqMtSkI5Lez eIg1bXabg2JR/8i9Qw/JE6U/BtjmKKM= X-Google-Smtp-Source: ABdhPJw3xrAerR1T9VX6doOFKRXXdXWTjoVfHAo2BGPnSqBRUFmWcLtPOjhhHww9xZXOkDKmF2wUzA== X-Received: by 2002:a7b:c308:: with SMTP id k8mr5821247wmj.54.1613059117924; Thu, 11 Feb 2021 07:58:37 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc1aa4b.dynamic.kabel-deutschland.de. [188.193.170.75]) by smtp.gmail.com with ESMTPSA id i10sm6326348wrp.0.2021.02.11.07.58.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Feb 2021 07:58:37 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 11 Feb 2021 16:57:59 +0100 Message-Id: <20210211155759.309391-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] avcodec/pthread_frame: Fix checks and cleanup during init X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Up until now, ff_frame_thread_init had several bugs: 1. It did not check whether the condition and mutexes could be successfully created. 2. In case an error happened when setting up the child threads, ff_frame_thread_free is called to clean up all threads set up so far, including the current one. But a half-allocated context needs special handling which ff_frame_thread_frame_free doesn't provide. Notably, if allocating the AVCodecInternal, the codec's private data or setting the options fails, the codec's close function will be called (if there is one); it will also be called if the codec's init function fails, regardless of whether the FF_CODEC_CAP_INIT_CLEANUP is set. This is not supported by all codecs; in ticket #9099 (which this commit fixes) it led to a crash. This commit fixes both of these issues. Given that it is not documented to be save to destroy mutexes/conditions that have only been zeroed, but not initialized (or whose initialization has failed), one either needs to duplicate the code to destroy them in ff_frame_thread_init or record which mutexes/condition variables need to be destroyed and check for this in ff_frame_thread_free. This patch uses the former way for the mutexes/condition variables, but lets ff_frame_thread_free take over for all threads whose AVCodecContext could be allocated. Signed-off-by: Andreas Rheinhardt --- After several unsuccessful attempts to make pthread_cond/mutex_init fail, I looked at the source [1] and it seems that the glibc versions of these functions can't fail at all (unless one sets nondefault attributes). Therefore this part of the code is untested, unfortunately. (Removing all pthread_mutex/cond_destroy calls does not result in any complaints from Valgrind/ASAN either; seems the glibc implementation doesn't need allocations.) [1]: https://github.com/bminor/glibc/blob/master/nptl/pthread_cond_init.c https://github.com/bminor/glibc/blob/master/nptl/pthread_mutex_init.c libavcodec/pthread_frame.c | 175 ++++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 69 deletions(-) diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 4429a4d59c..a10d8c1266 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -64,6 +64,12 @@ enum { STATE_SETUP_FINISHED, }; +enum { + UNINITIALIZED, ///< Thread has not been created, AVCodec->close mustn't be called + NEEDS_CLOSE, ///< AVCodec->close needs to be called + INITIALIZED, ///< Thread has been properly set up +}; + /** * Context used by codec threads and stored in their AVCodecInternal thread_ctx. */ @@ -698,27 +704,40 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count) for (i = 0; i < thread_count; i++) { PerThreadContext *p = &fctx->threads[i]; + AVCodecContext *ctx = p->avctx; - pthread_mutex_lock(&p->mutex); - p->die = 1; - pthread_cond_signal(&p->input_cond); - pthread_mutex_unlock(&p->mutex); - - if (p->thread_init) - pthread_join(p->thread, NULL); - p->thread_init=0; + if (ctx->internal) { + if (p->thread_init == INITIALIZED) { + pthread_mutex_lock(&p->mutex); + p->die = 1; + pthread_cond_signal(&p->input_cond); + pthread_mutex_unlock(&p->mutex); - if (codec->close && p->avctx) - codec->close(p->avctx); + pthread_join(p->thread, NULL); + } + if (codec->close && p->thread_init != UNINITIALIZED) + codec->close(ctx); #if FF_API_THREAD_SAFE_CALLBACKS - release_delayed_buffers(p); + release_delayed_buffers(p); + for (int j = 0; j < p->released_buffers_allocated; j++) + av_frame_free(&p->released_buffers[j]); + av_freep(&p->released_buffers); #endif - av_frame_free(&p->frame); - } + if (ctx->priv_data) { + if (codec->priv_class) + av_opt_free(ctx->priv_data); + av_freep(&ctx->priv_data); + } - for (i = 0; i < thread_count; i++) { - PerThreadContext *p = &fctx->threads[i]; + av_freep(&ctx->slice_offset); + + av_buffer_unref(&ctx->internal->pool); + av_freep(&ctx->internal); + av_buffer_unref(&ctx->hw_frames_ctx); + } + + av_frame_free(&p->frame); pthread_mutex_destroy(&p->mutex); pthread_mutex_destroy(&p->progress_mutex); @@ -727,26 +746,6 @@ void ff_frame_thread_free(AVCodecContext *avctx, int thread_count) pthread_cond_destroy(&p->output_cond); av_packet_unref(&p->avpkt); -#if FF_API_THREAD_SAFE_CALLBACKS - for (int j = 0; j < p->released_buffers_allocated; j++) - av_frame_free(&p->released_buffers[j]); - av_freep(&p->released_buffers); -#endif - - if (p->avctx) { - if (codec->priv_class) - av_opt_free(p->avctx->priv_data); - av_freep(&p->avctx->priv_data); - - av_freep(&p->avctx->slice_offset); - } - - if (p->avctx) { - av_buffer_unref(&p->avctx->internal->pool); - av_freep(&p->avctx->internal); - av_buffer_unref(&p->avctx->hw_frames_ctx); - } - av_freep(&p->avctx); } @@ -791,14 +790,19 @@ int ff_frame_thread_init(AVCodecContext *avctx) fctx->threads = av_mallocz_array(thread_count, sizeof(PerThreadContext)); if (!fctx->threads) { - av_freep(&avctx->internal->thread_ctx); - return AVERROR(ENOMEM); + err = AVERROR(ENOMEM); + goto threads_alloc_fail; } - pthread_mutex_init(&fctx->buffer_mutex, NULL); - pthread_mutex_init(&fctx->hwaccel_mutex, NULL); - pthread_mutex_init(&fctx->async_mutex, NULL); - pthread_cond_init(&fctx->async_cond, NULL); +#define PTHREAD_INIT(type, ctx, field) \ + if (err = pthread_ ## type ## _init(&ctx->field, NULL)) { \ + err = AVERROR(err); \ + goto field ## _fail; \ + } + PTHREAD_INIT(mutex, fctx, buffer_mutex) + PTHREAD_INIT(mutex, fctx, hwaccel_mutex) + PTHREAD_INIT(mutex, fctx, async_mutex) + PTHREAD_INIT(cond, fctx, async_cond) fctx->async_lock = 1; fctx->delaying = 1; @@ -806,40 +810,37 @@ int ff_frame_thread_init(AVCodecContext *avctx) if (codec->type == AVMEDIA_TYPE_VIDEO) avctx->delay = src->thread_count - 1; - for (i = 0; i < thread_count; i++) { - AVCodecContext *copy = av_malloc(sizeof(AVCodecContext)); + for (i = 0; i < thread_count; ) { PerThreadContext *p = &fctx->threads[i]; + AVCodecContext *copy; + int first = !i; - pthread_mutex_init(&p->mutex, NULL); - pthread_mutex_init(&p->progress_mutex, NULL); - pthread_cond_init(&p->input_cond, NULL); - pthread_cond_init(&p->progress_cond, NULL); - pthread_cond_init(&p->output_cond, NULL); - - p->frame = av_frame_alloc(); - if (!p->frame) { - av_freep(©); - err = AVERROR(ENOMEM); - goto error; - } + PTHREAD_INIT(mutex, p, mutex) + PTHREAD_INIT(mutex, p, progress_mutex) + PTHREAD_INIT(cond, p, input_cond) + PTHREAD_INIT(cond, p, progress_cond) + PTHREAD_INIT(cond, p, output_cond) - p->parent = fctx; - p->avctx = copy; + atomic_init(&p->state, STATE_INPUT_READY); + copy = av_memdup(src, sizeof(*src)); if (!copy) { err = AVERROR(ENOMEM); - goto error; + goto copy_fail; } + /* From now on, this PerThreadContext will be cleaned up by + * ff_frame_thread_free in case of errors. */ + i++; - *copy = *src; + p->parent = fctx; + p->avctx = copy; - copy->internal = av_malloc(sizeof(AVCodecInternal)); + copy->internal = av_memdup(src->internal, sizeof(*src->internal)); if (!copy->internal) { copy->priv_data = NULL; err = AVERROR(ENOMEM); goto error; } - *copy->internal = *src->internal; copy->internal->thread_ctx = p; copy->internal->last_pkt_props = &p->avpkt; @@ -860,30 +861,66 @@ int ff_frame_thread_init(AVCodecContext *avctx) } } - if (i) + p->frame = av_frame_alloc(); + if (!p->frame) { + err = AVERROR(ENOMEM); + goto error; + } + + if (!first) copy->internal->is_copy = 1; - if (codec->init) + if (codec->init) { err = codec->init(copy); + if (err) { + if (codec->caps_internal & FF_CODEC_CAP_INIT_CLEANUP) + p->thread_init = NEEDS_CLOSE; + goto error; + } + } + p->thread_init = NEEDS_CLOSE; - if (err) goto error; - - if (!i) + if (first) update_context_from_thread(avctx, copy, 1); atomic_init(&p->debug_threads, (copy->debug & FF_DEBUG_THREADS) != 0); err = AVERROR(pthread_create(&p->thread, NULL, frame_worker_thread, p)); - p->thread_init= !err; - if(!p->thread_init) + if (err) goto error; + p->thread_init = INITIALIZED; + continue; + + copy_fail: + pthread_cond_destroy(&p->output_cond); + output_cond_fail: + pthread_cond_destroy(&p->progress_cond); + progress_cond_fail: + pthread_cond_destroy(&p->input_cond); + input_cond_fail: + pthread_mutex_destroy(&p->progress_mutex); + progress_mutex_fail: + pthread_mutex_destroy(&p->mutex); + mutex_fail: + goto error; } return 0; error: - ff_frame_thread_free(avctx, i+1); + ff_frame_thread_free(avctx, i); + return err; +async_cond_fail: + pthread_mutex_destroy(&fctx->async_mutex); +async_mutex_fail: + pthread_mutex_destroy(&fctx->hwaccel_mutex); +hwaccel_mutex_fail: + pthread_mutex_destroy(&fctx->buffer_mutex); +buffer_mutex_fail: + av_freep(&fctx->threads); +threads_alloc_fail: + av_freep(&avctx->internal->thread_ctx); return err; }