[FFmpeg-devel] [PATCH] pthread_frame: save the FF_DEBUG_THREADS option in FrameThreadContext.

[Ronald S. Bultje]

Hi,

On Sat, Jul 8, 2017 at 5:28 PM, Wan-Teh Chang <wtc-at-google.com at ffmpeg.org>
wrote:

> Hi Ronald,
>
> On Sat, Jul 8, 2017 at 6:19 AM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang <
> wtc-at-google.com at ffmpeg.org>
> > wrote:
> >
> >> @@ -763,6 +764,7 @@ int ff_frame_thread_init(AVCodecContext *avctx)
> >>
> >>      fctx->async_lock = 1;
> >>      fctx->delaying = 1;
> >> +    fctx->debug_threads = (avctx->debug & FF_DEBUG_THREADS) != 0;
> >
> > Shouldn't this be done in update_context_from_user()?
>
> This patch differs from the approach you outlined at the end of
> http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213259.html
> as follows:
>
> * The debug_threads field is added to FrameThreadContext and applies to
>   all the decoding threads owned by the FrameThreadContext.
> * The debug_threads field is set when avcodec_open2() is called, and
>   never changes thereafter.
>
> In this design, we just need to set fctx->debug_threads in
> ff_frame_thread_init() (which is called by avcodec_open2()).


I can see the design from the patch.

What's missing is a justification for the downside of the design, which is
that updates to this variable by the user are no longer propagated to the
worker threads. The syncing is extremely trivial (simply by moving the
assignment from init to update_from_user) and has no threading implications
(since it's a boolean, so you can make it atomic) so this really shouldn't
be a big deal to amend.

Ronald