[FFmpeg-devel] [PATCH 1/2] Revert "avcodec/qtrle: Do not output duplicated frames on insufficient input"

[Marton Balint]

On Wed, 8 May 2019, Michael Niedermayer wrote:

> On Tue, May 07, 2019 at 02:03:22AM +0200, Marton Balint wrote:
>>
>>
>> On Tue, 7 May 2019, Michael Niedermayer wrote:
>>
>>> On Sun, May 05, 2019 at 08:51:08PM +0200, Marton Balint wrote:
>>>> This reverts commit a9dacdeea6168787a142209bd19fdd74aefc9dd6.
>>>>
>>>> I don't think it is a good idea to drop frames from CFR input just because they
>>>> are duplicated, that can cause issues for API users expecting CFR input. Also
>>>> it can cause issues at the end of file, if the last frame is a duplicated
>>>> frame.
>>>>
>>>> Fixes ticket #7880.
>>>>
>>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>>>> ---
>>>> libavcodec/qtrle.c        |  12 ++---
>>>> tests/ref/fate/qtrle-8bit | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 115 insertions(+), 6 deletions(-)
>>>
>>> This change would make the decoder quite a bit slower.
>>
>> I guess that can be easily fixed by only copying the buffer if it really is
>> a different frame.
>>
>>> It also would make encoding the output harder.
>>> For example motion estimation would be run over unchanged frames even when
>>> no cfr is wanted.
>>
>> The performance penalty is much more acceptable to me than the issue
>> described in the ticket. Do you see a straightforward way to fix it other
>> than reverting?
>
> decoders can in general have frames at the end which need to be flushed
> out. For example IPB mpeg1/2/4/...
> In the same way the decoder could output a last frame representing the end
> exactly

Yeah, that probably would fix the ticket. However I am still worried 
that it is just a matter of time before somebody reports something else 
that is broken because of VFR and unknown frame duration.

Do you plan on working on fixing the ticket? It is a regression, so 
prefeably it should either be fixed in a reasonable time frame or if 
nobody is willing to do it then we should revert.

The original ossfuzz issue might also be fixed by keeping only the hunk 
which moves ff_reget_buffer down.

Regards,
Marton