[FFmpeg-devel] [PATCH 05/11] avcodec/h264_parser: Reuse the RBSP buffer

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Kieran Kunhya:
>> On Fri, 16 Aug 2019 at 06:08, Andreas Rheinhardt <
>> andreas.rheinhardt at gmail.com> wrote:
>>
>>> Kieran Kunhya:
>>>> On Fri, 16 Aug 2019 at 04:20, Andreas Rheinhardt <
>>>> andreas.rheinhardt at gmail.com> wrote:
>>>>
>>>>> Up until now, the H.264 parser has allocated a new buffer for every
>>>>> frame in order to store the unescaped RBSP in case the part of the RBSP
>>>>> that will be unescaped contains 0x03 escape bytes. This is expensive
>>>>> and this commit changes this: The buffer is now kept and reused.
>>>>>
>>>>> For an AVC file with an average framesize of 15304 B (without in-band
>>>>> parameter sets etc.) and 132044 frames (131072 of which ended up in the
>>>>> results) this reduced the average time used by the H.264 parser from
>>>>> 87708 decicycles (excluding about 1100-1200 skips in each iteration)
>>>>> to 16963 decicycles (excluding about 10-15 skips in each iteration).
>>>>> The test has been iterated 10 times.
>>>>>
>>>>
>>>> If I understand correctly, this patch means if you have a large frame (or
>>>> large NAL) you are stuck with the large allocation of memory until the
>>>> decoder is closed.
>>>> Not sure if you have read the discussion here
>>>> https://patchwork.ffmpeg.org/patch/5631/
>>>>
>>>> Kieran
>>>>
>>> I am aware of said discussion; and also of your solution [1] to it. It
>>> actually does exactly the same as I propose for the parser: It keeps a
>>> single buffer that does not shrink. Given that this is used for the
>>> decoders (and for cbs_h2645), I didn't deem this to be problematic.
>>> (There is clearly no quadratic memory usage and what Derek warns about
>>> (with huge NALs at specific positions) is a no issue for both.)
>>>
>>> - Andreas
>>>
>>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/219100.html
>>
>>
>> My solution frees the buffer when it's done. With yours it stays around as
>> a large buffer essentially forever.
>>
>> Kieran
>>
> Your solution frees the buffer in the parser when it's done, but the
> buffers for the decoders are kept (and only grow) during decoding. So
> this commit merely does for the parser what is already done for the
> deocder.
> Anyway, it would be easy to add a check to the parser to free the
> buffer if it is considered excessively large. I don't know how easy it
> would be to add such precautions to the decoder, though. I am also not
> sure what should be considered excessively large? 5 MB? 10 MB? Setting
> it so high that even the highest profiles can't hit it is impossible,
> because for certain profiles ((Progressive) High 10, High 4:2:2, ...)
> no limit is imposed at all (apart from that, such a limit would surely
> be too high).
> 
> - Andreas
> 
Ping. What is other's opinion on this matter? Notice that the current
behaviour is suboptimal even if it is decided that the buffer should not
be kept: It allocates 1/16 more than needed (in addition to
AV_INPUT_BUFFER_PADDING_SIZE) that is guaranteed not to be used; and it
uses mallocz for the allocation.

- Andreas