Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
Fixed so threading isn't removed completely.
Sorry this took so long. Took too long to figure out that decode_ihdr_chunk()
was being called in each thread and thus overwriting the cur_w/h value set by
update_thread_context().
("Remove threading support" patch can be ignored now)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
Hi,
On Wed, Jun 3, 2015 at 1:20 AM, Donny Yang wrote:
> On 3 June 2015 at 04:15, Ronald S. Bultje wrote:
> > On Tue, Jun 2, 2015 at 1:42 PM, Donny Yang wrote:
> > > On 3 June 2015 at 03:31, Paul B Mahol wrote:
> > > > Dana 2. 6. 2015. 17:49 osoba "Donny Yang" napisala
> je:
> > > > >
> > > > > Each frame depends on the previous frame any way, and it will
> > > > > cause bugs with frame disposal
> > > > >
> > > > > Signed-off-by: Donny Yang
> > > > > ---
> > > > > 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
On 3 June 2015 at 04:15, Ronald S. Bultje wrote:
> Hi,
>
> On Tue, Jun 2, 2015 at 1:42 PM, Donny Yang wrote:
>
> > On 3 June 2015 at 03:31, Paul B Mahol wrote:
> >
> > > Dana 2. 6. 2015. 17:49 osoba "Donny Yang" napisala je:
> > > >
> > > > Each frame depends on the previous frame any way, and it will
> > > > cause bugs with frame disposal
> > > >
> > > > Signed-off-by: Donny Yang
> > > > ---
> > > > 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
Hi,
On Tue, Jun 2, 2015 at 1:42 PM, Donny Yang wrote:
> On 3 June 2015 at 03:31, Paul B Mahol wrote:
>
> > Dana 2. 6. 2015. 17:49 osoba "Donny Yang" napisala je:
> > >
> > > Each frame depends on the previous frame any way, and it will
> > > cause bugs with frame disposal
> > >
> > > Signed-off-by: Donny Yang
> > > ---
> > > 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.
Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
On 3 June 2015 at 03:31, Paul B Mahol wrote:
> Dana 2. 6. 2015. 17:49 osoba "Donny Yang" napisala je:
> >
> > Each frame depends on the previous frame any way, and it will
> > cause bugs with frame disposal
> >
> > Signed-off-by: Donny Yang
> > ---
> > 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.
But if we wait for the frame before reading the next frame, we've
effectively made it only execute a single thread at a time, which is why
I've removed threading support altogether.
I suppose another possible solution is to only have decompression be
multithreaded while everything else is single threaded.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/6] avcodec/apng: Remove threading support
Dana 2. 6. 2015. 17:49 osoba "Donny Yang" napisala je:
>
> Each frame depends on the previous frame any way, and it will
> cause bugs with frame disposal
>
> Signed-off-by: Donny Yang
> ---
> 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?
___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
