[FFmpeg-devel] [PATCH] h264: remove clear_blocks call in threading init.

[Ronald S. Bultje]

Hi,

On Tue, Feb 12, 2013 at 9:05 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Feb 11, 2013 at 06:08:45PM -0800, Ronald S. Bultje wrote:
>> From: "Ronald S. Bultje" <rsbultje at gmail.com>
>>
>> Init code in that if statement goes down from 26716 cycles to 26047
>> cycles, i.e. the removal of the clear_blocks and smaller memcpy()
>> together save around 670 cycles.
>> ---
>>  libavcodec/h264.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/h264.c b/libavcodec/h264.c
>> index 2d6c5e6..994e002 100644
>> --- a/libavcodec/h264.c
>> +++ b/libavcodec/h264.c
>> @@ -1253,7 +1253,9 @@ static int decode_update_thread_context(AVCodecContext *dst,
>>
>>          // copy all fields after MpegEnc
>>          memcpy(&h->s + 1, &h1->s + 1,
>> -               sizeof(H264Context) - sizeof(MpegEncContext));
>> +               offsetof(H264Context, intra_gb) - sizeof(MpegEncContext));
>> +        memcpy(&h->cabac, &h1->cabac,
>> +               sizeof(H264Context) - offsetof(H264Context, cabac));
>>          memset(h->sps_buffers, 0, sizeof(h->sps_buffers));
>>          memset(h->pps_buffers, 0, sizeof(h->pps_buffers));
>>
>
> If either of these fields is moved in the structure or a new field
> added between then threading can mysteriously break.
> also the comment above would not match the code anymore
> iam not sure this is a good idea but if it is done then the fields
> should be documented in the struct so a developer touching them would
> know they are special ...

Isn't that the case already? The non-init code in
update_thread_context() for mpeg and h264 100% depends on ordering of
the fields, any field ordering change will break the threading.

(I'm willing to document it, I'm just saying that I don't think I'm
introducing a new problem that didn't already exist before, and that I
don't really consider a problem anyway.)

Ronald