[FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support

[Donny Yang]

On 3 June 2015 at 04:15, Ronald S. Bultje <rsbultje at gmail.com> wrote:

> Hi,
>
> On Tue, Jun 2, 2015 at 1:42 PM, Donny Yang <work at kota.moe> wrote:
>
> > On 3 June 2015 at 03:31, Paul B Mahol <onemda at gmail.com> wrote:
> >
> > > Dana 2. 6. 2015. 17:49 osoba "Donny Yang" <work at kota.moe> napisala je:
> > > >
> > > > Each frame depends on the previous frame any way, and it will
> > > > cause bugs with frame disposal
> > > >
> > > > Signed-off-by: Donny Yang <work at kota.moe>
> > > > ---
> > > >  libavcodec/pngdec.c | 11 +----------
> > > >  1 file changed, 1 insertion(+), 10 deletions(-)
> > > >
> > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > > > index 2512799..2ea3e8b 100644
> > > > --- a/libavcodec/pngdec.c
> > > > +++ b/libavcodec/pngdec.c
> > > > @@ -1224,17 +1224,10 @@ static int
> update_thread_context(AVCodecContext
> > > *dst, const AVCodecContext *src)
> > > >      if (dst == src)
> > > >          return 0;
> > > >
> > > > -    pdst->frame_id = psrc->frame_id;
> > > > -
> > > >      ff_thread_release_buffer(dst, &pdst->picture);
> > > >      if (psrc->picture.f->data[0] &&
> > > >          (ret = ff_thread_ref_frame(&pdst->picture, &psrc->picture))
> <
> > 0)
> > > >          return ret;
> > > > -    if (CONFIG_APNG_DECODER && dst->codec_id == AV_CODEC_ID_APNG) {
> > > > -        ff_thread_release_buffer(dst, &pdst->last_picture);
> > > > -        if (psrc->last_picture.f->data[0])
> > > > -            return ff_thread_ref_frame(&pdst->last_picture,
> > > &psrc->last_picture);
> > > > -    }
> > > >
> > > >      return 0;
> > > >  }
> > > > @@ -1294,9 +1287,7 @@ AVCodec ff_apng_decoder = {
> > > >      .init           = png_dec_init,
> > > >      .close          = png_dec_end,
> > > >      .decode         = decode_frame_apng,
> > > > -    .init_thread_copy = ONLY_IF_THREADS_ENABLED(png_dec_init),
> > > > -    .update_thread_context =
> > > ONLY_IF_THREADS_ENABLED(update_thread_context),
> > > > -    .capabilities   = CODEC_CAP_DR1 | CODEC_CAP_FRAME_THREADS /*|
> > > CODEC_CAP_DRAW_HORIZ_BAND*/,
> > > > +    .capabilities   = CODEC_CAP_DR1 /*| CODEC_CAP_DRAW_HORIZ_BAND*/,
> > > >  };
> > > >  #endif
> > > >
> > > > --
> > > > 2.4.0
> > > >
> > >
> > > Hmm, have you checked it is really broken now?
> > >
> >
> > It wasn't broken originally, but breaks when I fix the frame disposing.
> > More specifically, since each frame depends on the metadata of the
> previous
> > frame as well as the frame itself, having it threaded would give
> > inconsistent last-frame metadata, unless we wait for the frame before
> > reading the next frame's metadata.
>
>
> What is this metadata? I'm fairly confused over this.
>

Metadata as referring to the data contained in the fcTL chunks, stored
inside PNGDecContext in ffmpeg.
More specifically, these properties:
    int last_w, last_h;
    int last_x_offset, last_y_offset;
    uint8_t last_dispose_op;

For example, when I tested with
https://people.mozilla.org/~dolske/apng/demo-2-over+previous.png
last_dispose_op would always be APNG_DISPOSE_OP_NONE rather than
APNG_DISPOSE_OP_PREVIOUS
for the second frame. the last_* properties were probably also wrong, but I
didn't check those.
Disabling threading fixed it.