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

[Frank Plowman]

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.

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