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

[Ronald S. Bultje]

Hi,

On Wed, Jun 3, 2015 at 1:20 AM, Donny Yang <work at kota.moe> wrote:

> On 3 June 2015 at 04:15, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> > 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.


So sync these struct members in the update_thread_context callback (check
mpeg/h26x/vpx codecs for examples)? Disabling threading seems like a bit
much.

Ronald