[FFmpeg-devel] [PATCH] libavcodec/pngdec: support 'previous' dispose operation for APNG.

[Michael Niedermayer]

On Tue, Dec 02, 2014 at 08:44:02AM +0100, Benoit Fouet wrote:
> Hi,
> 
> On December 1, 2014 11:34:44 PM GMT+01:00, Michael Niedermayer <michaelni at gmx.at> wrote:
> >On Mon, Dec 01, 2014 at 11:41:41AM +0100, Benoit Fouet wrote:
> >> ---
> >> Tested against all the materials I have at hand.
> >> There is an artifact showing for
> >https://raw.githubusercontent.com/maxcom/lorsource/master/src/test/resources/images/i_want_to_be_a_hero__apng_animated__by_tamalesyatole-d5ht8eu.png
> >> which I don't really understand, as it seems the individual frames
> >are correct
> >> for our decoder, but the disposal that's done for other decoders
> >(tested
> >> firefox and chrome) is not the same for the end of the cape.
> >> ---
> >>  libavcodec/pngdec.c | 93
> >++++++++++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 71 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> >> index 9e52d0b..2ca3dee 100644
> >> --- a/libavcodec/pngdec.c
> >> +++ b/libavcodec/pngdec.c
> >> @@ -23,6 +23,7 @@
> >>  
> >>  #include "libavutil/bprint.h"
> >>  #include "libavutil/imgutils.h"
> >> +#include "libavutil/thread.h"
> >>  #include "avcodec.h"
> >>  #include "bytestream.h"
> >>  #include "internal.h"
> >> @@ -38,9 +39,16 @@ typedef struct PNGDecContext {
> >>      AVCodecContext *avctx;
> >>  
> >>      GetByteContext gb;
> >> +    ThreadFrame previous_picture;
> >>      ThreadFrame last_picture;
> >>      ThreadFrame picture;
> >>  
> >> +#if CONFIG_APNG_DECODER
> >> +    AVMutex mutex;
> >> +    int frame_id;
> >> +    int *pframe_id;
> >> +#endif
> >
> >why do you need a mutex ?
> >
> 
> Actually, the only thing I need is the frame index. The best place for that would be in the demuxer, but I didn't find a place where this information is accessible. Did I miss something (I hope so)? Do you think I should be using side data for this?
> To answer the question though, the access is done is all the decoder threads, so I did not want the reset to happen between the reading and the writing of the ++.
> Thinking more about this, I think it's wrong anyway. I really need the demuxer to handle this, it would be simpler and more correct...

iam not sure i understand
considering a single threaded decoder it cannot change anything
for a past or future iteration (because these dont exist anymore or
yet) but only its own state now with a multi threaded decoder it gets
a copy of the previous
decoders state and changes its own state only, nothing else should
change its state so there should be no need for a mutex.
the thread for the next frame would not start before the current
is done with its basic setup of stuff like the frame index

the frame index for each frame is a copy of the last + 1
i assume theres enough information in the bitstream for the decoder
to know when to reset the index and seeking if ts supported would
call avcodec_flush_buffers()

but quite possibly iam missing something

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141202/d45d2d85/attachment.asc>