Message ID | 20210211215833.3133-1-onemda@gmail.com |
---|---|
State | Accepted |
Commit | 63231fa8d30f41045658d6c382b00fe1ebe18d05 |
Headers | show |
Series | [FFmpeg-devel] avcodec/pngdec: fix possible race condition with APNG decoding | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
Quoting Paul B Mahol (2021-02-11 22:58:33) > Fixes #9017 > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/pngdec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > index 395b86bbe7..61642b7cbe 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.17.1 It's pretty non-obvious what the race is and how is it fixed by reordering the calls.
On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2021-02-11 22:58:33) > > Fixes #9017 > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > --- > > libavcodec/pngdec.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > > index 395b86bbe7..61642b7cbe 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.17.1 > > It's pretty non-obvious what the race is and how is it fixed by > reordering the calls. > Before patch hash of decoded output would differ with single threading and multiple threads. Now it does not. > > -- > 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".
Quoting Paul B Mahol (2021-02-14 11:41:20) > On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Paul B Mahol (2021-02-11 22:58:33) > > > Fixes #9017 > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > --- > > > libavcodec/pngdec.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > > > index 395b86bbe7..61642b7cbe 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.17.1 > > > > It's pretty non-obvious what the race is and how is it fixed by > > reordering the calls. > > > > Before patch hash of decoded output would differ with single threading and > multiple threads. > Now it does not. That still does not explain where the bug is and why this specific change is the right way to fix it.
On Sun, Feb 14, 2021 at 11:44 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2021-02-14 11:41:20) > > On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> > wrote: > > > > > Quoting Paul B Mahol (2021-02-11 22:58:33) > > > > Fixes #9017 > > > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > --- > > > > libavcodec/pngdec.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > > > > index 395b86bbe7..61642b7cbe 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.17.1 > > > > > > It's pretty non-obvious what the race is and how is it fixed by > > > reordering the calls. > > > > > > > Before patch hash of decoded output would differ with single threading > and > > multiple threads. > > Now it does not. > > That still does not explain where the bug is and why this specific > change is the right way to fix it. > Speedup gain is still there with multiple threads. And when using helgrind one of items was gone and no new items were introduced. > > -- > 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".
Quoting Paul B Mahol (2021-02-14 11:48:14) > On Sun, Feb 14, 2021 at 11:44 AM Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Paul B Mahol (2021-02-14 11:41:20) > > > On Sun, Feb 14, 2021 at 11:21 AM Anton Khirnov <anton@khirnov.net> > > wrote: > > > > > > > Quoting Paul B Mahol (2021-02-11 22:58:33) > > > > > Fixes #9017 > > > > > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > --- > > > > > libavcodec/pngdec.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > > > > > index 395b86bbe7..61642b7cbe 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.17.1 > > > > > > > > It's pretty non-obvious what the race is and how is it fixed by > > > > reordering the calls. > > > > > > > > > > Before patch hash of decoded output would differ with single threading > > and > > > multiple threads. > > > Now it does not. > > > > That still does not explain where the bug is and why this specific > > change is the right way to fix it. > > > > Speedup gain is still there with multiple threads. > And when using helgrind one of items was gone and no new items were > introduced. That still does not explain where the bug was. If you used helgrind, you might as well explain what it found.
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index 395b86bbe7..61642b7cbe 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;
Fixes #9017 Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/pngdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)