On 2016-11-24 19:19:59 +0100, Anton Khirnov wrote:
> Only allow the decoding thread to run while the user is inside a lavc
> decode call (avcodec_send_packet/receive_frame).
> Hardware decoding APIs are often not thread-safe, so having the user
> access decoded hardware surfaces while the decoder is running in another
> thread can cause failures (this is mainly known to happen with DXVA2).
This looks a little extreme. How painful would it be to provide an
option to share the mutex with the calling process?
> ---
> libavcodec/pthread_frame.c | 58
> +++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 2736a81..430854f 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -99,6 +99,8 @@ typedef struct PerThreadContext {
> int requested_flags; ///< flags passed to get_buffer() for
> requested_frame
>
> int die; ///< Set when the thread should exit.
> +
> + int hwaccel_serializing;
> } PerThreadContext;
>
> /**
> @@ -110,6 +112,14 @@ typedef struct FrameThreadContext {
>
> pthread_mutex_t buffer_mutex; ///< Mutex used to protect
> get/release_buffer().
>
> + /**
> + * This lock is used for making sure hwaccel decoding does not run
> + * concurrently with the calling thread. It is held by the calling thread
> + * most of the time and released only while inside
> ff_thread_decode_frame(),
> + * at which time the worker threads are allowed to progress past
> finish_setup().
> + */
> + pthread_mutex_t hwaccel_mutex;
> +
> int next_decoding; ///< The next context to submit a packet
> to.
> int next_finished; ///< The next context to return output
> from.
>
> @@ -163,6 +173,11 @@ static attribute_align_arg void
> *frame_worker_thread(void *arg)
> if (atomic_load(&p->state) == STATE_SETTING_UP)
> ff_thread_finish_setup(avctx);
>
> + if (p->hwaccel_serializing) {
> + pthread_mutex_unlock(&p->parent->hwaccel_mutex);
> + p->hwaccel_serializing = 0;
although protected by other mutexes it would make sense to protect
hwaccel_serializing explicitly by hwaccel_mutex
> + }
> +
> atomic_store(&p->state, STATE_INPUT_READY);
>
> pthread_mutex_lock(&p->progress_mutex);
> @@ -386,7 +401,11 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> FrameThreadContext *fctx = avctx->internal->thread_ctx;
> int finished = fctx->next_finished;
> PerThreadContext *p;
> - int err;
> + int err, ret;
> +
> + /* release the hwaccel lock, permitting blocked hwaccel threads to go
> + * forward while we are inside this function */
> + pthread_mutex_unlock(&fctx->hwaccel_mutex);
>
> /*
> * Submit a packet to the next decoding thread.
> @@ -394,9 +413,11 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>
> p = &fctx->threads[fctx->next_decoding];
> err = update_context_from_user(p->avctx, avctx);
> - if (err) return err;
> + if (err)
> + goto finish;
> err = submit_packet(p, avpkt);
> - if (err) return err;
> + if (err)
> + goto finish;
>
> /*
> * If we're still receiving the initial packets, don't return a frame.
> @@ -406,8 +427,10 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> if (fctx->next_decoding >= (avctx->thread_count-1)) fctx->delaying =
> 0;
>
> *got_picture_ptr=0;
> - if (avpkt->size)
> - return avpkt->size;
> + if (avpkt->size) {
> + ret = avpkt->size;
> + goto finish;
> + }
> }
>
> /*
> @@ -448,8 +471,14 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>
> fctx->next_finished = finished;
>
> + ret = (p->result >= 0) ? avpkt->size : p->result;
> +finish:
> + pthread_mutex_lock(&fctx->hwaccel_mutex);
> + if (err < 0)
> + return err;
> +
> /* return the size of the consumed packet if no error occurred */
> - return (p->result >= 0) ? avpkt->size : p->result;
> + return ret;
> }
>
> void ff_thread_report_progress(ThreadFrame *f, int n, int field)
> @@ -505,6 +534,11 @@ void ff_thread_finish_setup(AVCodecContext *avctx) {
>
> pthread_cond_broadcast(&p->progress_cond);
> pthread_mutex_unlock(&p->progress_mutex);
> +
> + if (avctx->hwaccel) {
> + pthread_mutex_lock(&p->parent->hwaccel_mutex);
> + p->hwaccel_serializing = 1;
> + }
I would have expected the lock before the codecs decode call. Since we
don't call finish_setup until after the frame is decode this can't
guarantee serialized access to the hw decoding library.
Janne
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
