[FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag

[Frank Plowman]

On 03/02/2024 15:46, Nuo Mi wrote:
> On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post at frankplowman.com> wrote:
> 
>> On 02/02/2024 14:39, Nuo Mi wrote:
>>> On Thu, Feb 1, 2024 at 10:01 PM <post at frankplowman.com> wrote:
>>>
>>>> From: Frank Plowman <post at frankplowman.com>
>>>>
>>>> pps_single_slice_per_subpic_flag is not yet supported.  Support is WIP,
>>>> but in the meantime throw an error when trying to decode a bitstream
>>>> with it set, avoiding an out-of-bounds array access.
>>>>
>>>> Fixes: out-of-bounds array access for conformance bitstreams
>>>> SUBPIC_C_ERICSSON_1, SUBPIC_D_ERICSSON_1, MNUT_A_Nokia_4 and
>>>> MNUT_B_Nokia_3.
>>>>
>>>> Signed-off-by: Frank Plowman <post at frankplowman.com>
>>>> ---
>>>>    libavcodec/vvc/vvc_ps.c | 21 ++++++++++++++++-----
>>>>    1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
>>>> index 2cf156b323..bd81d70e71 100644
>>>> --- a/libavcodec/vvc/vvc_ps.c
>>>> +++ b/libavcodec/vvc/vvc_ps.c
>>>> @@ -381,11 +381,16 @@ static void pps_multi_tiles_slice(VVCPPS *pps,
>> const
>>>> int tile_idx, const int i,
>>>>        }
>>>>    }
>>>>
>>>> -static void pps_rect_slice(VVCPPS* pps)
>>>> +static int pps_rect_slice(VVCPPS* pps)
>>>>    {
>>>>        const H266RawPPS* r = pps->r;
>>>>        int tile_idx = 0, off = 0;
>>>>
>>>> +    if (r->pps_single_slice_per_subpic_flag) {
>>>> +        avpriv_report_missing_feature(NULL,
>>>> "pps_single_slice_per_subpic_flag");
>>>> +        return AVERROR_PATCHWELCOME;
>>>> +    }
>>>> +
>>>>        for (int i = 0; i < r->pps_num_slices_in_pic_minus1 + 1; i++) {
>>>>            if (!r->pps_slice_width_in_tiles_minus1[i] &&
>>>>                !r->pps_slice_height_in_tiles_minus1[i]) {
>>>> @@ -396,9 +401,11 @@ static void pps_rect_slice(VVCPPS* pps)
>>>>            }
>>>>            tile_idx = next_tile_idx(tile_idx, i, r);
>>>>        }
>>>> +
>>>> +    return 0;
>>>>    }
>>>>
>>>> -static void pps_no_rect_slice(VVCPPS* pps)
>>>> +static int pps_no_rect_slice(VVCPPS* pps)
>>>>    {
>>>>        const H266RawPPS* r = pps->r;
>>>>        int ctu_x, ctu_y, off = 0;
>>>> @@ -409,20 +416,24 @@ static void pps_no_rect_slice(VVCPPS* pps)
>>>>                pps_add_ctus(pps, &off, ctu_x, ctu_y,
>>>> r->col_width_val[tile_x], r->row_height_val[tile_y]);
>>>>            }
>>>>        }
>>>> +
>>>> +    return 0;
>>>>    }
>>>>
>>>>    static int pps_slice_map(VVCPPS *pps)
>>>>    {
>>>> +    int ret;
>>>> +
>>>>        pps->ctb_addr_in_slice = av_calloc(pps->ctb_count,
>>>> sizeof(*pps->ctb_addr_in_slice));
>>>>        if (!pps->ctb_addr_in_slice)
>>>>            return AVERROR(ENOMEM);
>>>>
>>>>        if (pps->r->pps_rect_slice_flag)
>>>> -        pps_rect_slice(pps);
>>>> +        ret = pps_rect_slice(pps);
>>>>        else
>>>> -        pps_no_rect_slice(pps);
>>>> +        ret = pps_no_rect_slice(pps);
>>>>
>>>> -    return 0;
>>>> +    return ret;
>>>>    }
>>>>
>>> Thank you Frank. This changed  too much code.
>>> How about we only check the sps_num_subpics_minus1 in decode_sps.
>>
>> I wrote it like this so that the avpriv_report_missing_feature is where
>> the feature would need to be, helping readability and searchability.
> 
> We need to make changes to both the cbs and the decoder for subpic support.
> pps_slice_map is not the first place.

There is nothing strictly missing in the CBS, only the derivation of 
NumSlicesInSub needs to be moved which is quite subtle;  I think the 
putting the error in the parameter set parser is clearer.

How is the patch below as an alternative?

diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
index 2cf156b323..4ef8f9f9b9 100644
--- a/libavcodec/vvc/vvc_ps.c
+++ b/libavcodec/vvc/vvc_ps.c
@@ -413,13 +413,20 @@ static void pps_no_rect_slice(VVCPPS* pps)

  static int pps_slice_map(VVCPPS *pps)
  {
+    const H266RawPPS* r = pps->r;
+
      pps->ctb_addr_in_slice = av_calloc(pps->ctb_count, 
sizeof(*pps->ctb_addr_in_slice));
      if (!pps->ctb_addr_in_slice)
          return AVERROR(ENOMEM);

-    if (pps->r->pps_rect_slice_flag)
+    if (pps->r->pps_rect_slice_flag) {
+        if (r->pps_single_slice_per_subpic_flag) {
+            avpriv_report_missing_feature(NULL, 
"pps_single_slice_per_subpic_flag");
+            return AVERROR_PATCHWELCOME;
+        }
+
          pps_rect_slice(pps);
-    else
+    } else
          pps_no_rect_slice(pps);

      return 0;