Quoting Janne Grunau (2016-11-25 00:17:17)
> 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?
Quite a lot I think. And I don't think this patch should cause any
significant performance impact. Didn't make any exact measurements yet,
but I can if you think it's a problem.
>
> > ---
> > 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
This variable is never accessed from any threads other than the owner of
p, so there should be no need to protect it.
>
> > + }
> > +
> > 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.
I assume that all hwaccelled decoders call finish_setup() explicitly.
The lock cannot happen any earlier, because we don't know we are using
hwaccel until finish_setup().
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
