[FFmpeg-devel] [PATCH] avcodec/bsf: Avoid allocation for AVBSFInternal

[James Almer]

On 8/10/2020 8:12 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 8/10/2020 7:11 PM, Nicolas George wrote:
>>> James Almer (12020-08-10):
>>>> Personally, i don't like it. It's extra complexity to save a single 8 or
>>>> 12 byte allocation that happens once during bsf alloc. It's kind of a
>>>> pointless micro-optimization.
>>>
>>> I do not agree at all.
>>>
>>> First, it is not extra complexity, it actually makes the code simpler:
>>> less mutually dependant allocations that can lead to leaks if they are
>>> not handled properly, better guarantees, for no more code.
>>
>> It adds an extra struct and makes the code harder to read. Might as well
>> just do
>>
>> ctx = av_mallocz(sizeof(*ctx) + sizeof(AVBSFInternal));
>> ctx->internal = &ctx[1];
> 
> This is not ok due to padding/alignment.

libdav1d would have broke by now if that was the case. Do you know a
system where this could fail?

> 
>>
>> if removing one tiny allocation in an incredibly cold function is so
>> important. Less code, same result.
> 
> I don't see an obfuscation/complication (and also no problem with
> maintainability); I see a very simple method to save an allocation. And
> I actually think that it simplifies the code, because now one doesn't
> have to worry at all any more whether internal has been properly
> allocated or not in av_bsf_free().

I don't deny it simplifies the code in regards to freeing the context in
failure paths, but adding a third struct does not make things clearer at
all. So Mark's suggestion to add AVBSFContext into AVBSFInternal may be
a better approach.

> Do you remember 31aafdac2404c5e01b21e53255db3fb5ed53c7c9
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/251816.html)
> where you fixed the bug that avformat_free_context() gets called upon
> failure to allocate the AVFormatInternal despite avformat_free_context()
> requiring the internal to be successfully allocated? That issue would
> have never even existed if one allocated the context and its internal in
> one piece.
> 
> - Andreas
> _______________________________________________
> 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".
>