| Message ID | 20170707210233.3965-1-wtc@google.com |
|---|---|
| State | New |
| Headers | show |
Note: I suspect we can simply delete the following line from
update_context_from_user() in libavcodec/pthread_frame.c:
dst->debug = src->debug;
That also fixes the tsan warning, but it'll take more time to
investigate whether it is necessary to update the |debug| field from
the user's AVCodecContext (src).
That line in update_context_from_user() was added in the initial
commit of libavcodec/pthread.c:
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=37b00b47cbeecd66bb34c5c7c534d016d6e8da24
Does any user actually modify avctx->debug after the avcodec_open2() call?
Thanks,
Wan-Teh Chang
Hi, On Fri, Jul 7, 2017 at 5:30 PM, Wan-Teh Chang <wtc-at-google.com@ffmpeg.org> wrote: > Note: I suspect we can simply delete the following line from > update_context_from_user() in libavcodec/pthread_frame.c: > > dst->debug = src->debug; > > That also fixes the tsan warning, but it'll take more time to > investigate whether it is necessary to update the |debug| field from > the user's AVCodecContext (src). > > That line in update_context_from_user() was added in the initial > commit of libavcodec/pthread.c: > > http://git.videolan.org/?p=ffmpeg.git;a=commit;h= > 37b00b47cbeecd66bb34c5c7c534d016d6e8da24 > > Does any user actually modify avctx->debug after the avcodec_open2() call? To sync values of debug between worker threads if the user dynamically toggles bits in this flag. Ronald
Hi, On Fri, Jul 7, 2017 at 5:31 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Jul 7, 2017 at 5:30 PM, Wan-Teh Chang < > wtc-at-google.com@ffmpeg.org> wrote: > >> Note: I suspect we can simply delete the following line from >> update_context_from_user() in libavcodec/pthread_frame.c: >> >> dst->debug = src->debug; >> >> That also fixes the tsan warning, but it'll take more time to >> investigate whether it is necessary to update the |debug| field from >> the user's AVCodecContext (src). >> >> That line in update_context_from_user() was added in the initial >> commit of libavcodec/pthread.c: >> >> http://git.videolan.org/?p=ffmpeg.git;a=commit;h=37b00b47cbe >> ecd66bb34c5c7c534d016d6e8da24 >> >> Does any user actually modify avctx->debug after the avcodec_open2() call? > > > To sync values of debug between worker threads if the user dynamically > toggles bits in this flag. > Hm, I misread your question yesterday, sorry about that. Yes, users can dynamically toggle this flag. Whether they do is a good question, but we'd typically consider it a regression if this breaks. Since it's not hard to keep it working, I'd prefer to keep it working. Ronald
Hi, On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang <wtc-at-google.com@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()? Ronald
Hi Ronald, On Sat, Jul 8, 2017 at 6:19 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang <wtc-at-google.com@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()). Wan-Teh Chang
Hi, On Sat, Jul 8, 2017 at 5:28 PM, Wan-Teh Chang <wtc-at-google.com@ffmpeg.org> wrote: > Hi Ronald, > > On Sat, Jul 8, 2017 at 6:19 AM, Ronald S. Bultje <rsbultje@gmail.com> > wrote: > > Hi, > > > > On Fri, Jul 7, 2017 at 5:02 PM, Wan-Teh Chang < > wtc-at-google.com@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
Hi Ronald, On Sat, Jul 8, 2017 at 2:33 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > > 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. My justification is the YAGNI principle. Although the current code allows the FF_DEBUG_THREADS option to be toggled dynamically, I believe that was not intended, and I believe nobody actually does that. In my (admittedly limited) code search, I only see the FF_DEBUG_THREADS option set via the -debug thread_ops command-line option. > 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. Yes, the syncing is trivial to add. When someone actually needs to toggle the FF_DEBUG_THREADS option dynamically, we can easily add the syncing at that time. This is your call. Please let me know your decision. Thanks! Wan-Teh Chang
On Sat, Jul 08, 2017 at 03:38:11PM -0700, Wan-Teh Chang wrote: > Hi Ronald, > > On Sat, Jul 8, 2017 at 2:33 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > > > > 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. > > My justification is the YAGNI principle. > > Although the current code allows the FF_DEBUG_THREADS option to be > toggled dynamically, I believe that was not intended, and I believe > nobody actually does that. In my (admittedly limited) code search, I > only see the FF_DEBUG_THREADS option set via the -debug thread_ops > command-line option. ffmpeg (the command line tool) allows changing the AVCodecContext->debug value at runtime [...]
On Sat, Jul 8, 2017 at 3:49 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Jul 08, 2017 at 03:38:11PM -0700, Wan-Teh Chang wrote: >> Hi Ronald, >> >> On Sat, Jul 8, 2017 at 2:33 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: >> > >> > 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. >> >> My justification is the YAGNI principle. >> >> Although the current code allows the FF_DEBUG_THREADS option to be >> toggled dynamically, I believe that was not intended, and I believe >> nobody actually does that. In my (admittedly limited) code search, I >> only see the FF_DEBUG_THREADS option set via the -debug thread_ops >> command-line option. > > ffmpeg (the command line tool) allows changing the AVCodecContext->debug > value at runtime Thank you. I abandoned this patch and wrote a new patch. Please see http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213501.html. Wan-Teh Chang
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 363b139f71..eb4d2d9458 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -133,6 +133,7 @@ typedef struct FrameThreadContext { * Set for the first N packets, where N is the number of threads. * While it is set, ff_thread_en/decode_frame won't return any results. */ + int debug_threads; ///< Set if the FF_DEBUG_THREADS option is set. } FrameThreadContext; #define THREAD_SAFE_CALLBACKS(avctx) \ @@ -566,7 +567,7 @@ void ff_thread_report_progress(ThreadFrame *f, int n, int field) p = f->owner[field]->internal->thread_ctx; pthread_mutex_lock(&p->progress_mutex); - if (f->owner[field]->debug&FF_DEBUG_THREADS) + if (p->parent->debug_threads) av_log(f->owner[field], AV_LOG_DEBUG, "%p finished %d field %d\n", progress, n, field); @@ -588,7 +589,7 @@ void ff_thread_await_progress(ThreadFrame *f, int n, int field) p = f->owner[field]->internal->thread_ctx; pthread_mutex_lock(&p->progress_mutex); - if (f->owner[field]->debug&FF_DEBUG_THREADS) + if (p->parent->debug_threads) av_log(f->owner[field], AV_LOG_DEBUG, "thread awaiting %d field %d from %p\n", n, field, progress); while (atomic_load_explicit(&progress[field], memory_order_relaxed) < n) @@ -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; for (i = 0; i < thread_count; i++) { AVCodecContext *copy = av_malloc(sizeof(AVCodecContext));
Add the debug_threads boolean field to FrameThreadContext. The debug_threads field records whether the FF_DEBUG_THREADS bit is set in the debug field of the avctx passed to ff_frame_thread_init(). The debug_threads field is set when avcodec_open2() is called, never changes thereafter, and applies to all the decoding threads owned by a FrameThreadContext. The current code allows different decoding threads to have different FF_DEBUG_THREADS options, but that does not seem useful. This fixes the tsan warning that 2e664b9c1e73c80aab91070c1eb7676f04bdd12d attempted to fix: WARNING: ThreadSanitizer: data race (pid=452658) Write of size 4 at 0x7b640003f4fc by main thread (mutexes: write M248499): #0 update_context_from_user [..]/libavcodec/pthread_frame.c:335:19 (5ab42bb1a6f4b068d7863dabe9b2bacc+0xe73859) [..] Previous read of size 4 at 0x7b640003f4fc by thread T130 (mutexes: write M248502, write M248500): #0 ff_thread_await_progress [..]/libavcodec/pthread_frame.c:591:26 (5ab42bb1a6f4b068d7863dabe9b2bacc+0xe749a1) Signed-off-by: Wan-Teh Chang <wtc@google.com> --- libavcodec/pthread_frame.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)