[FFmpeg-devel] [PATCH 1/6] avcodec/avcodec: Move initializing frame-thrd encoder to encode_preinit

[FFmpeg-devel] [PATCH 1/6] avcodec/avcodec: Move initializing frame-thrd encoder to encode_preinit

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/avcodec.c | 6 ------
 libavcodec/encode.c  | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index f82d9e9f74..0451f57f82 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -283,12 +283,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if (ret < 0)
         goto free_and_end;
 
-    if (CONFIG_FRAME_THREAD_ENCODER && av_codec_is_encoder(avctx->codec)) {
-        ret = ff_frame_thread_encoder_init(avctx);
-        if (ret < 0)
-            goto free_and_end;
-    }
-
     if (HAVE_THREADS
         && !(avci->frame_thread_encoder && (avctx->active_thread_type&FF_THREAD_FRAME))) {
         /* Frame-threaded decoders call FFCodec.init for their child contexts. */
diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 7919b165da..bd66f138a3 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -668,6 +668,12 @@ int ff_encode_preinit(AVCodecContext *avctx)
             return AVERROR(ENOMEM);
     }
 
+    if (CONFIG_FRAME_THREAD_ENCODER) {
+        ret = ff_frame_thread_encoder_init(avctx);
+        if (ret < 0)
+            return ret;
+    }
+
     return 0;
 }
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/6] avcodec/avcodec: Remove redundant check

[Andreas Rheinhardt]

At this point active_thread_type is set iff active_thread_type
is set to FF_THREAD_FRAME iff AVCodecInternal.frame_thread_encoder
is set.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/avcodec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 0451f57f82..29643199be 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -283,8 +283,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if (ret < 0)
         goto free_and_end;
 
