[FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
James Almer
jamrial at gmail.com
Mon Jan 29 23:13:44 EET 2024
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list