[FFmpeg-devel,2/2] h264_metadata: Support overscan_appropriate_flag

Fixes #8041.
---
Requested in <https://trac.ffmpeg.org/ticket/8041>.


 doc/bitstream_filters.texi     |  4 ++++
 libavcodec/h264_metadata_bsf.c | 11 +++++++++++
 2 files changed, 15 insertions(+)

Mark Thompson:
> Fixes #8041.
> ---
> Requested in <https://trac.ffmpeg.org/ticket/8041>.
> 
> 
>  doc/bitstream_filters.texi     |  4 ++++
>  libavcodec/h264_metadata_bsf.c | 11 +++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
> index 023945e9be..50a1679fc7 100644
> --- a/doc/bitstream_filters.texi
> +++ b/doc/bitstream_filters.texi
> @@ -224,6 +224,10 @@ Insert or remove AUD NAL units in all access units of the stream.
>  @item sample_aspect_ratio
>  Set the sample aspect ratio of the stream in the VUI parameters.
>  
> +@item overscan_appropriate_flag
> +Set whether the stream is suitable for display using overscan
> +or not (see H.264 section E.2.1).
> +
>  @item video_format
>  @item video_full_range_flag
>  Set the video format in the stream (see H.264 section E.2.1 and
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 3684e6bf7f..5de74be9d6 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -57,6 +57,8 @@ typedef struct H264MetadataContext {
>  
>      AVRational sample_aspect_ratio;
>  
> +    int overscan_appropriate_flag;
> +
>      int video_format;
>      int video_full_range_flag;
>      int colour_primaries;
> @@ -129,6 +131,11 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>          } \
>      } while (0)
>  
> +    if (ctx->overscan_appropriate_flag >= 0) {
> +        SET_VUI_FIELD(overscan_appropriate_flag);

LGTM. But just to make sure: You are aware that the check contained in
SET_VUI_FIELD is redundant here?

> +        sps->vui.overscan_info_present_flag = 1;
> +    }
> +
>      if (ctx->video_format             >= 0 ||
>          ctx->video_full_range_flag    >= 0 ||
>          ctx->colour_primaries         >= 0 ||
> @@ -630,6 +637,10 @@ static const AVOption h264_metadata_options[] = {
>          OFFSET(sample_aspect_ratio), AV_OPT_TYPE_RATIONAL,
>          { .dbl = 0.0 }, 0, 65535, FLAGS },
>  
> +    { "overscan_appropriate_flag", "Set VUI overscan appropriate flag",
> +        OFFSET(overscan_appropriate_flag), AV_OPT_TYPE_INT,
> +        { .i64 = -1 }, -1, 1, FLAGS },
> +
>      { "video_format", "Set video format (table E-2)",
>          OFFSET(video_format), AV_OPT_TYPE_INT,
>          { .i64 = -1 }, -1, 7, FLAGS},
>

On 29/07/2019 00:40, Andreas Rheinhardt wrote:
> Mark Thompson:
>> Fixes #8041.
>> ---
>> Requested in <https://trac.ffmpeg.org/ticket/8041>.
>>
>>
>>  doc/bitstream_filters.texi     |  4 ++++
>>  libavcodec/h264_metadata_bsf.c | 11 +++++++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>> index 023945e9be..50a1679fc7 100644
>> --- a/doc/bitstream_filters.texi
>> +++ b/doc/bitstream_filters.texi
>> @@ -224,6 +224,10 @@ Insert or remove AUD NAL units in all access units of the stream.
>>  @item sample_aspect_ratio
>>  Set the sample aspect ratio of the stream in the VUI parameters.
>>  
>> +@item overscan_appropriate_flag
>> +Set whether the stream is suitable for display using overscan
>> +or not (see H.264 section E.2.1).
>> +
>>  @item video_format
>>  @item video_full_range_flag
>>  Set the video format in the stream (see H.264 section E.2.1 and
>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>> index 3684e6bf7f..5de74be9d6 100644
>> --- a/libavcodec/h264_metadata_bsf.c
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -57,6 +57,8 @@ typedef struct H264MetadataContext {
>>  
>>      AVRational sample_aspect_ratio;
>>  
>> +    int overscan_appropriate_flag;
>> +
>>      int video_format;
>>      int video_full_range_flag;
>>      int colour_primaries;
>> @@ -129,6 +131,11 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>>          } \
>>      } while (0)
>>  
>> +    if (ctx->overscan_appropriate_flag >= 0) {
>> +        SET_VUI_FIELD(overscan_appropriate_flag);
> 
> LGTM. But just to make sure: You are aware that the check contained in
> SET_VUI_FIELD is redundant here?

It seemed marginally more consistent to use the same logic as other places.  I'll change it if you have any stronger feelings on it than "I already wrote it this way"?

>> +        sps->vui.overscan_info_present_flag = 1;
>> +    }
>> +
>>      if (ctx->video_format             >= 0 ||
>>          ctx->video_full_range_flag    >= 0 ||
>>          ctx->colour_primaries         >= 0 ||
>> @@ -630,6 +637,10 @@ static const AVOption h264_metadata_options[] = {
>>          OFFSET(sample_aspect_ratio), AV_OPT_TYPE_RATIONAL,
>>          { .dbl = 0.0 }, 0, 65535, FLAGS },
>>  
>> +    { "overscan_appropriate_flag", "Set VUI overscan appropriate flag",
>> +        OFFSET(overscan_appropriate_flag), AV_OPT_TYPE_INT,
>> +        { .i64 = -1 }, -1, 1, FLAGS },
>> +
>>      { "video_format", "Set video format (table E-2)",
>>          OFFSET(video_format), AV_OPT_TYPE_INT,
>>          { .i64 = -1 }, -1, 7, FLAGS},
>>

Thanks,

- Mark

Mark Thompson:
> On 29/07/2019 00:40, Andreas Rheinhardt wrote:
>> Mark Thompson:
>>> Fixes #8041.
>>> ---
>>> Requested in <https://trac.ffmpeg.org/ticket/8041>.
>>>
>>>
>>>  doc/bitstream_filters.texi     |  4 ++++
>>>  libavcodec/h264_metadata_bsf.c | 11 +++++++++++
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>>> index 023945e9be..50a1679fc7 100644
>>> --- a/doc/bitstream_filters.texi
>>> +++ b/doc/bitstream_filters.texi
>>> @@ -224,6 +224,10 @@ Insert or remove AUD NAL units in all access units of the stream.
>>>  @item sample_aspect_ratio
>>>  Set the sample aspect ratio of the stream in the VUI parameters.
>>>  
>>> +@item overscan_appropriate_flag
>>> +Set whether the stream is suitable for display using overscan
>>> +or not (see H.264 section E.2.1).
>>> +
>>>  @item video_format
>>>  @item video_full_range_flag
>>>  Set the video format in the stream (see H.264 section E.2.1 and
>>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>>> index 3684e6bf7f..5de74be9d6 100644
>>> --- a/libavcodec/h264_metadata_bsf.c
>>> +++ b/libavcodec/h264_metadata_bsf.c
>>> @@ -57,6 +57,8 @@ typedef struct H264MetadataContext {
>>>  
>>>      AVRational sample_aspect_ratio;
>>>  
>>> +    int overscan_appropriate_flag;
>>> +
>>>      int video_format;
>>>      int video_full_range_flag;
>>>      int colour_primaries;
>>> @@ -129,6 +131,11 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>>>          } \
>>>      } while (0)
>>>  
>>> +    if (ctx->overscan_appropriate_flag >= 0) {
>>> +        SET_VUI_FIELD(overscan_appropriate_flag);
>>
>> LGTM. But just to make sure: You are aware that the check contained in
>> SET_VUI_FIELD is redundant here?
> 
> It seemed marginally more consistent to use the same logic as other places.  I'll change it if you have any stronger feelings on it than "I already wrote it this way"?

I don't care. The compiler will optimize it away anyway.

- Andreas