| Message ID | 20240311185331.84556-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: AVERROR_PATCHWELCOME for subpictures | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On Mon, Mar 11, 2024 at 06:53:31PM +0000, Frank Plowman wrote: > VVC's subpictures feature is not yet implemented in the native decoder. > Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream using > the feature. Fixes crashes when trying to decode bitstreams which > use the feature. > > Signed-off-by: Frank Plowman <post@frankplowman.com> > --- > libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) This breaks fate-vvc-conformance-SUBPIC_A_3 make fate-vvc-conformance-SUBPIC_A_3 TEST vvc-conformance-SUBPIC_A_3 --- ./tests/ref/fate/vvc-conformance-SUBPIC_A_3 2024-03-05 02:37:36.235300141 +0100 +++ tests/data/fate/vvc-conformance-SUBPIC_A_3 2024-03-12 01:47:27.301593567 +0100 @@ -1,9 +0,0 @@ -#tb 0: 1/25 -#media_type 0: video -#codec_id 0: rawvideo -#dimensions 0: 1920x1080 -#sar 0: 0/1 -0, 0, 0, 1, 6220800, 0xa419cfb6 -0, 1, 1, 1, 6220800, 0xa419cfb6 -0, 2, 2, 1, 6220800, 0xa419cfb6 -0, 3, 3, 1, 6220800, 0xa419cfb6 Test vvc-conformance-SUBPIC_A_3 failed. Look at tests/data/fate/vvc-conformance-SUBPIC_A_3.err for details. tests/Makefile:318: recipe for target 'fate-vvc-conformance-SUBPIC_A_3' failed make: *** [fate-vvc-conformance-SUBPIC_A_3] Error 69 thx [...]
On 3/11/2024 9:49 PM, Michael Niedermayer wrote: > On Mon, Mar 11, 2024 at 06:53:31PM +0000, Frank Plowman wrote: >> VVC's subpictures feature is not yet implemented in the native decoder. >> Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream using >> the feature. Fixes crashes when trying to decode bitstreams which >> use the feature. >> >> Signed-off-by: Frank Plowman <post@frankplowman.com> >> --- >> libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) > > This breaks fate-vvc-conformance-SUBPIC_A_3 > > make fate-vvc-conformance-SUBPIC_A_3 > TEST vvc-conformance-SUBPIC_A_3 > --- ./tests/ref/fate/vvc-conformance-SUBPIC_A_3 2024-03-05 02:37:36.235300141 +0100 > +++ tests/data/fate/vvc-conformance-SUBPIC_A_3 2024-03-12 01:47:27.301593567 +0100 > @@ -1,9 +0,0 @@ > -#tb 0: 1/25 > -#media_type 0: video > -#codec_id 0: rawvideo > -#dimensions 0: 1920x1080 > -#sar 0: 0/1 > -0, 0, 0, 1, 6220800, 0xa419cfb6 > -0, 1, 1, 1, 6220800, 0xa419cfb6 > -0, 2, 2, 1, 6220800, 0xa419cfb6 > -0, 3, 3, 1, 6220800, 0xa419cfb6 > Test vvc-conformance-SUBPIC_A_3 failed. Look at tests/data/fate/vvc-conformance-SUBPIC_A_3.err for details. > tests/Makefile:318: recipe for target 'fate-vvc-conformance-SUBPIC_A_3' failed > make: *** [fate-vvc-conformance-SUBPIC_A_3] Error 69 > > thx The sample appears to decode fine and doesn't crash, although all four frames are exactly the same (I don't know is that's intended). Maybe the crashes can be fixed in some other form? And abort only if FF_COMPLIANCE_STRICT is requested.
On Mon, 2024-03-11 at 21:57 -0300, James Almer wrote: > On 3/11/2024 9:49 PM, Michael Niedermayer wrote: > > On Mon, Mar 11, 2024 at 06:53:31PM +0000, Frank Plowman wrote: > > > VVC's subpictures feature is not yet implemented in the native > > > decoder. > > > Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream > > > using > > > the feature. Fixes crashes when trying to decode bitstreams > > > which > > > use the feature. > > > > > > Signed-off-by: Frank Plowman <post@frankplowman.com> > > > --- > > > libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > This breaks fate-vvc-conformance-SUBPIC_A_3 > > > > make fate-vvc-conformance-SUBPIC_A_3 > > TEST vvc-conformance-SUBPIC_A_3 > > --- ./tests/ref/fate/vvc-conformance-SUBPIC_A_3 2024-03-05 > > 02:37:36.235300141 +0100 > > +++ tests/data/fate/vvc-conformance-SUBPIC_A_3 2024-03-12 > > 01:47:27.301593567 +0100 > > @@ -1,9 +0,0 @@ > > -#tb 0: 1/25 > > -#media_type 0: video > > -#codec_id 0: rawvideo > > -#dimensions 0: 1920x1080 > > -#sar 0: 0/1 > > -0, 0, 0, 1, 6220800, 0xa419cfb6 > > -0, 1, 1, 1, 6220800, 0xa419cfb6 > > -0, 2, 2, 1, 6220800, 0xa419cfb6 > > -0, 3, 3, 1, 6220800, 0xa419cfb6 > > Test vvc-conformance-SUBPIC_A_3 failed. Look at > > tests/data/fate/vvc-conformance-SUBPIC_A_3.err for details. > > tests/Makefile:318: recipe for target 'fate-vvc-conformance- > > SUBPIC_A_3' failed > > make: *** [fate-vvc-conformance-SUBPIC_A_3] Error 69 > > > > thx > > The sample appears to decode fine and doesn't crash, although all > four > frames are exactly the same (I don't know is that's intended). The result is correct. Assume native decode can handle subpic, at least a part of the feature. > Maybe the crashes can be fixed in some other form? And abort only if > FF_COMPLIANCE_STRICT is requested. Previous I made a patch to fix the crash in setup ps, but still get error in decoding slice, it's better to return AVERROR_PATCHWELCOME only in case of pps_single_slice_per_subpic_flag. https://github.com/intel-media-ci/ffmpeg/pull/723 Thanks Fei > _______________________________________________ > 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".
On 12/03/2024 03:33, Wang, Fei W wrote: > On Mon, 2024-03-11 at 21:57 -0300, James Almer wrote: >> On 3/11/2024 9:49 PM, Michael Niedermayer wrote: >>> On Mon, Mar 11, 2024 at 06:53:31PM +0000, Frank Plowman wrote: >>>> VVC's subpictures feature is not yet implemented in the native >>>> decoder. >>>> Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream >>>> using >>>> the feature. Fixes crashes when trying to decode bitstreams >>>> which >>>> use the feature. >>>> >>>> Signed-off-by: Frank Plowman <post@frankplowman.com> >>>> --- >>>> libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>> >>> This breaks fate-vvc-conformance-SUBPIC_A_3 >>> >>> make fate-vvc-conformance-SUBPIC_A_3 >>> TEST vvc-conformance-SUBPIC_A_3 >>> --- ./tests/ref/fate/vvc-conformance-SUBPIC_A_3 2024-03-05 >>> 02:37:36.235300141 +0100 >>> +++ tests/data/fate/vvc-conformance-SUBPIC_A_3 2024-03-12 >>> 01:47:27.301593567 +0100 >>> @@ -1,9 +0,0 @@ >>> -#tb 0: 1/25 >>> -#media_type 0: video >>> -#codec_id 0: rawvideo >>> -#dimensions 0: 1920x1080 >>> -#sar 0: 0/1 >>> -0, 0, 0, 1, 6220800, 0xa419cfb6 >>> -0, 1, 1, 1, 6220800, 0xa419cfb6 >>> -0, 2, 2, 1, 6220800, 0xa419cfb6 >>> -0, 3, 3, 1, 6220800, 0xa419cfb6 >>> Test vvc-conformance-SUBPIC_A_3 failed. Look at >>> tests/data/fate/vvc-conformance-SUBPIC_A_3.err for details. >>> tests/Makefile:318: recipe for target 'fate-vvc-conformance- >>> SUBPIC_A_3' failed >>> make: *** [fate-vvc-conformance-SUBPIC_A_3] Error 69 >>> >>> thx >> >> The sample appears to decode fine and doesn't crash, although all >> four >> frames are exactly the same (I don't know is that's intended). > > The result is correct. Assume native decode can handle subpic, at least > a part of the feature. > >> Maybe the crashes can be fixed in some other form? And abort only if >> FF_COMPLIANCE_STRICT is requested. > > Previous I made a patch to fix the crash in setup ps, but still get > error in decoding slice, it's better to return AVERROR_PATCHWELCOME > only in case of pps_single_slice_per_subpic_flag. > > https://github.com/intel-media-ci/ffmpeg/pull/723 > > Thanks > Fei Hi, Thanks all for your reviews. Yes, the feature is most problematic if pps_single_slice_per_subpic_flag is 1. This is because there is some unimplemented logic in the parameter set parser in this case which leads to some out-of-bounds accesses. Subpictures will also fail to decode bitexact if sps_independent_subpics is 1, although in practice the distortion this introduces is fairly subtle. I think we are able to decode other subpicture bitstreams bitexact. SUBPIC_A_HUAWEI_3 falls into this last category. I posted a patch, like yours Fei, which errors only the most problematic pps_single_slice_per_subpic_flag bitstreams some time ago: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240201140055.63805-1-post@frankplowman.com/ and the feedback I received there was to make this patch. I have also implemented the logic needed for pps_single_slice_per_subpic_flag here: https://github.com/ffvvc/FFmpeg/pull/191. I would be happy to make a v2 of the pps_single_slice_per_subpic_flag patch which moves where the error is being returned, or to put the GitHub PR on the ML if people would rather one of those two alternatives. Thanks again,
On Tue, Mar 12, 2024 at 5:09 PM Frank Plowman <post@frankplowman.com> wrote: > On 12/03/2024 03:33, Wang, Fei W wrote: > > On Mon, 2024-03-11 at 21:57 -0300, James Almer wrote: > >> On 3/11/2024 9:49 PM, Michael Niedermayer wrote: > >>> On Mon, Mar 11, 2024 at 06:53:31PM +0000, Frank Plowman wrote: > >>>> VVC's subpictures feature is not yet implemented in the native > >>>> decoder. > >>>> Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream > >>>> using > >>>> the feature. Fixes crashes when trying to decode bitstreams > >>>> which > >>>> use the feature. > >>>> > >>>> Signed-off-by: Frank Plowman <post@frankplowman.com> > >>>> --- > >>>> libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ > >>>> 1 file changed, 15 insertions(+) > >>> > >>> This breaks fate-vvc-conformance-SUBPIC_A_3 > >>> > >>> make fate-vvc-conformance-SUBPIC_A_3 > >>> TEST vvc-conformance-SUBPIC_A_3 > >>> --- ./tests/ref/fate/vvc-conformance-SUBPIC_A_3 2024-03-05 > >>> 02:37:36.235300141 +0100 > >>> +++ tests/data/fate/vvc-conformance-SUBPIC_A_3 2024-03-12 > >>> 01:47:27.301593567 +0100 > >>> @@ -1,9 +0,0 @@ > >>> -#tb 0: 1/25 > >>> -#media_type 0: video > >>> -#codec_id 0: rawvideo > >>> -#dimensions 0: 1920x1080 > >>> -#sar 0: 0/1 > >>> -0, 0, 0, 1, 6220800, 0xa419cfb6 > >>> -0, 1, 1, 1, 6220800, 0xa419cfb6 > >>> -0, 2, 2, 1, 6220800, 0xa419cfb6 > >>> -0, 3, 3, 1, 6220800, 0xa419cfb6 > >>> Test vvc-conformance-SUBPIC_A_3 failed. Look at > >>> tests/data/fate/vvc-conformance-SUBPIC_A_3.err for details. > >>> tests/Makefile:318: recipe for target 'fate-vvc-conformance- > >>> SUBPIC_A_3' failed > >>> make: *** [fate-vvc-conformance-SUBPIC_A_3] Error 69 > >>> > >>> thx > >> > >> The sample appears to decode fine and doesn't crash, although all > >> four > >> frames are exactly the same (I don't know is that's intended). > > > > The result is correct. Assume native decode can handle subpic, at least > > a part of the feature. > > > >> Maybe the crashes can be fixed in some other form? And abort only if > >> FF_COMPLIANCE_STRICT is requested. > > > > Previous I made a patch to fix the crash in setup ps, but still get > > error in decoding slice, it's better to return AVERROR_PATCHWELCOME > > only in case of pps_single_slice_per_subpic_flag. > > > > https://github.com/intel-media-ci/ffmpeg/pull/723 > > > > Thanks > > Fei > > Hi, > > Thanks all for your reviews. > > Yes, the feature is most problematic if pps_single_slice_per_subpic_flag > is 1. This is because there is some unimplemented logic in the > parameter set parser in this case which leads to some out-of-bounds > accesses. Subpictures will also fail to decode bitexact if > sps_independent_subpics is 1, although in practice the distortion this > introduces is fairly subtle. I think we are able to decode other > subpicture bitstreams bitexact. SUBPIC_A_HUAWEI_3 falls into this last > category. > > I posted a patch, like yours Fei, which errors only the most problematic > pps_single_slice_per_subpic_flag bitstreams some time ago: > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240201140055.63805-1-post@frankplowman.com/ > and the feedback I received there was to make this patch. I have also > implemented the logic needed for pps_single_slice_per_subpic_flag here: > https://github.com/ffvvc/FFmpeg/pull/191. I would be happy to make a v2 > of the pps_single_slice_per_subpic_flag patch which moves where the > error is being returned, or to put the GitHub PR on the ML if people > would rather one of those two alternatives. > Hi Frank and all, Thank you for the patch and review. I have a local patch for subpic, hope I can make all clips passed and send out patches in the following 1~2 weeks. > > Thanks again, > -- > Frank > _______________________________________________ > 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". >
diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c index e6e46d2039..62515a6343 100644 --- a/libavcodec/vvc/vvc_ps.c +++ b/libavcodec/vvc/vvc_ps.c @@ -72,6 +72,18 @@ static int sps_map_pixel_format(VVCSPS *sps, void *log_ctx) return 0; } +static int sps_subpic_info(VVCSPS *sps, void *log_ctx) +{ + const H266RawSPS *r = sps->r; + + if (r->sps_num_subpics_minus1 > 0) { + avpriv_report_missing_feature(log_ctx, "Subpictures"); + return AVERROR_PATCHWELCOME; + } + + return 0; +} + static int sps_bit_depth(VVCSPS *sps, void *log_ctx) { const H266RawSPS *r = sps->r; @@ -177,6 +189,9 @@ static int sps_derive(VVCSPS *sps, void *log_ctx) int ret; const H266RawSPS *r = sps->r; + ret = sps_subpic_info(sps, log_ctx); + if (ret < 0) + return ret; ret = sps_bit_depth(sps, log_ctx); if (ret < 0) return ret;
VVC's subpictures feature is not yet implemented in the native decoder. Throw an AVERROR_PATCHWELCOME when trying to decode a bitstream using the feature. Fixes crashes when trying to decode bitstreams which use the feature. Signed-off-by: Frank Plowman <post@frankplowman.com> --- libavcodec/vvc/vvc_ps.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)