[FFmpeg-devel] cbs_vp9: Ensure that reserved zero bits are actually zero

---
On 27/10/18 20:26, James Almer wrote:
> On 10/27/2018 4:13 PM, James Almer wrote:
>> On 10/27/2018 4:11 PM, Mark Thompson wrote:
>>> On 26/10/18 20:37, James Almer wrote:
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  libavcodec/cbs_vp9_syntax_template.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
>>>> index 0db0f52a6d..b4a7f65e85 100644
>>>> --- a/libavcodec/cbs_vp9_syntax_template.c
>>>> +++ b/libavcodec/cbs_vp9_syntax_template.c
>>>> @@ -65,6 +65,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>          if (profile == 1 || profile == 3) {
>>>>              infer(subsampling_x, 0);
>>>>              infer(subsampling_y, 0);
>>>> +            f(1, color_config_reserved_zero);
>>>>          }
>>>>      }
>>>>  
>>>>
>>>
>>> This and the one above should probably be called "reserved_zero" for trace purposes.  Not sure where the longer name came from.
>>>
>>
>> Yeah, the spec simply says reserved_zero, so i'll change it.
> 
> Looks like you came up with the longer name because there's another
> "reserved_zero" field in the uncompressed header, and of course you
> couldn't use it for both.
> 
> The solution would be to move the color config fields to its own struct
> called VP9RawColorConfig, like you did in cbs av1.

Or this, so we don't store them at all :)


 libavcodec/cbs_vp9.c                 | 13 +++++++++++++
 libavcodec/cbs_vp9.h                 |  2 --
 libavcodec/cbs_vp9_syntax_template.c |  6 +++---
 3 files changed, 16 insertions(+), 5 deletions(-)

On 10/27/2018 4:41 PM, Mark Thompson wrote:
> ---
> On 27/10/18 20:26, James Almer wrote:
>> On 10/27/2018 4:13 PM, James Almer wrote:
>>> On 10/27/2018 4:11 PM, Mark Thompson wrote:
>>>> On 26/10/18 20:37, James Almer wrote:
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>>  libavcodec/cbs_vp9_syntax_template.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
>>>>> index 0db0f52a6d..b4a7f65e85 100644
>>>>> --- a/libavcodec/cbs_vp9_syntax_template.c
>>>>> +++ b/libavcodec/cbs_vp9_syntax_template.c
>>>>> @@ -65,6 +65,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>>          if (profile == 1 || profile == 3) {
>>>>>              infer(subsampling_x, 0);
>>>>>              infer(subsampling_y, 0);
>>>>> +            f(1, color_config_reserved_zero);
>>>>>          }
>>>>>      }
>>>>>  
>>>>>
>>>>
>>>> This and the one above should probably be called "reserved_zero" for trace purposes.  Not sure where the longer name came from.
>>>>
>>>
>>> Yeah, the spec simply says reserved_zero, so i'll change it.
>>
>> Looks like you came up with the longer name because there's another
>> "reserved_zero" field in the uncompressed header, and of course you
>> couldn't use it for both.
>>
>> The solution would be to move the color config fields to its own struct
>> called VP9RawColorConfig, like you did in cbs av1.
> 
> Or this, so we don't store them at all :)
> 
> 
>  libavcodec/cbs_vp9.c                 | 13 +++++++++++++
>  libavcodec/cbs_vp9.h                 |  2 --
>  libavcodec/cbs_vp9_syntax_template.c |  6 +++---
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
> index 7498be4b73..c03ce986c0 100644
> --- a/libavcodec/cbs_vp9.c
> +++ b/libavcodec/cbs_vp9.c
> @@ -314,6 +314,12 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>          current->name = prob; \
>      } while (0)
>  
> +#define fixed(width, name, value) do { \
> +        av_unused uint32_t fixed_value = value; \
> +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
> +                                   0, &fixed_value, value, value)); \
> +    } while (0)
> +
>  #define infer(name, value) do { \
>          current->name = value; \
>      } while (0)
> @@ -331,6 +337,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  #undef fle
>  #undef delta_q
>  #undef prob
> +#undef fixed
>  #undef infer
>  #undef byte_alignment
>  
> @@ -370,6 +377,11 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>              xf(8, name.prob, current->name, subs, __VA_ARGS__); \
>      } while (0)
>  
> +#define fixed(width, name, value) do { \
> +        CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
> +                                    0, value, value, value)); \
> +    } while (0)
> +
>  #define infer(name, value) do { \
>          if (current->name != (value)) { \
>              av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
> @@ -392,6 +404,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  #undef fle
>  #undef delta_q
>  #undef prob
> +#undef fixed
>  #undef infer
>  #undef byte_alignment
>  
> diff --git a/libavcodec/cbs_vp9.h b/libavcodec/cbs_vp9.h
> index 7eee6d5e9e..ac66da5af1 100644
> --- a/libavcodec/cbs_vp9.h
> +++ b/libavcodec/cbs_vp9.h
> @@ -84,7 +84,6 @@ typedef struct VP9RawFrameHeader {
>      uint8_t frame_marker;
>      uint8_t profile_low_bit;
>      uint8_t profile_high_bit;
> -    uint8_t profile_reserved_zero;
>  
>      uint8_t show_existing_frame;
>      uint8_t frame_to_show_map_idx;
> @@ -99,7 +98,6 @@ typedef struct VP9RawFrameHeader {
>      uint8_t color_range;
>      uint8_t subsampling_x;
>      uint8_t subsampling_y;
> -    uint8_t color_config_reserved_zero;
>  
>      uint8_t refresh_frame_flags;
>  
> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
> index cd5b83a4f5..ec8463e6d7 100644
> --- a/libavcodec/cbs_vp9_syntax_template.c
> +++ b/libavcodec/cbs_vp9_syntax_template.c
> @@ -59,7 +59,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw,
>          if (profile == 1 || profile == 3) {
>              f(1, subsampling_x);
>              f(1, subsampling_y);
> -            f(1, color_config_reserved_zero);
> +            fixed(1, reserved_zero, 0);
>          } else {
>              infer(subsampling_x, 1);
>              infer(subsampling_y, 1);
> @@ -69,7 +69,7 @@ static int FUNC(color_config)(CodedBitstreamContext *ctx, RWContext *rw,
>          if (profile == 1 || profile == 3) {
>              infer(subsampling_x, 0);
>              infer(subsampling_y, 0);
> -            f(1, color_config_reserved_zero);
> +            fixed(1, reserved_zero, 0);
>          }
>      }
>  
> @@ -278,7 +278,7 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>      f(1, profile_high_bit);
>      profile = (current->profile_high_bit << 1) + current->profile_low_bit;
>      if (profile == 3)
> -        f(1, profile_reserved_zero);
> +        fixed(1, reserved_zero, 0);
>  
>      f(1, show_existing_frame);
>      if (current->show_existing_frame) {

Looks good, thanks!