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

[Benoit Fouet]

Le 02/12/2014 15:21, Michael Niedermayer a écrit :
> 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 (flawed) solution I used was to have in each context a pointer to 
the frame index of the first allocated one...

> 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'll try this simple approach. I just didn't quite look at how the 
threads were handled, but it seems that just incrementing the frame 
index unconditionnaly would be enough for my use case.

> i assume theres enough information in the bitstream for the decoder
> to know when to reset the index

yes

>   and seeking if ts supported would
> call avcodec_flush_buffers()
>
> but quite possibly iam missing something

Doesn't seem so. That was me, just as I suspected...

-- 
Ben