[FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race
Michael Niedermayer
michael at niedermayer.cc
Wed Aug 17 00:09:43 EEST 2022
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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220816/535f8150/attachment.sig>
More information about the ffmpeg-devel
mailing list