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

[James Almer]

On 8/10/2020 9:14 PM, Andreas Rheinhardt wrote:
> 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.

But is that the case here? AVBSFContext is 64 bytes on x86_64 (six 8
byte pointers, four ints, no padding), and AVBSFInternal is 16 (8 byte
pointer properly aligned after AVBSFContext, 4 byte int, 4 byte
padding). On 32 bit systems with 4 byte alignment for pointers, it would
also be fine.

In any case, I'm not suggesting to do this. I'd rather just not make any
change at all.

> 
> Could you elaborate the point about libdav1d?

It uses this same approach to insert a byte array at the end of a
context rather than a struct, so probably not the same situation.

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

Instruction encoding for bigger offsets (>=128 on x86) is not an
acceptable reason to choose one approach or another here. That is beyond
ricing. This is not a hot SIMD loop in a video DSP function where every
byte counts.

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