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

[zhilizhao]

> On Aug 11, 2020, at 5:48 PM, Nicolas George <george at nsup.org> wrote:
> 
> Andreas Rheinhardt (12020-08-11):
>> 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.
> 
> With this, you answered to James' question to me "What is undefined in
> that?".
> 
>> There are two reasons why I like my approach more than Mark's: It allows
> 
> I cannot find what you refer to "Mark's" approach.

Andreas referr to struct AVCodecHWConfigInternal with commit
24cc0a53e99.

> 
>> 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.
> 
> ... and therefore, I cannot understand exactly the point you are making
> here.
> 
>> 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.
> 
> In this, you are mistaken.
> 
> First, AFAIK, the increase in cost for accessing pointer+offset only
> happens at a few specific offsets, probably related to powers of two,
> which means the gain, minute as it is, only happens for the few fields
> that are pushed over the edge, if there are any.
> 
> But second, and most of all, you are neglecting a much more important
> optimization. If AVBSFContext is the first field of AVBSFInternal, then
> they both have the same address (it is explicitly specified in the spec,
> and it has been used for decades by Gtk+ for example), and therefore we
> can dispense with the ctx->internal pointer and just use it directly.
> 
> We can still have two pointers in the code, one for each type, to make
> it more compact and readable. But even if we do, the compiler will know
> they are the same.
> 
> Note that getting rid of the .internal field reduces the size of the
> structure, and is in itself another optimization.
> 
> And since we only have one pointer instead of two, it frees a register
> for other uses.
> 
> So this is an even better solution than your proposal, but it requires
> all uses of ->internal to be replaced. I I count correctly, that makes
> only four lines to change, and for a trivial change.
> 
> Note that all this can be done while keeping the specifics and the
> pointer magic hidden in a single file:
> 
>    static inline AVBSFInternal *bsf_internal(AVBSFContext *bsf) {
>        return (AVBSFInternal *)bsf;
>    }
> 
> Then the code only needs to write:
> 
>    AVBSFInternal *int = bsf_internal(bsf);
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".