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

2018-10-27 Thread James Almer 
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 
>>> ---
>>>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-10-27 Thread James Almer 
On 10/26/2018 6:56 PM, Hendrik Leppkes wrote:
> On Fri, Oct 26, 2018 at 9:37 PM James Almer  wrote:
>>
>> Signed-off-by: James Almer 
>> ---
>>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-10-27 Thread James Almer 
On 10/27/2018 4:11 PM, Mark Thompson wrote:
> On 26/10/18 20:37, James Almer wrote:
>> Signed-off-by: James Almer 
>> ---
>>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-10-27 Thread Mark Thompson 
On 26/10/18 20:37, James Almer wrote:
> Signed-off-by: James Almer 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2018-10-26 Thread Hendrik Leppkes 
On Fri, Oct 26, 2018 at 9:37 PM James Almer  wrote:
>
> Signed-off-by: James Almer 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel