diff mbox

[FFmpeg-devel,1/2] avcodec/cbs_vp9: fix parsing sRGB samples

Message ID 20181026193708.8056-1-jamrial@gmail.com
State Accepted
Commit a5d98da4d6f3d0cd89d61af43e6377a9c9dd4cb2
Headers show

Commit Message

James Almer Oct. 26, 2018, 7:37 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_vp9_syntax_template.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Hendrik Leppkes Oct. 26, 2018, 9:56 p.m. UTC | #1 
On Fri, Oct 26, 2018 at 9:37 PM James Almer <jamrial@gmail.com> 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);
>          }
>      }
>

What an odd quirk in the bitstream, the entire RGB branch has no coded
values but that single reserved one?
LGTM.

- Hendrik
Mark Thompson Oct. 27, 2018, 7:11 p.m. UTC | #2 
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.

LGTM in any case.

Thanks,

- Mark
James Almer Oct. 27, 2018, 7:13 p.m. UTC | #3 
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.

> LGTM in any case.
> 
> Thanks,
> 
> - Mark

Pushed, thanks.
James Almer Oct. 27, 2018, 7:14 p.m. UTC | #4 
On 10/26/2018 6:56 PM, Hendrik Leppkes wrote:
> On Fri, Oct 26, 2018 at 9:37 PM James Almer <jamrial@gmail.com> 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);
>>          }
>>      }
>>
> 
> What an odd quirk in the bitstream, the entire RGB branch has no coded
> values but that single reserved one?
> LGTM.
> 
> - Hendrik

The RGB branch is weird in AV1 as well :p

Pushed, thanks.
James Almer Oct. 27, 2018, 7:26 p.m. UTC | #5 
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.
diff mbox

Patch

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);
         }
     }