-    if (HAVE_THREADS
-        && !(avci->frame_thread_encoder && (avctx->active_thread_type&FF_THREAD_FRAME))) {
+    if (HAVE_THREADS && !avci->frame_thread_encoder) {
         /* Frame-threaded decoders call FFCodec.init for their child contexts. */
         lock_avcodec(codec2);
         ret = ff_thread_init(avctx);
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/6] avcodec/mpegpicture: Always reset motion val buffer

[Andreas Rheinhardt]

Codecs call ff_find_unused_picture() to get the index of
an unused picture; said picture may have buffers left
from using it previously (these buffers are intentionally
not unreferenced so that it might be possible to reuse them;
this is mpegvideo's version of a bufferpool). They should
not make any assumptions about which picture they get.
Yet somehow this is not true when decoding OBMC: Returning
random empty pictures (instead of the first one) leads
to nondeterministic results; similarly, explicitly
rezeroing the buffer before handing it over to the codec
changes the outcome of the h263-obmc tests, but it makes it
independent of the returned pictures. Therefore this commit
does so.

(No, this commit is not intended to be applied. I just hope
to arouse the interest of people familiar with H.263
to look at this issue.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I still don't intend to apply this. If no one looks into this,
I will just add the necessary changes to the h263-obmc ref-files
in the next commit and leave this one out.

 libavcodec/mpegpicture.c               | 4 ++++
 tests/ref/vsynth/vsynth1-h263-obmc     | 4 ++--
 tests/ref/vsynth/vsynth2-h263-obmc     | 4 ++--
 tests/ref/vsynth/vsynth_lena-h263-obmc | 4 ++--
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
index c57f149752..2192f74cea 100644
--- a/libavcodec/mpegpicture.c
+++ b/libavcodec/mpegpicture.c
@@ -280,6 +280,10 @@ int ff_alloc_picture(AVCodecContext *avctx, Picture *pic, MotionEstContext *me,
         for (i = 0; i < 2; i++) {
             pic->motion_val[i] = (int16_t (*)[2])pic->motion_val_buf[i]->data + 4;
             pic->ref_index[i]  = pic->ref_index_buf[i]->data;
+            /* FIXME: The output of H.263 with OBMC depends upon
+             * the earlier content of the buffer; therefore we
+             * reset it here. */
+            memset(pic->motion_val_buf[i]->data, 0, pic->motion_val_buf[i]->size);
         }
     }
 
diff --git a/tests/ref/vsynth/vsynth1-h263-obmc b/tests/ref/vsynth/vsynth1-h263-obmc
index b7a267a8cb..aed283ed53 100644
--- a/tests/ref/vsynth/vsynth1-h263-obmc
+++ b/tests/ref/vsynth/vsynth1-h263-obmc
@@ -1,4 +1,4 @@
 7dec64380f375e5118b66f3baaaa1e24 *tests/data/fate/vsynth1-h263-obmc.avi
 657320 tests/data/fate/vsynth1-h263-obmc.avi
-844f7ee27fa122e199fe20987b41a15c *tests/data/fate/vsynth1-h263-obmc.out.rawvideo
-stddev:    8.16 PSNR: 29.89 MAXDIFF:  113 bytes:  7603200/  7603200
+2a69f6b37378aa34418dfd04ec98c1c8 *tests/data/fate/vsynth1-h263-obmc.out.rawvideo
+stddev:    8.38 PSNR: 29.66 MAXDIFF:  116 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth2-h263-obmc b/tests/ref/vsynth/vsynth2-h263-obmc
index 2cef7f551b..c0dcc3239e 100644
--- a/tests/ref/vsynth/vsynth2-h263-obmc
+++ b/tests/ref/vsynth/vsynth2-h263-obmc
@@ -1,4 +1,4 @@
 2d8a58b295e03f94e6a41468b2d3909e *tests/data/fate/vsynth2-h263-obmc.avi
 208522 tests/data/fate/vsynth2-h263-obmc.avi
-4a939ef99fc759293f2e609bfcacd2a4 *tests/data/fate/vsynth2-h263-obmc.out.rawvideo
-stddev:    6.10 PSNR: 32.41 MAXDIFF:   90 bytes:  7603200/  7603200
+3500b4227c1e6309ca5213414599266f *tests/data/fate/vsynth2-h263-obmc.out.rawvideo
+stddev:    6.19 PSNR: 32.29 MAXDIFF:  111 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth_lena-h263-obmc b/tests/ref/vsynth/vsynth_lena-h263-obmc
index 5b963107f6..78d7cc7277 100644
--- a/tests/ref/vsynth/vsynth_lena-h263-obmc
+++ b/tests/ref/vsynth/vsynth_lena-h263-obmc
@@ -1,4 +1,4 @@
 3c6946f808412ac320be9e0c36051ea2 *tests/data/fate/vsynth_lena-h263-obmc.avi
 154730 tests/data/fate/vsynth_lena-h263-obmc.avi
-588d992d9d8096da8bdc5027268da914 *tests/data/fate/vsynth_lena-h263-obmc.out.rawvideo
-stddev:    5.39 PSNR: 33.49 MAXDIFF:   82 bytes:  7603200/  7603200
+737af7fb166e2260ba049ae6bc30673d *tests/data/fate/vsynth_lena-h263-obmc.out.rawvideo
+stddev:    5.42 PSNR: 33.44 MAXDIFF:   77 bytes:  7603200/  7603200
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race

[Andreas Rheinhardt]

mpegvideo uses an array of Pictures and when it is done with using
them, it only unreferences them incompletely: Some buffers are kept
so that they can be reused lateron if the same slot in the Picture
array is reused, making this a sort of a bufferpool.
(Basically, a Picture is considered used if the AVFrame's buf is set.)
Yet given that other pieces of the decoder may have a reference to
these buffers, they need not be writable and are made writable using
av_buffer_make_writable() when preparing a new Picture. This involves
reading the buffer's data, although the old content of the buffer
need not be retained.

Worse, this read can be racy, because the buffer can be used by another
thread at the same time. This happens for Real Video 3 and 4.

This commit fixes this race by no longer copying the data;
instead the old buffer is replaced by a new, zero-allocated buffer.

(Here are the details of what happens with three or more decoding threads
when decoding rv30.rm from the FATE-suite as happens in the rv30 test:
The first decoding thread uses the first slot of its picture array
to store its current pic; update_thread_context copies this for the
second thread that decodes a P-frame. It uses the second slot in its
Picture array to store its P-frame. This arrangement is then copied
to the third decode thread, which decodes a B-frame. It uses the third
slot in its Picture array for its current frame.
update_thread_context copies this to the next thread. It unreferences
the third slot containing the other B-frame and then it reuses this
slot for its current frame. Because the pic array slots are only
incompletely unreferenced, the buffers of the previous B-frame are
still in there and they are not writable; in fact the previous
thread is concurrently writing to them, causing races when making
the buffer writable.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegpicture.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mpegpicture.c b/libavcodec/mpegpicture.c
index 2192f74cea..ed96abbe2d 100644
--- a/libavcodec/mpegpicture.c
+++ b/libavcodec/mpegpicture.c
@@ -47,11 +47,25 @@ static void av_noinline free_picture_tables(Picture *pic)
     }
 }
 
+static int make_table_writable(AVBufferRef **ref)
+{
+    AVBufferRef *old = *ref, *new;
+
+    if (av_buffer_is_writable(old))
+        return 0;
+    new = av_buffer_allocz(old->size);
+    if (!new)
+        return AVERROR(ENOMEM);
+    av_buffer_unref(ref);
+    *ref = new;
+    return 0;
+}
+
 static int make_tables_writable(Picture *pic)
 {
 #define MAKE_WRITABLE(table) \
 do {\
-    int ret = av_buffer_make_writable(&pic->table); \
+    int ret = make_table_writable(&pic->table); \
     if (ret < 0) \
         return ret; \
 } while (0)
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/6] avcodec/mpegvideodec: Constify some functions

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegvideo_dec.c | 4 ++--
 libavcodec/mpegvideodec.h  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/mpegvideo_dec.c b/libavcodec/mpegvideo_dec.c
index 93ba4e31b3..7566fe69f9 100644
--- a/libavcodec/mpegvideo_dec.c
+++ b/libavcodec/mpegvideo_dec.c
@@ -497,14 +497,14 @@ void ff_mpv_frame_end(MpegEncContext *s)
         ff_thread_report_progress(&s->current_picture_ptr->tf, INT_MAX, 0);
 }
 
-void ff_print_debug_info(MpegEncContext *s, Picture *p, AVFrame *pict)
+void ff_print_debug_info(const MpegEncContext *s, const Picture *p, AVFrame *pict)
 {
     ff_print_debug_info2(s->avctx, pict, s->mbskip_table, p->mb_type,
                          p->qscale_table, p->motion_val,
                          s->mb_width, s->mb_height, s->mb_stride, s->quarter_sample);
 }
 
-int ff_mpv_export_qp_table(MpegEncContext *s, AVFrame *f, Picture *p, int qp_type)
+int ff_mpv_export_qp_table(const MpegEncContext *s, AVFrame *f, const Picture *p, int qp_type)
 {
     AVVideoEncParams *par;
     int mult = (qp_type == FF_MPV_QSCALE_TYPE_MPEG1) ? 2 : 1;
diff --git a/libavcodec/mpegvideodec.h b/libavcodec/mpegvideodec.h
index fabc1b2202..250034b486 100644
--- a/libavcodec/mpegvideodec.h
+++ b/libavcodec/mpegvideodec.h
@@ -53,12 +53,12 @@ int ff_mpv_frame_start(MpegEncContext *s, AVCodecContext *avctx);
 void ff_mpv_report_decode_progress(MpegEncContext *s);
 void ff_mpv_frame_end(MpegEncContext *s);
 
-int ff_mpv_export_qp_table(MpegEncContext *s, AVFrame *f, Picture *p, int qp_type);
+int ff_mpv_export_qp_table(const MpegEncContext *s, AVFrame *f, const Picture *p, int qp_type);
 int ff_mpeg_update_thread_context(AVCodecContext *dst, const AVCodecContext *src);
 void ff_mpeg_draw_horiz_band(MpegEncContext *s, int y, int h);
 void ff_mpeg_flush(AVCodecContext *avctx);
 
-void ff_print_debug_info(MpegEncContext *s, Picture *p, AVFrame *pict);
+void ff_print_debug_info(const MpegEncContext *s, const Picture *p, AVFrame *pict);
 
 static inline int mpeg_get_qscale(MpegEncContext *s)
 {
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 6/6] avcodec/mpegvideo: Don't zero unnecessarily

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mpegvideo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index ff08168362..1190f29954 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -651,10 +651,10 @@ int ff_mpv_init_context_frame(MpegEncContext *s)
             s->dc_val_base[i] = 1024;
     }
 
-    /* which mb is an intra block,  init macroblock skip table */
-    if (!(s->mbintra_table = av_mallocz(mb_array_size)) ||
-        // Note the + 1 is for a quicker MPEG-4 slice_end detection
-        !(s->mbskip_table  = av_mallocz(mb_array_size + 2)))
+    // Note the + 1 is for a quicker MPEG-4 slice_end detection
+    if (!(s->mbskip_table  = av_mallocz(mb_array_size + 2)) ||
+        /* which mb is an intra block,  init macroblock skip table */
+        !(s->mbintra_table = av_malloc(mb_array_size)))
         return AVERROR(ENOMEM);
     memset(s->mbintra_table, 1, mb_array_size);
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6] avcodec/avcodec: Move initializing frame-thrd encoder to encode_preinit

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/avcodec.c | 6 ------
>  libavcodec/encode.c  | 6 ++++++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index f82d9e9f74..0451f57f82 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -283,12 +283,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if (ret < 0)
>          goto free_and_end;
>  
> -    if (CONFIG_FRAME_THREAD_ENCODER && av_codec_is_encoder(avctx->codec)) {
> -        ret = ff_frame_thread_encoder_init(avctx);
> -        if (ret < 0)
> -            goto free_and_end;
> -    }
> -
>      if (HAVE_THREADS
>          && !(avci->frame_thread_encoder && (avctx->active_thread_type&FF_THREAD_FRAME))) {
>          /* Frame-threaded decoders call FFCodec.init for their child contexts. */
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index 7919b165da..bd66f138a3 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -668,6 +668,12 @@ int ff_encode_preinit(AVCodecContext *avctx)
>              return AVERROR(ENOMEM);
>      }
>  
> +    if (CONFIG_FRAME_THREAD_ENCODER) {
> +        ret = ff_frame_thread_encoder_init(avctx);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
>      return 0;
>  }
>  

Will apply this patchset tonight unless there are objections.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4972 bytes --]

