Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss

[James Almer <jamrial@gmail.com>]

On 1/29/2024 5:19 PM, Frank Plowman wrote:
> On 29/01/2024 19:04, James Almer wrote:
> 
>>
>> Well, turns out the current code is fine and my suggested change above 
>> is wrong. Fun how that goes.
>>
>> Can you test the following instead?
>>
>>> diff --git a/libavcodec/cbs_h266_syntax_template.c 
>>> b/libavcodec/cbs_h266_syntax_template.c
>>> index 549d021211..30b4ae3bc0 100644
>>> --- a/libavcodec/cbs_h266_syntax_template.c
>>> +++ b/libavcodec/cbs_h266_syntax_template.c
>>> @@ -764,7 +764,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, 
>>> RWContext *rw,
>>>              infer(vps_each_layer_is_an_ols_flag, 0);
>>>          if (!current->vps_each_layer_is_an_ols_flag) {
>>>              if (!current->vps_all_independent_layers_flag)
>>> -                ub(2, vps_ols_mode_idc);
>>> +                u(2, vps_ols_mode_idc, 0, 2);
>>>              else
>>>                  infer(vps_ols_mode_idc, 2);
>>>              if (current->vps_ols_mode_idc == 2) {
> The spec reads "Decoders conforming to this version of this 
> Specification shall *ignore* the OLSs with
> vps_ols_mode_idc equal to 3."  This change throws an error for these 
> OLSs, which I don't think is correct.
> There is already some logic just below this to warn the user if 
> vps_ols_mode_idc is 3.
>>> @@ -902,11 +902,10 @@ static int FUNC(vps) (CodedBitstreamContext 
>>> *ctx, RWContext *rw,
>>>                         current->vps_ols_mode_idc == 1) {
>>>                  num_layers_in_ols = i + 1;
>>>              } else if (current->vps_ols_mode_idc == 2) {
>>> -                for (k = 0, j = 0; k <= 
>>> current->vps_max_layers_minus1; k++) {
>>> +                for (k = 0, j = 0; k <= 
>>> current->vps_max_layers_minus1; k++)
>>>                      if (layer_included_in_ols_flag[i][k])
>>>                          j++;
>>> -                    num_layers_in_ols = j;
>>> -                }
>>> +                num_layers_in_ols = j;
>>>              }
>>>              if (num_layers_in_ols > 1) {
>>>                  num_multi_layer_olss++;
> 
> This looks good to me, the old behaviour was wrong.  I don't think this 
> is what was causing this
> particular crash however.

Will apply this part then.

> 
> Below is a patch which addresses the issue, an integer overflow when 
> calculating the bounds for
> vps_num_ols_timing_hrd_params_minus1.  There's also a similar fix for 
> vps_num_dpb_params_minus1.
> 
> diff --git a/libavcodec/cbs_h266_syntax_template.c 
> b/libavcodec/cbs_h266_syntax_template.c
> index 549d021211..49bf2e45ac 100644
> --- a/libavcodec/cbs_h266_syntax_template.c
> +++ b/libavcodec/cbs_h266_syntax_template.c
> @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, 
> RWContext *rw,
> 
>       if (!current->vps_each_layer_is_an_ols_flag) {
>           uint16_t vps_num_dpb_params;
> -        ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1);
> +        ue(vps_num_dpb_params_minus1, 0,
> +           num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0);

FFMAX(0, num_multi_layer_olss - 1); looks better.

If the spec explicitly states num_multi_layer_olss - 1 should be used 
here, wouldn't that mean that it doesn't expect num_multi_layer_olss to 
be 0 at this point for vps_ols_mode_idc >= 0 && vps_ols_mode_idc < 3?
When vps_each_layer_is_an_ols_flag is true, num_multi_layer_olss is 
inferred as 1, so I'd expect it to also be at least 1 for 
vps_each_layer_is_an_ols_flag == false.

>           if (current->vps_each_layer_is_an_ols_flag)
>               vps_num_dpb_params = 0;
>           else
> @@ -991,7 +992,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, 
> RWContext *rw,
>               else
>                   infer(vps_sublayer_cpb_params_present_flag, 0);
>               ue(vps_num_ols_timing_hrd_params_minus1, 0,
> -               num_multi_layer_olss - 1);
> +               num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0);
>               for (i = 0; i <= 
> current->vps_num_ols_timing_hrd_params_minus1; i++) {
>                   uint8_t first_sublayer;
>                   if (!current->vps_default_ptl_dpb_hrd_max_tid_flag)
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".