diff mbox series

[FFmpeg-devel,4/6] Revert "avcodec/pngdec: fix possible race condition with APNG decoding"

Message ID 20210216202416.12116-4-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/6] tests: add a test for LSCR
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Anton Khirnov Feb. 16, 2021, 8:24 p.m. UTC
This reverts commit 63231fa8d30f41045658d6c382b00fe1ebe18d05. It did not
actually fix the cited issue, nor is the issue a race. It will be fixed
properly in the following commit.
---
 libavcodec/pngdec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paul B Mahol Feb. 16, 2021, 8:57 p.m. UTC | #1
Do you have actual proof for such claims?

There is no point in reverting commit if that commit fixed some behavior.

On Tue, Feb 16, 2021 at 9:26 PM Anton Khirnov <anton@khirnov.net> wrote:

> This reverts commit 63231fa8d30f41045658d6c382b00fe1ebe18d05. It did not
> actually fix the cited issue, nor is the issue a race. It will be fixed
> properly in the following commit.
> ---
>  libavcodec/pngdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index e05e44f564..d02c45f1a7 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -711,13 +711,13 @@ static int decode_idat_chunk(AVCodecContext *avctx,
> PNGDecContext *s,
>              s->bpp += byte_depth;
>          }
>
> +        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> AV_GET_BUFFER_FLAG_REF)) < 0)
> +            return ret;
>          if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op !=
> APNG_DISPOSE_OP_PREVIOUS) {
>              ff_thread_release_buffer(avctx, &s->previous_picture);
>              if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture,
> AV_GET_BUFFER_FLAG_REF)) < 0)
>                  return ret;
>          }
> -        if ((ret = ff_thread_get_buffer(avctx, &s->picture,
> AV_GET_BUFFER_FLAG_REF)) < 0)
> -            return ret;
>          p->pict_type        = AV_PICTURE_TYPE_I;
>          p->key_frame        = 1;
>          p->interlaced_frame = !!s->interlace_type;
> --
> 2.28.0
>
> _______________________________________________
> 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".
Anton Khirnov Feb. 17, 2021, 8:14 a.m. UTC | #2
Quoting Paul B Mahol (2021-02-16 21:57:47)
> Do you have actual proof for such claims?

The burden of proof is on you here - you are supposed to prove that your
commit fixes something. And when I asked you about details, you couldn't
even tell me what the bug was, much less why would switching the order
of allocations be the correct fix for it.

> 
> There is no point in reverting commit if that commit fixed some behavior.

It didn't fix anything though. Before your commit, frame 69 of the
sample in #9017 is almost black with 1 thread and looks okay with 2
threads. After your commit, it is almost black in both cases. So your
commit made it consistently wrong. This shows up in my tests and is also
supported by the last comment in https://trac.ffmpeg.org/ticket/9017
Feel free to test it yourself.
Paul B Mahol Feb. 17, 2021, 10:45 a.m. UTC | #3
On Wed, Feb 17, 2021 at 9:14 AM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Paul B Mahol (2021-02-16 21:57:47)
> > Do you have actual proof for such claims?
>
> The burden of proof is on you here - you are supposed to prove that your
> commit fixes something. And when I asked you about details, you couldn't
> even tell me what the bug was, much less why would switching the order
> of allocations be the correct fix for it.
>

It is not about order of allocations.


>
> >
> > There is no point in reverting commit if that commit fixed some behavior.
>
> It didn't fix anything though. Before your commit, frame 69 of the
> sample in #9017 is almost black with 1 thread and looks okay with 2
> threads. After your commit, it is almost black in both cases. So your
> commit made it consistently wrong. This shows up in my tests and is also
>


You are mistaken. It fixed race. Prove it does not.
Another patch is supposed to fix output for both single and multi threads.



> supported by the last comment in https://trac.ffmpeg.org/ticket/9017
> Feel free to test it yourself.
>
> Next not reviewed patch fixed it completely.
Thanks for actually confirming it makes consistent result with different
threads and as such commit should not
be reverted.

If you have no proof that your changes too apng code improves anything
besides line counts than
I nack whole patch set.

I have not tested your set at all. But plan to if you provide file you used
to actually test this and that file is different from already provided file
on trac.
Jean-Baptiste Kempf Feb. 17, 2021, 11:05 a.m. UTC | #4
On Wed, 17 Feb 2021, at 11:45, Paul B Mahol wrote:
> > It didn't fix anything though. Before your commit, frame 69 of the
> > sample in #9017 is almost black with 1 thread and looks okay with 2
> > threads. After your commit, it is almost black in both cases. So your
> > commit made it consistently wrong. This shows up in my tests and is also
> 
> You are mistaken. It fixed race. Prove it does not.
> Another patch is supposed to fix output for both single and multi threads.

