[FFmpeg-devel,6/8] h264_metadata: Remove unused fields

The SEI NAL is unused since 69062d0f9b6aef5d9d9b8c9c9b5cfb23037caddb,
while the AUD NAL is small and would more sensibly be on the stack.
---
 libavcodec/h264_metadata_bsf.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

On 3/11/2018 3:30 PM, Mark Thompson wrote:
> The SEI NAL is unused since 69062d0f9b6aef5d9d9b8c9c9b5cfb23037caddb,
> while the AUD NAL is small and would more sensibly be on the stack.
> ---
>  libavcodec/h264_metadata_bsf.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index d340c55990..760fe99c41 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -47,9 +47,6 @@ typedef struct H264MetadataContext {
>  
>      int done_first_au;
>  
> -    H264RawAUD aud_nal;
> -    H264RawSEI sei_nal;
> -
>      int aud;
>  
>      AVRational sample_aspect_ratio;
> @@ -263,7 +260,9 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out)
>                  0x3ff, // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
>              };
>              int primary_pic_type_mask = 0xff;
> -            H264RawAUD *aud = &ctx->aud_nal;
> +            H264RawAUD aud = {
> +                .nal_unit_header.nal_unit_type = H264_NAL_AUD,
> +            };

Afaik every other field is not zero initialized if you do this, unlike
if you keep it in H264MetadataContext.
Not sure if that may have some consequences or not here.

>  
>              for (i = 0; i < au->nb_units; i++) {
>                  if (au->units[i].type == H264_NAL_SLICE ||
> @@ -286,11 +285,10 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out)
>                  goto fail;
>              }
>  
> -            aud->nal_unit_header.nal_unit_type = H264_NAL_AUD;
> -            aud->primary_pic_type = j;
> +            aud.primary_pic_type = j;
>  
>              err = ff_cbs_insert_unit_content(ctx->cbc, au,
> -                                             0, H264_NAL_AUD, aud, NULL);
> +                                             0, H264_NAL_AUD, &aud, NULL);
>              if (err < 0) {
>                  av_log(bsf, AV_LOG_ERROR, "Failed to insert AUD.\n");
>                  goto fail;
>

On Sun, Mar 11, 2018 at 7:55 PM, James Almer <jamrial@gmail.com> wrote:
> On 3/11/2018 3:30 PM, Mark Thompson wrote:
>> The SEI NAL is unused since 69062d0f9b6aef5d9d9b8c9c9b5cfb23037caddb,
>> while the AUD NAL is small and would more sensibly be on the stack.
>> ---
>>  libavcodec/h264_metadata_bsf.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>> index d340c55990..760fe99c41 100644
>> --- a/libavcodec/h264_metadata_bsf.c
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -47,9 +47,6 @@ typedef struct H264MetadataContext {
>>
>>      int done_first_au;
>>
>> -    H264RawAUD aud_nal;
>> -    H264RawSEI sei_nal;
>> -
>>      int aud;
>>
>>      AVRational sample_aspect_ratio;
>> @@ -263,7 +260,9 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out)
>>                  0x3ff, // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
>>              };
>>              int primary_pic_type_mask = 0xff;
>> -            H264RawAUD *aud = &ctx->aud_nal;
>> +            H264RawAUD aud = {
>> +                .nal_unit_header.nal_unit_type = H264_NAL_AUD,
>> +            };
>
> Afaik every other field is not zero initialized if you do this, unlike
> if you keep it in H264MetadataContext.
> Not sure if that may have some consequences or not here.
>

All other members are initialized with zero if you use any sort of
initializer syntax.

- Hendrik

On 11/03/18 18:55, James Almer wrote:
> On 3/11/2018 3:30 PM, Mark Thompson wrote:
>> The SEI NAL is unused since 69062d0f9b6aef5d9d9b8c9c9b5cfb23037caddb,
>> while the AUD NAL is small and would more sensibly be on the stack.
>> ---
>>  libavcodec/h264_metadata_bsf.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>> index d340c55990..760fe99c41 100644
>> --- a/libavcodec/h264_metadata_bsf.c
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -47,9 +47,6 @@ typedef struct H264MetadataContext {
>>  
>>      int done_first_au;
>>  
>> -    H264RawAUD aud_nal;
>> -    H264RawSEI sei_nal;
>> -
>>      int aud;
>>  
>>      AVRational sample_aspect_ratio;
>> @@ -263,7 +260,9 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out)
>>                  0x3ff, // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
>>              };
>>              int primary_pic_type_mask = 0xff;
>> -            H264RawAUD *aud = &ctx->aud_nal;
>> +            H264RawAUD aud = {
>> +                .nal_unit_header.nal_unit_type = H264_NAL_AUD,
>> +            };
> 
> Afaik every other field is not zero initialized if you do this, unlike
> if you keep it in H264MetadataContext.
> Not sure if that may have some consequences or not here.

As long as one member is set the rest are are zero-initialised.

C11 §6.7.9, paragraph 19:
"all subobjects that are not initialized explicitly shall be initialized implicitly the same as
objects that have static storage duration."

(This is why the "{ 0 }" syntax works, because it initialises the first member to zero (whatever that is) and then the rest are implicitly zero as well.)

- Mark

On 3/11/2018 4:02 PM, Mark Thompson wrote:
> On 11/03/18 18:55, James Almer wrote:
>> On 3/11/2018 3:30 PM, Mark Thompson wrote:
>>> The SEI NAL is unused since 69062d0f9b6aef5d9d9b8c9c9b5cfb23037caddb,
>>> while the AUD NAL is small and would more sensibly be on the stack.
>>> ---
>>>  libavcodec/h264_metadata_bsf.c | 12 +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>>> index d340c55990..760fe99c41 100644
>>> --- a/libavcodec/h264_metadata_bsf.c
>>> +++ b/libavcodec/h264_metadata_bsf.c
>>> @@ -47,9 +47,6 @@ typedef struct H264MetadataContext {
>>>  
>>>      int done_first_au;
>>>  
>>> -    H264RawAUD aud_nal;
>>> -    H264RawSEI sei_nal;
>>> -
>>>      int aud;
>>>  
>>>      AVRational sample_aspect_ratio;
>>> @@ -263,7 +260,9 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out)
>>>                  0x3ff, // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
>>>              };
>>>              int primary_pic_type_mask = 0xff;
>>> -            H264RawAUD *aud = &ctx->aud_nal;
>>> +            H264RawAUD aud = {
>>> +                .nal_unit_header.nal_unit_type = H264_NAL_AUD,
>>> +            };
>>
>> Afaik every other field is not zero initialized if you do this, unlike
>> if you keep it in H264MetadataContext.
>> Not sure if that may have some consequences or not here.
> 
> As long as one member is set the rest are are zero-initialised.
> 
> C11 §6.7.9, paragraph 19:
> "all subobjects that are not initialized explicitly shall be initialized implicitly the same as
> objects that have static storage duration."
> 
> (This is why the "{ 0 }" syntax works, because it initialises the first member to zero (whatever that is) and then the rest are implicitly zero as well.)
> 
> - Mark

Good to know, thanks.