[FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
This bug was found by Dmitry Vyukov. If two threads may call
ff_thread_report_progress at the same time, progress[field] may
decrease. For example, suppose progress[field] is 10 and two threads
call ff_thread_report_progress to update progress[field] to 11 and
12, respectively. If the second thread acquires progress_mutex first
and updates progress[field] to 12, then the first thread will update
progress[field] to 11, causing progress[field] to decrease.
---
libavcodec/pthread_frame.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index b77dd1e..a43e8fe 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -482,8 +482,10 @@ void ff_thread_report_progress(ThreadFrame *f, int n, int
field)
av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field %d\n", progress,
n, field);
pthread_mutex_lock(&p->progress_mutex);
-progress[field] = n;
-pthread_cond_broadcast(&p->progress_cond);
+if (progress[field] < n) {
+progress[field] = n;
+pthread_cond_broadcast(&p->progress_cond);
+}
pthread_mutex_unlock(&p->progress_mutex);
}
--
2.7.0.rc3.207.g0ac5344
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
Hi,
On Mon, Feb 29, 2016 at 5:41 PM, Wan-Teh Chang wrote:
> This bug was found by Dmitry Vyukov. If two threads may call
> ff_thread_report_progress at the same time, progress[field] may
> decrease. For example, suppose progress[field] is 10 and two threads
> call ff_thread_report_progress to update progress[field] to 11 and
> 12, respectively. If the second thread acquires progress_mutex first
> and updates progress[field] to 12, then the first thread will update
> progress[field] to 11, causing progress[field] to decrease.
> ---
> libavcodec/pthread_frame.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index b77dd1e..a43e8fe 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -482,8 +482,10 @@ void ff_thread_report_progress(ThreadFrame *f, int n,
> int field)
> av_log(f->owner, AV_LOG_DEBUG, "%p finished %d field %d\n",
> progress, n, field);
>
> pthread_mutex_lock(&p->progress_mutex);
> -progress[field] = n;
> -pthread_cond_broadcast(&p->progress_cond);
> +if (progress[field] < n) {
> +progress[field] = n;
> +pthread_cond_broadcast(&p->progress_cond);
> +}
> pthread_mutex_unlock(&p->progress_mutex);
> }
Do you have a sample+commandline to reproduce? The thing is, in all cases
where we use this, only one thread writes to a specific progress[n]. Two
threads may write to progress[], one per field, but one will write to
progress[0] and the other to progress[1]. If this happens, I don't mind the
patch, but I'd like to know how exactly this happens (also for posterity
documentation purposes).
Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
On Mon, Feb 29, 2016 at 7:00 PM, Ronald S. Bultje wrote: > > Do you have a sample+commandline to reproduce? The thing is, in all cases > where we use this, only one thread writes to a specific progress[n]. Two > threads may write to progress[], one per field, but one will write to > progress[0] and the other to progress[1]. If this happens, I don't mind the > patch, but I'd like to know how exactly this happens (also for posterity > documentation purposes). Dmitry Vyukov found this issue by code inspection. We understand this is a bug only if two threads may call ff_thread_report_progress, on the same progress[field] value (which I didn't state in my original report), at the same time. I thought we should report the issue here to get your opinion. You can reject this patch, or accept the patch to make it easier to reason about the correctness of the code. By the way, I'm also wondering why ff_thread_report_progress is sometimes called with progress[field] >= n? I saw it happen when I ran make fate-h264, but did not investigate it. Thanks, Wan-Teh Chang ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
Hi, On Tue, Mar 1, 2016 at 9:34 AM, Wan-Teh Chang wrote: > By the way, I'm also wondering why ff_thread_report_progress is > sometimes called with progress[field] >= n? I saw it happen when I ran > make fate-h264, but did not investigate it. I don't know, which sample (test name) specifically? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
On Tue, Mar 1, 2016 at 7:44 AM, Ronald S. Bultje wrote: > Hi, > > On Tue, Mar 1, 2016 at 9:34 AM, Wan-Teh Chang > wrote: > >> By the way, I'm also wondering why ff_thread_report_progress is >> sometimes called with progress[field] >= n? I saw it happen when I ran >> make fate-h264, but did not investigate it. > > I don't know, which sample (test name) specifically? The attached ff_thread_report_progress.txt file is a patch for ffmpeg that counts the fast and slow code paths in ff_thread_report_progress and ff_thread_await_progress, and prints the counters at the end. I ran "make fate-h264 THREADS=4". Here are the results, which show that the fast code path (progress[field] >= n) was taken in ff_thread_report_progress. wtc@hostname:~/ffmpeg-dev/fast-slow-paths-instrumentation/ffmpeg/tests/data/fate$ grep slow *.err h264-conformance-aud_mw_e.err:report_progress: fast=100, slow=1000 | await_progress: fast=10999, slow=198 h264-conformance-ba1_ft_c.err:report_progress: fast=299, slow=5382 | await_progress: fast=112159, slow=1254 h264-conformance-ba1_sony_d.err:report_progress: fast=17, slow=153 | await_progress: fast=0, slow=0 h264-conformance-ba2_sony_f.err:report_progress: fast=300, slow=2700 | await_progress: fast=36259, slow=688 h264-conformance-ba3_sva_c.err:report_progress: fast=17, slow=153 | await_progress: fast=6054, slow=48 h264-conformance-bamq1_jvc_c.err:report_progress: fast=30, slow=270 | await_progress: fast=0, slow=0 h264-conformance-bamq2_jvc_c.err:report_progress: fast=30, slow=270 | await_progress: fast=4235, slow=87 h264-conformance-ba_mw_d.err:report_progress: fast=100, slow=900 | await_progress: fast=10969, slow=223 h264-conformance-banm_mw_d.err:report_progress: fast=100, slow=900 | await_progress: fast=9003, slow=243 h264-conformance-basqp1_sony_c.err:report_progress: fast=4, slow=36 | await_progress: fast=0, slow=0 h264-conformance-caba1_sony_d.err:report_progress: fast=50, slow=450 | await_progress: fast=0, slow=0 h264-conformance-caba1_sva_b.err:report_progress: fast=17, slow=153 | await_progress: fast=0, slow=0 h264-conformance-caba2_sony_e.err:report_progress: fast=300, slow=2700 | await_progress: fast=34215, slow=920 h264-conformance-caba2_sva_b.err:report_progress: fast=17, slow=153 | await_progress: fast=1610, slow=24 h264-conformance-caba3_sony_c.err:report_progress: fast=101, slow=909 | await_progress: fast=60774, slow=264 h264-conformance-caba3_sva_b.err:report_progress: fast=17, slow=153 | await_progress: fast=6637, slow=91 h264-conformance-caba3_toshiba_e.err:report_progress: fast=300, slow=2700 | await_progress: fast=28113, slow=716 h264-conformance-cabaci3_sony_b.err:report_progress: fast=101, slow=909 | await_progress: fast=60989, slow=302 h264-conformance-cabac_mot_fld0_full.err:report_progress: fast=24, slow=360 | await_progress: fast=72775, slow=83 h264-conformance-cabac_mot_frm0_full.err:report_progress: fast=12, slow=360 | await_progress: fast=79299, slow=225 h264-conformance-cabac_mot_mbaff0_full.err:report_progress: fast=12, slow=180 | await_progress: fast=77371, slow=119 h264-conformance-cabac_mot_picaff0_full.err:report_progress: fast=18, slow=346 | await_progress: fast=88910, slow=169 h264-conformance-cabast3_sony_e.err:report_progress: fast=9, slow=162 | await_progress: fast=7851, slow=54 h264-conformance-cabastbr3_sony_b.err:report_progress: fast=25, slow=450 | await_progress: fast=7967, slow=119 h264-conformance-cabref3_sand_d.err:report_progress: fast=36, slow=324 | await_progress: fast=38201, slow=43 h264-conformance-cacqp3_sony_d.err:report_progress: fast=26, slow=234 | await_progress: fast=9512, slow=126 h264-conformance-cafi1_sva_c.err:report_progress: fast=34, slow=510 | await_progress: fast=78904, slow=2 h264-conformance-cama1_sony_c.err:report_progress: fast=5, slow=75 | await_progress: fast=0, slow=0 h264-conformance-cama1_toshiba_b.err:report_progress: fast=30, slow=270 | await_progress: fast=72871, slow=191 h264-conformance-cama1_vtc_c.err:report_progress: fast=3, slow=45 | await_progress: fast=9041, slow=14 h264-conformance-cama2_vtc_b.err:report_progress: fast=3, slow=57 | await_progress: fast=13790, slow=14 h264-conformance-cama3_sand_e.err:report_progress: fast=18, slow=162 | await_progress: fast=34497, slow=77 h264-conformance-cama3_vtc_b.err:report_progress: fast=3, slow=54 | await_progress: fast=13784, slow=13 h264-conformance-camaci3_sony_c.err:report_progress: fast=7, slow=28 | await_progress: fast=1327, slow=22 h264-conformance-camanl1_toshiba_b.err:report_progress: fast=30, slow=300 | await_progress: fast=71845, slow=200 h264-conformance-camanl2_toshiba_b.err:report_progress: fast=30, slow=300 | await_progress: fast=65382, slow=187 h264-conformance-camanl3_sand_e.err:report_progress: fast=18, slow=180 | await_progress: fast=34512, slow=61 h264-conformance-camasl3_sony_b.err:report_progress: fast=7, slow=28 | await_progress: fast=3142, slow=23 h264-conformance-camp_mot_mbaff_l30.err:report_progress: fast=12, slow=180 | await_progress
Re: [FFmpeg-devel] [PATCH] Fix a bug in ff_thread_report_progress in updating progress[field].
Hi, On Tue, Mar 1, 2016 at 8:05 PM, Wan-Teh Chang wrote: > On Tue, Mar 1, 2016 at 7:44 AM, Ronald S. Bultje > wrote: > > Hi, > > > > On Tue, Mar 1, 2016 at 9:34 AM, Wan-Teh Chang < > wtc-at-google@ffmpeg.org> > > wrote: > > > >> By the way, I'm also wondering why ff_thread_report_progress is > >> sometimes called with progress[field] >= n? I saw it happen when I ran > >> make fate-h264, but did not investigate it. > > > > I don't know, which sample (test name) specifically? > > The attached ff_thread_report_progress.txt file is a patch for ffmpeg > that counts the fast and slow code paths in ff_thread_report_progress > and ff_thread_await_progress, and prints the counters at the end. > > I ran "make fate-h264 THREADS=4". Here are the results, which show > that the fast code path (progress[field] >= n) was taken in > ff_thread_report_progress. I think I mis-read the question. We don't care if progress[field] == n, that's a logical thing that can happen in many cases. We care only if progress[field] > n, which would be an actual race. It's only logical that the fast code path is taken if progress[field] == n, and this can happen for all sort of reasons, none of which are pathological or worth spending time on. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
