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

[Andreas Rheinhardt]

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?
> 

Imagine the context to only contain elements that require a alignment of
4 and the internal structure to require an alignment of eight. Then it
is perfectly possible for &ctx[1] to only have an alignment of four and
not of eight as internal requires it.

Could you elaborate the point about libdav1d?

>>
>>>
>>> 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.
> 
There are two reasons why I like my approach more than Mark's: It allows
to hide the details of the context<->internal relationship to the place
where allocations happen (and if one asserted as Nicolas suggested
(av_assert2(ctx->internal == &((AVBSFCombined *)ctx)->internal), it
would also need to be visible for the function doing the freeing, but
even then it can still be confined to one file). After all, the internal
of several important other structures are in private headers to be used
by lots of code.
The second point is another micro-optimization: If one puts the
AVBSFContext at the beginning of AVBSFInternal, then the offsets of all
the fields currently existing in AVBSFInternal will increase and the
encoding of the "read address from pointer + offset" is typically
shorter if offset is smaller. This is not a problem here as the
structures here are small, but other structures are bigger. This could
of course be overcome by putting the AVBSFContext at the end of
AVBSFInternal; but somehow this feels unnatural, although I could get
used to it.

>> 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