Which patch fixes everything?
Anton Khirnov Feb. 17, 2021, 11:31 a.m. UTC | #5
Quoting Paul B Mahol (2021-02-17 11:45:02)
> On Wed, Feb 17, 2021 at 9:14 AM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Paul B Mahol (2021-02-16 21:57:47)
> > > Do you have actual proof for such claims?
> >
> > The burden of proof is on you here - you are supposed to prove that your
> > commit fixes something. And when I asked you about details, you couldn't
> > even tell me what the bug was, much less why would switching the order
> > of allocations be the correct fix for it.
> >
> 
> It is not about order of allocations.

What is it about then?

> 
> 
> >
> > >
> > > There is no point in reverting commit if that commit fixed some behavior.
> >
> > It didn't fix anything though. Before your commit, frame 69 of the
> > sample in #9017 is almost black with 1 thread and looks okay with 2
> > threads. After your commit, it is almost black in both cases. So your
> > commit made it consistently wrong. This shows up in my tests and is also
> >
> 
> 
> You are mistaken. It fixed race. Prove it does not.

You have it backwards. You are making a claim - that your patch fixes a
race. It is your responsibility to prove that your patch is correct. So
far you did not even explain what this supposed race is.
Paul B Mahol Feb. 17, 2021, 1:33 p.m. UTC | #6
On Wed, Feb 17, 2021 at 12:31 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Paul B Mahol (2021-02-17 11:45:02)
> > On Wed, Feb 17, 2021 at 9:14 AM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > > Quoting Paul B Mahol (2021-02-16 21:57:47)
> > > > Do you have actual proof for such claims?
> > >
> > > The burden of proof is on you here - you are supposed to prove that
> your
> > > commit fixes something. And when I asked you about details, you
> couldn't
> > > even tell me what the bug was, much less why would switching the order
> > > of allocations be the correct fix for it.
> > >
> >
> > It is not about order of allocations.
>
> What is it about then?
>
> >
> >
> > >
> > > >
> > > > There is no point in reverting commit if that commit fixed some
> behavior.
> > >
> > > It didn't fix anything though. Before your commit, frame 69 of the
> > > sample in #9017 is almost black with 1 thread and looks okay with 2
> > > threads. After your commit, it is almost black in both cases. So your
> > > commit made it consistently wrong. This shows up in my tests and is
> also
> > >
> >
> >
> > You are mistaken. It fixed race. Prove it does not.
>
> You have it backwards. You are making a claim - that your patch fixes a
> race. It is your responsibility to prove that your patch is correct. So
> far you did not even explain what this supposed race is.
>

I'm loosing my precious time discussing irrelevant matters here.

Is helgrind report clean with your patch set?


> --
> Anton Khirnov
> _______________________________________________
> 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".
Anton Khirnov Feb. 18, 2021, 12:15 p.m. UTC | #7
Quoting Paul B Mahol (2021-02-17 14:33:25)
> On Wed, Feb 17, 2021 at 12:31 PM Anton Khirnov <anton@khirnov.net> wrote:
> 
> Is helgrind report clean with your patch set?

Helgrind is not clean before your patch, after your patch, or after my
set. The reason is because helgrind does not understand atomics, so its
reports are bogus.
Paul B Mahol Feb. 18, 2021, 12:27 p.m. UTC | #8
On Thu, Feb 18, 2021 at 1:15 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Paul B Mahol (2021-02-17 14:33:25)
> > On Wed, Feb 17, 2021 at 12:31 PM Anton Khirnov <anton@khirnov.net>
> wrote:
> >
> > Is helgrind report clean with your patch set?
>
> Helgrind is not clean before your patch, after your patch, or after my
> set. The reason is because helgrind does not understand atomics, so its
> reports are bogus.
>

I only can accept this patch if you squash it with next one.



>
> --
> Anton Khirnov
> _______________________________________________
> 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".
Anton Khirnov Feb. 18, 2021, 12:33 p.m. UTC | #9
Quoting Paul B Mahol (2021-02-18 13:27:26)
> On Thu, Feb 18, 2021 at 1:15 PM Anton Khirnov <anton@khirnov.net> wrote:
> 
> > Quoting Paul B Mahol (2021-02-17 14:33:25)
> > > On Wed, Feb 17, 2021 at 12:31 PM Anton Khirnov <anton@khirnov.net>
> > wrote:
> > >
> > > Is helgrind report clean with your patch set?
> >
> > Helgrind is not clean before your patch, after your patch, or after my
> > set. The reason is because helgrind does not understand atomics, so its
> > reports are bogus.
> >
> 
> I only can accept this patch if you squash it with next one.

sure
diff mbox series

Patch

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index e05e44f564..d02c45f1a7 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -711,13 +711,13 @@  static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
             s->bpp += byte_depth;
         }
 
+        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
+            return ret;
         if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) {
             ff_thread_release_buffer(avctx, &s->previous_picture);
             if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0)
                 return ret;
         }
-        if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0)
-            return ret;
         p->pict_type        = AV_PICTURE_TYPE_I;
         p->key_frame        = 1;
         p->interlaced_frame = !!s->interlace_type;