On Sat, Aug 13, 2022 at 05:03:04PM +0200, Andreas Rheinhardt wrote:
> mpegvideo uses an array of Pictures and when it is done with using
> them, it only unreferences them incompletely: Some buffers are kept
> so that they can be reused lateron if the same slot in the Picture
> array is reused, making this a sort of a bufferpool.
> (Basically, a Picture is considered used if the AVFrame's buf is set.)
> Yet given that other pieces of the decoder may have a reference to
> these buffers, they need not be writable and are made writable using
> av_buffer_make_writable() when preparing a new Picture. This involves
> reading the buffer's data, although the old content of the buffer
> need not be retained.
> 
> Worse, this read can be racy, because the buffer can be used by another
> thread at the same time. This happens for Real Video 3 and 4.
> 
> This commit fixes this race by no longer copying the data;
> instead the old buffer is replaced by a new, zero-allocated buffer.
> 
> (Here are the details of what happens with three or more decoding threads
> when decoding rv30.rm from the FATE-suite as happens in the rv30 test:
> The first decoding thread uses the first slot of its picture array
> to store its current pic; update_thread_context copies this for the
> second thread that decodes a P-frame. It uses the second slot in its
> Picture array to store its P-frame. This arrangement is then copied
> to the third decode thread, which decodes a B-frame. It uses the third
> slot in its Picture array for its current frame.
> update_thread_context copies this to the next thread. It unreferences
> the third slot containing the other B-frame and then it reuses this
> slot for its current frame. Because the pic array slots are only
> incompletely unreferenced, the buffers of the previous B-frame are
> still in there and they are not writable; in fact the previous
> thread is concurrently writing to them, causing races when making
> the buffer writable.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/mpegpicture.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

This causes a difference in some frames of: (i have not investigates why
just reporting as i noticed)
quite possibly thats just showing your bugfix in action

./ffmpeg -y -bitexact -i fate/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi 
./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -

@@ -141,7 +141,7 @@
 0,         22,         22,        1,   115200, 0x4cc933e9
 0,         23,         23,        1,   115200, 0x693a40a9
 0,         24,         24,        1,   115200, 0x956e3b15
-0,         25,         25,        1,   115200, 0x61763ff4
+0,         25,         25,        1,   115200, 0xc9e53d1c
 0,         26,         26,        1,   115200, 0x5c5c3dfc
 0,         27,         27,        1,   115200, 0x7de641ea
 0,         28,         28,        1,   115200, 0xe6cc4136
@@ -187,7 +187,7 @@
 0,         68,         68,        1,   115200, 0x49dcbf4e
 0,         69,         69,        1,   115200, 0x1ea1c7d1
 0,         70,         70,        1,   115200, 0xdf77c67b
-0,         71,         71,        1,   115200, 0x7f6bd16d
+0,         71,         71,        1,   115200, 0x33d9d206
 0,         72,         72,        1,   115200, 0x5e37cb3a
 0,         73,         73,        1,   115200, 0x15abcda3
 0,         74,         74,        1,   115200, 0xbf4dcbd4
@@ -199,7 +199,7 @@
 0,         80,         80,        1,   115200, 0x17d1d667
 0,         81,         81,        1,   115200, 0x0c1fdf9c
 0,         82,         82,        1,   115200, 0x7eabde6b
-0,         83,         83,        1,   115200, 0xe623e7af
+0,         83,         83,        1,   115200, 0x3bf6e873
 0,         84,         84,        1,   115200, 0xf480dc82
 0,         85,         85,        1,   115200, 0x5fd6e098
 0,         86,         86,        1,   115200, 0xf520de95
@@ -211,7 +211,7 @@
 0,         92,         92,        1,   115200, 0x34cfe1c2
 0,         93,         93,        1,   115200, 0x1d94e1c3
 0,         94,         94,        1,   115200, 0x6d32e147
-0,         95,         95,        1,   115200, 0x7e40ee91
+0,         95,         95,        1,   115200, 0x09fbefd0
 0,         96,         96,        1,   115200, 0xa5f5eb43
 0,         97,         97,        1,   115200, 0x39b9ec3d
 0,         98,         98,        1,   115200, 0x3256ec18

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race

[Andreas Rheinhardt]

Michael Niedermayer:
> On Sat, Aug 13, 2022 at 05:03:04PM +0200, Andreas Rheinhardt wrote:
>> mpegvideo uses an array of Pictures and when it is done with using
>> them, it only unreferences them incompletely: Some buffers are kept
>> so that they can be reused lateron if the same slot in the Picture
>> array is reused, making this a sort of a bufferpool.
>> (Basically, a Picture is considered used if the AVFrame's buf is set.)
>> Yet given that other pieces of the decoder may have a reference to
>> these buffers, they need not be writable and are made writable using
>> av_buffer_make_writable() when preparing a new Picture. This involves
>> reading the buffer's data, although the old content of the buffer
>> need not be retained.
>>
>> Worse, this read can be racy, because the buffer can be used by another
>> thread at the same time. This happens for Real Video 3 and 4.
>>
>> This commit fixes this race by no longer copying the data;
>> instead the old buffer is replaced by a new, zero-allocated buffer.
>>
>> (Here are the details of what happens with three or more decoding threads
>> when decoding rv30.rm from the FATE-suite as happens in the rv30 test:
>> The first decoding thread uses the first slot of its picture array
>> to store its current pic; update_thread_context copies this for the
>> second thread that decodes a P-frame. It uses the second slot in its
>> Picture array to store its P-frame. This arrangement is then copied
>> to the third decode thread, which decodes a B-frame. It uses the third
>> slot in its Picture array for its current frame.
>> update_thread_context copies this to the next thread. It unreferences
>> the third slot containing the other B-frame and then it reuses this
>> slot for its current frame. Because the pic array slots are only
>> incompletely unreferenced, the buffers of the previous B-frame are
>> still in there and they are not writable; in fact the previous
>> thread is concurrently writing to them, causing races when making
>> the buffer writable.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/mpegpicture.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> This causes a difference in some frames of: (i have not investigates why
> just reporting as i noticed)
> quite possibly thats just showing your bugfix in action
> 
> ./ffmpeg -y -bitexact -i fate/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi 
> ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -
> 
> @@ -141,7 +141,7 @@
>  0,         22,         22,        1,   115200, 0x4cc933e9
>  0,         23,         23,        1,   115200, 0x693a40a9
>  0,         24,         24,        1,   115200, 0x956e3b15
> -0,         25,         25,        1,   115200, 0x61763ff4
> +0,         25,         25,        1,   115200, 0xc9e53d1c
>  0,         26,         26,        1,   115200, 0x5c5c3dfc
>  0,         27,         27,        1,   115200, 0x7de641ea
>  0,         28,         28,        1,   115200, 0xe6cc4136
> @@ -187,7 +187,7 @@
>  0,         68,         68,        1,   115200, 0x49dcbf4e
>  0,         69,         69,        1,   115200, 0x1ea1c7d1
>  0,         70,         70,        1,   115200, 0xdf77c67b
> -0,         71,         71,        1,   115200, 0x7f6bd16d
> +0,         71,         71,        1,   115200, 0x33d9d206
>  0,         72,         72,        1,   115200, 0x5e37cb3a
>  0,         73,         73,        1,   115200, 0x15abcda3
>  0,         74,         74,        1,   115200, 0xbf4dcbd4
> @@ -199,7 +199,7 @@
>  0,         80,         80,        1,   115200, 0x17d1d667
>  0,         81,         81,        1,   115200, 0x0c1fdf9c
>  0,         82,         82,        1,   115200, 0x7eabde6b
> -0,         83,         83,        1,   115200, 0xe623e7af
> +0,         83,         83,        1,   115200, 0x3bf6e873
>  0,         84,         84,        1,   115200, 0xf480dc82
>  0,         85,         85,        1,   115200, 0x5fd6e098
>  0,         86,         86,        1,   115200, 0xf520de95
> @@ -211,7 +211,7 @@
>  0,         92,         92,        1,   115200, 0x34cfe1c2
>  0,         93,         93,        1,   115200, 0x1d94e1c3
>  0,         94,         94,        1,   115200, 0x6d32e147
> -0,         95,         95,        1,   115200, 0x7e40ee91
> +0,         95,         95,        1,   115200, 0x09fbefd0
>  0,         96,         96,        1,   115200, 0xa5f5eb43
>  0,         97,         97,        1,   115200, 0x39b9ec3d
>  0,         98,         98,        1,   115200, 0x3256ec18
> 
> [...]
> 

Decoding this sample uses earlier values of mbskip_table. If I zero
mbskip_table_buf in every ff_alloc_picture(), nothing changes due to
this patch and (most importantly) the output of decoding does not depend
upon the number of threads used (which it currently does -- with and
without this patch).
I don't know exactly where the bug is or whether there is a cheaper way
to mitigate it than just to zero the buffer every time.
I guess there are more such bugs affecting damaged samples than we are
currently aware of.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 6137 bytes --]

On Tue, Aug 16, 2022 at 10:38:55PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Sat, Aug 13, 2022 at 05:03:04PM +0200, Andreas Rheinhardt wrote:
> >> mpegvideo uses an array of Pictures and when it is done with using
> >> them, it only unreferences them incompletely: Some buffers are kept
> >> so that they can be reused lateron if the same slot in the Picture
> >> array is reused, making this a sort of a bufferpool.
> >> (Basically, a Picture is considered used if the AVFrame's buf is set.)
> >> Yet given that other pieces of the decoder may have a reference to
> >> these buffers, they need not be writable and are made writable using
> >> av_buffer_make_writable() when preparing a new Picture. This involves
> >> reading the buffer's data, although the old content of the buffer
> >> need not be retained.
> >>
> >> Worse, this read can be racy, because the buffer can be used by another
> >> thread at the same time. This happens for Real Video 3 and 4.
> >>
> >> This commit fixes this race by no longer copying the data;
> >> instead the old buffer is replaced by a new, zero-allocated buffer.
> >>
> >> (Here are the details of what happens with three or more decoding threads
> >> when decoding rv30.rm from the FATE-suite as happens in the rv30 test:
> >> The first decoding thread uses the first slot of its picture array
> >> to store its current pic; update_thread_context copies this for the
> >> second thread that decodes a P-frame. It uses the second slot in its
> >> Picture array to store its P-frame. This arrangement is then copied
> >> to the third decode thread, which decodes a B-frame. It uses the third
> >> slot in its Picture array for its current frame.
> >> update_thread_context copies this to the next thread. It unreferences
> >> the third slot containing the other B-frame and then it reuses this
> >> slot for its current frame. Because the pic array slots are only
> >> incompletely unreferenced, the buffers of the previous B-frame are
> >> still in there and they are not writable; in fact the previous
> >> thread is concurrently writing to them, causing races when making
> >> the buffer writable.)
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> ---
> >>  libavcodec/mpegpicture.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > This causes a difference in some frames of: (i have not investigates why
> > just reporting as i noticed)
> > quite possibly thats just showing your bugfix in action
> > 
> > ./ffmpeg -y -bitexact -i fate/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5  -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi 
> > ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc -
> > 
> > @@ -141,7 +141,7 @@
> >  0,         22,         22,        1,   115200, 0x4cc933e9
> >  0,         23,         23,        1,   115200, 0x693a40a9
> >  0,         24,         24,        1,   115200, 0x956e3b15
> > -0,         25,         25,        1,   115200, 0x61763ff4
> > +0,         25,         25,        1,   115200, 0xc9e53d1c
> >  0,         26,         26,        1,   115200, 0x5c5c3dfc
> >  0,         27,         27,        1,   115200, 0x7de641ea
> >  0,         28,         28,        1,   115200, 0xe6cc4136
> > @@ -187,7 +187,7 @@
> >  0,         68,         68,        1,   115200, 0x49dcbf4e
> >  0,         69,         69,        1,   115200, 0x1ea1c7d1
> >  0,         70,         70,        1,   115200, 0xdf77c67b
> > -0,         71,         71,        1,   115200, 0x7f6bd16d
> > +0,         71,         71,        1,   115200, 0x33d9d206
> >  0,         72,         72,        1,   115200, 0x5e37cb3a
> >  0,         73,         73,        1,   115200, 0x15abcda3
> >  0,         74,         74,        1,   115200, 0xbf4dcbd4
> > @@ -199,7 +199,7 @@
> >  0,         80,         80,        1,   115200, 0x17d1d667
> >  0,         81,         81,        1,   115200, 0x0c1fdf9c
> >  0,         82,         82,        1,   115200, 0x7eabde6b
> > -0,         83,         83,        1,   115200, 0xe623e7af
> > +0,         83,         83,        1,   115200, 0x3bf6e873
> >  0,         84,         84,        1,   115200, 0xf480dc82
> >  0,         85,         85,        1,   115200, 0x5fd6e098
> >  0,         86,         86,        1,   115200, 0xf520de95
> > @@ -211,7 +211,7 @@
> >  0,         92,         92,        1,   115200, 0x34cfe1c2
> >  0,         93,         93,        1,   115200, 0x1d94e1c3
> >  0,         94,         94,        1,   115200, 0x6d32e147
> > -0,         95,         95,        1,   115200, 0x7e40ee91
> > +0,         95,         95,        1,   115200, 0x09fbefd0
> >  0,         96,         96,        1,   115200, 0xa5f5eb43
> >  0,         97,         97,        1,   115200, 0x39b9ec3d
> >  0,         98,         98,        1,   115200, 0x3256ec18
> > 
> > [...]
> > 
> 
> Decoding this sample uses earlier values of mbskip_table. If I zero
> mbskip_table_buf in every ff_alloc_picture(), nothing changes due to
> this patch and (most importantly) the output of decoding does not depend
> upon the number of threads used (which it currently does -- with and
> without this patch).
> I don't know exactly where the bug is or whether there is a cheaper way
> to mitigate it than just to zero the buffer every time.
> I guess there are more such bugs affecting damaged samples than we are
> currently aware of.

Theres code in ff_er_frame_end() which cleans the mbskip_table
I dont remember it very much but it looks like that should not be optional
If this will not be run mbskip_table should maybe be zeroed on "alloc"

not sure this is the issue, iam just looking at the code 

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".