[FFmpeg-devel] [PATCH 5/6] pngdec: fix and simplify apng reference handling

[James Almer]

On 3/30/2021 5:56 AM, Michael Niedermayer wrote:
> On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote:
>> Current code is very confused and confusing. It uses two different
>> reference frames - "previous" and "last" - when only one is really
>> necessary. It also confuses the two, leading to incorrect output with
>> APNG_DISPOSE_OP_PREVIOUS mode.
>>
>> Fixes #9017.
>> ---
>>   libavcodec/pngdec.c | 93 ++++++++++++++++++++-------------------------
>>   1 file changed, 42 insertions(+), 51 deletions(-)
> 
> 
> [...]
> 
>> @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
>>       if (!buffer)
>>           return AVERROR(ENOMEM);
>>   
>> +    ff_thread_await_progress(&s->last_picture, INT_MAX, 0);
>>   
>> -    // Do the disposal operation specified by the last frame on the frame
>> -    if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) {
>> -        ff_thread_await_progress(&s->last_picture, INT_MAX, 0);
>> -        memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height);
>> -
>> -        if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND)
>> -            for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y)
>> -                memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w);
>> +    // need to reset a rectangle to background:
>> +    // create a new writable copy
>> +    if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) {
>> +        int ret = av_frame_make_writable(s->last_picture.f);
>> +        if (ret < 0)
>> +            return ret;
>>   
>> -        memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height);
>> -        ff_thread_report_progress(&s->previous_picture, INT_MAX, 0);
>> -    } else {
>> -        ff_thread_await_progress(&s->previous_picture, INT_MAX, 0);
>> -        memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height);
>> +        for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) {
>> +            memset(s->last_picture.f->data[0] + s->image_linesize * y +
>> +                   s->bpp * s->last_x_offset, 0, s->bpp * s->last_w);
>> +        }
>>       }
>>   
>> +    memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height);
>> +
> 
> This results in out of array reads
> 
> av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup()
> assumes all frames have the same linesize

FATE didn't detect this? What sample triggers these out of array reads?

Also, instead of av_frame_make_writable(), this should probably call 
ff_thread_get_buffer() to get a new buffer, then copy the old data if 
needed. The difference in linesize is because 
avcodec_default_get_buffer2() allocates buffers in a different way than 
av_frame_get_buffer() as invoked by av_frame_make_writable().

> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>