* [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
@ 2024-02-01 14:00 post
2024-02-02 13:39 ` Nuo Mi
0 siblings, 1 reply; 7+ messages in thread
From: post @ 2024-02-01 14:00 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Frank Plowman
From: Frank Plowman <post@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@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;
}
static void pps_ref_wraparound_offset(VVCPPS *pps, const VVCSPS *sps)
--
2.43.0
_______________________________________________
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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-01 14:00 [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag post
@ 2024-02-02 13:39 ` Nuo Mi
2024-02-03 13:54 ` Frank Plowman
0 siblings, 1 reply; 7+ messages in thread
From: Nuo Mi @ 2024-02-02 13:39 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Frank Plowman
On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
> From: Frank Plowman <post@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@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.
>
> static void pps_ref_wraparound_offset(VVCPPS *pps, const VVCSPS *sps)
> --
> 2.43.0
>
> _______________________________________________
> 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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-02 13:39 ` Nuo Mi
@ 2024-02-03 13:54 ` Frank Plowman
2024-02-03 14:46 ` Nuo Mi
0 siblings, 1 reply; 7+ messages in thread
From: Frank Plowman @ 2024-02-03 13:54 UTC (permalink / raw)
To: ffmpeg-devel
On 02/02/2024 14:39, Nuo Mi wrote:
> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
>
>> From: Frank Plowman <post@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@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. I
could remove the return from pps_rect_slice and pps_no_rect_slice which
would get rid of a handful of changed lines but the changes are trivial
so it is not a big deal imo.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-03 13:54 ` Frank Plowman
@ 2024-02-03 14:46 ` Nuo Mi
2024-02-03 15:50 ` Frank Plowman
0 siblings, 1 reply; 7+ messages in thread
From: Nuo Mi @ 2024-02-03 14:46 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> wrote:
> On 02/02/2024 14:39, Nuo Mi wrote:
> > On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
> >
> >> From: Frank Plowman <post@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@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.
> I
> could remove the return from pps_rect_slice and pps_no_rect_slice which
> would get rid of a handful of changed lines but the changes are trivial
> so it is not a big deal imo.
>
Once we implemented subpic, both pps_rect_slice and pps_no_rect_slice are
not supposed to return a value. We need to change it back
> _______________________________________________
> 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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-03 14:46 ` Nuo Mi
@ 2024-02-03 15:50 ` Frank Plowman
2024-02-03 15:56 ` Nuo Mi
0 siblings, 1 reply; 7+ messages in thread
From: Frank Plowman @ 2024-02-03 15:50 UTC (permalink / raw)
To: ffmpeg-devel
On 03/02/2024 15:46, Nuo Mi wrote:
> On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com> wrote:
>
>> On 02/02/2024 14:39, Nuo Mi wrote:
>>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
>>>
>>>> From: Frank Plowman <post@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@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;
_______________________________________________
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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-03 15:50 ` Frank Plowman
@ 2024-02-03 15:56 ` Nuo Mi
2024-02-05 15:30 ` Frank Plowman
0 siblings, 1 reply; 7+ messages in thread
From: Nuo Mi @ 2024-02-03 15:56 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sat, Feb 3, 2024 at 11:51 PM Frank Plowman <post@frankplowman.com> wrote:
> On 03/02/2024 15:46, Nuo Mi wrote:
> > On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com>
> wrote:
> >
> >> On 02/02/2024 14:39, Nuo Mi wrote:
> >>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
> >>>
> >>>> From: Frank Plowman <post@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@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?
>
This fixes the single_slice_per_subpic_flag.
But fuzzer may find another subpic-related issue. Highly possible they will
crash too. :)
check sub picture number is a safer way
>
> 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;
> _______________________________________________
> 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".
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag
2024-02-03 15:56 ` Nuo Mi
@ 2024-02-05 15:30 ` Frank Plowman
0 siblings, 0 replies; 7+ messages in thread
From: Frank Plowman @ 2024-02-05 15:30 UTC (permalink / raw)
To: ffmpeg-devel
On 03/02/2024 16:56, Nuo Mi wrote:
> On Sat, Feb 3, 2024 at 11:51 PM Frank Plowman <post@frankplowman.com> wrote:
>
>> On 03/02/2024 15:46, Nuo Mi wrote:
>>> On Sat, Feb 3, 2024 at 9:54 PM Frank Plowman <post@frankplowman.com>
>> wrote:
>>>
>>>> On 02/02/2024 14:39, Nuo Mi wrote:
>>>>> On Thu, Feb 1, 2024 at 10:01 PM <post@frankplowman.com> wrote:
>>>>>
>>>>>> From: Frank Plowman <post@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@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?
>>
> This fixes the single_slice_per_subpic_flag.
> But fuzzer may find another subpic-related issue. Highly possible they will
> crash too. :)
> check sub picture number is a safer way
This issue can cause a crash even with the minimum
{s,p}ps_num_subpics_minus1 = 0 I believe, so this check is needed
regardless. We can add a PATCHWELCOME error if
{s,p}ps_num_subpics_minus1 > 0, but this should be a separate commit.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-05 15:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-01 14:00 [FFmpeg-devel] [PATCH] lavc/vvc: Error pps_single_slice_per_subpic_flag post
2024-02-02 13:39 ` Nuo Mi
2024-02-03 13:54 ` Frank Plowman
2024-02-03 14:46 ` Nuo Mi
2024-02-03 15:50 ` Frank Plowman
2024-02-03 15:56 ` Nuo Mi
2024-02-05 15:30 ` Frank Plowman
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git