diff mbox series

[FFmpeg-devel,v4] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode

Message ID 1584456838-7151-1-git-send-email-linjie.fu@intel.com
State Accepted
Commit 9473268cfb580dccd3f1f3be338cd22661ef791e
Headers show
Series [FFmpeg-devel,v4] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Fu, Linjie March 17, 2020, 2:53 p.m. UTC 
With the description in frame size with refs semantics (SPEC 7.2.5),
it is a requirement of bitstream conformance that for at least one
reference frame has the valid dimensions.

Modify the check to make sure the decoder works well in SINGLE_REFERENCE
mode that not all reference frames have valid dimensions.

Check and error out if invalid reference frame is used in inter_recon.

One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
with following reference pool:

0.  960x544    LAST    valid
1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
2. 1920x1088 ALTREF  invalid, but not used in single reference mode
3~7  ...     Unused

Identical logic in libvpx:
<https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_decodeframe.c#L736>

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
[v3]: replace assert with check/return, tested in both multi frame/slice mode
[v4]: clear error_info to make decoding still work for other frames in this stream

 libavcodec/vp9.c      | 20 ++++++++++++++++++--
 libavcodec/vp9dec.h   |  5 +++++
 libavcodec/vp9recon.c | 10 ++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

Comments

Ronald S. Bultje March 19, 2020, 12:10 p.m. UTC | #1 
Hi,

On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu <linjie.fu@intel.com> wrote:

> With the description in frame size with refs semantics (SPEC 7.2.5),
> it is a requirement of bitstream conformance that for at least one
> reference frame has the valid dimensions.
>
> Modify the check to make sure the decoder works well in SINGLE_REFERENCE
> mode that not all reference frames have valid dimensions.
>
> Check and error out if invalid reference frame is used in inter_recon.
>
> One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
> with following reference pool:
>
> 0.  960x544    LAST    valid
> 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> 3~7  ...     Unused
>
> Identical logic in libvpx:
> <
> https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_decodeframe.c#L736
> >
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> [v3]: replace assert with check/return, tested in both multi frame/slice
> mode
> [v4]: clear error_info to make decoding still work for other frames in
> this stream
>
>  libavcodec/vp9.c      | 20 ++++++++++++++++++--
>  libavcodec/vp9dec.h   |  5 +++++
>  libavcodec/vp9recon.c | 10 ++++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)


LGTM, thanks for the revisions. (We have been discussing this on IRC.)

Ronald
Carl Eugen Hoyos March 19, 2020, 1:03 p.m. UTC | #2 
> Am 19.03.2020 um 13:10 schrieb Ronald S. Bultje <rsbultje@gmail.com>:
> 
> Hi,
> 
>> On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu <linjie.fu@intel.com> wrote:
>> 
>> With the description in frame size with refs semantics (SPEC 7.2.5),
>> it is a requirement of bitstream conformance that for at least one
>> reference frame has the valid dimensions.
>> 
>> Modify the check to make sure the decoder works well in SINGLE_REFERENCE
>> mode that not all reference frames have valid dimensions.
>> 
>> Check and error out if invalid reference frame is used in inter_recon.
>> 
>> One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
>> with following reference pool:
>> 
>> 0.  960x544    LAST    valid
>> 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
>> 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
>> 3~7  ...     Unused
>> 
>> Identical logic in libvpx:
>> <
>> https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_decodeframe.c#L736
>>> 
>> 
>> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
>> ---
>> [v3]: replace assert with check/return, tested in both multi frame/slice
>> mode
>> [v4]: clear error_info to make decoding still work for other frames in
>> this stream
>> 
>> libavcodec/vp9.c      | 20 ++++++++++++++++++--
>> libavcodec/vp9dec.h   |  5 +++++
>> libavcodec/vp9recon.c | 10 ++++++++++
>> 3 files changed, 33 insertions(+), 2 deletions(-)
> 
> 
> LGTM, thanks for the revisions. (We have been discussing this on IRC.)

Why does this error out even if explode is not set?

Carl Eugen
Fu, Linjie March 19, 2020, 1:45 p.m. UTC | #3 
Hi Carl,
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Carl Eugen Hoyos
> Sent: Thursday, March 19, 2020 21:04
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> 
> 
> > Am 19.03.2020 um 13:10 schrieb Ronald S. Bultje <rsbultje@gmail.com>:
> >
> > Hi,
> >
> >> On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu <linjie.fu@intel.com> wrote:
> >>
> >> With the description in frame size with refs semantics (SPEC 7.2.5),
> >> it is a requirement of bitstream conformance that for at least one
> >> reference frame has the valid dimensions.
> >>
> >> Modify the check to make sure the decoder works well in
> SINGLE_REFERENCE
> >> mode that not all reference frames have valid dimensions.
> >>
> >> Check and error out if invalid reference frame is used in inter_recon.
> >>
> >> One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE
> mode)
> >> with following reference pool:
> >>
> >> 0.  960x544    LAST    valid
> >> 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> >> 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> >> 3~7  ...     Unused
> >>
> >> Identical logic in libvpx:
> >> <
> >>
> https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_de
> codeframe.c#L736
> >>>
> >>
> >> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> >> ---
> >> [v3]: replace assert with check/return, tested in both multi frame/slice
> >> mode
> >> [v4]: clear error_info to make decoding still work for other frames in
> >> this stream
> >>
> >> libavcodec/vp9.c      | 20 ++++++++++++++++++--
> >> libavcodec/vp9dec.h   |  5 +++++
> >> libavcodec/vp9recon.c | 10 ++++++++++
> >> 3 files changed, 33 insertions(+), 2 deletions(-)
> >
> >
> > LGTM, thanks for the revisions. (We have been discussing this on IRC.)
> 
> Why does this error out even if explode is not set?

"explode" seems to mean that aborting the whole decode procedure on minor error detection.

This fix [v4] intended to report errors and exit decode_frame() for the exact frame witch has
invalid mvscale factors used, and didn't mean to break the decoding for subsequent frames if
they could be decoded correctly. 

It's not an aborting for the whole decoding hence I think this behavior may not match the description
of explode.

Thanks,

- Linjie
Carl Eugen Hoyos March 19, 2020, 2:13 p.m. UTC | #4 
Am Do., 19. März 2020 um 14:45 Uhr schrieb Fu, Linjie <linjie.fu@intel.com>:

> This fix [v4] intended to report errors and exit decode_frame() for the exact frame witch has
> invalid mvscale factors used, and didn't mean to break the decoding for subsequent frames if
> they could be decoded correctly.

How does the output look visually before this patch and how does it
look with this patch?

Carl Eugen
Fu, Linjie March 19, 2020, 3:12 p.m. UTC | #5 
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Carl Eugen Hoyos
> Sent: Thursday, March 19, 2020 22:13
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> Am Do., 19. März 2020 um 14:45 Uhr schrieb Fu, Linjie <linjie.fu@intel.com>:
> 
> > This fix [v4] intended to report errors and exit decode_frame() for the
> exact frame witch has
> > invalid mvscale factors used, and didn't mean to break the decoding for
> subsequent frames if
> > they could be decoded correctly.
> 
> How does the output look visually before this patch and how does it
> look with this patch?
> 
$ ffmpeg -i g2_decoder_case_15865.ivf -f null -

* 1. Before this patch:
Reported "Invalid ref frame dimensions" in the decode_frame_header() and abort the decoding.
(only 8 frame decode)

====
[vp9 @ 0x55e6496b3050] Invalid ref frame dimensions 1920x1088 for frame size 480x272
[vp9 @ 0x55e6496baab0] Not all references are available
[vp9 @ 0x55e6496c2510] Not all references are available
[vp9 @ 0x55e6496c5620] Not all references are available
Output #0, null, to 'pipe:':
  Metadata:
    encoder         : Lavf58.42.100
    Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200 kb/s, 25 fps, 25 tbn, 25 tbc
    Metadata:
      encoder         : Lavc58.76.100 wrapped_avframe
Error while decoding stream #0:0: Invalid data found when processing input
    Last message repeated 3 times
frame=    8 fps=7.7 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A speed=0.464x
====

* 2. With this patch:
All frames (12) decoded correctly (visually) with a warning in the header check.

====
Input #0, ivf, from 'g2_decoder_case_15865.ivf':
  Duration: 00:00:00.01, start: 0.000000, bitrate: 100442 kb/s
    Stream #0:0: Video: vp9 (Profile 0) (VP90 / 0x30395056), yuv420p(tv), 1920x1088, 25 fps, 25 tbr, 1k tbn, 1k tbc
Stream mapping:
  Stream #0:0 -> #0:0 (vp9 (native) -> wrapped_avframe (native))
Press [q] to stop, [?] for help
[vp9 @ 0x56072d8df080] Invalid ref frame dimensions 1920x1088 for frame size 480x272
    Last message repeated 1 times
Output #0, null, to 'pipe:':
  Metadata:
    encoder         : Lavf58.41.100
    Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200 kb/s, 25 fps, 25 tbn, 25 tbc
    Metadata:
      encoder         : Lavc58.75.100 wrapped_avframe
frame=   12 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A speed=1.77x
====

If talking about the error out, it didn't happen in this bitstream since the invalid refs is not used.

It would take effect in the situations that invalid ref is unexpected used in either single/comp reference during
inter recon. And identical messages would be reported that invalid data found when processing input, and the
outputted frames is less than expected. 

- Linjie
Carl Eugen Hoyos March 19, 2020, 4:47 p.m. UTC | #6 
Am Do., 19. März 2020 um 16:12 Uhr schrieb Fu, Linjie <linjie.fu@intel.com>:
>
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Carl Eugen Hoyos
> > Sent: Thursday, March 19, 2020 22:13
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> > dimensions check for SINGLE_REFERENCE mode
> >
> > Am Do., 19. März 2020 um 14:45 Uhr schrieb Fu, Linjie <linjie.fu@intel.com>:
> >
> > > This fix [v4] intended to report errors and exit decode_frame() for the
> > exact frame witch has
> > > invalid mvscale factors used, and didn't mean to break the decoding for
> > subsequent frames if
> > > they could be decoded correctly.
> >
> > How does the output look visually before this patch and how does it
> > look with this patch?
> >
> $ ffmpeg -i g2_decoder_case_15865.ivf -f null -
>
> * 1. Before this patch:
> Reported "Invalid ref frame dimensions" in the decode_frame_header() and abort the decoding.
> (only 8 frame decode)
>
> ====
> [vp9 @ 0x55e6496b3050] Invalid ref frame dimensions 1920x1088 for frame size 480x272
> [vp9 @ 0x55e6496baab0] Not all references are available
> [vp9 @ 0x55e6496c2510] Not all references are available
> [vp9 @ 0x55e6496c5620] Not all references are available
> Output #0, null, to 'pipe:':
>   Metadata:
>     encoder         : Lavf58.42.100
>     Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200 kb/s, 25 fps, 25 tbn, 25 tbc
>     Metadata:
>       encoder         : Lavc58.76.100 wrapped_avframe
> Error while decoding stream #0:0: Invalid data found when processing input
>     Last message repeated 3 times
> frame=    8 fps=7.7 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A speed=0.464x
> ====
>
> * 2. With this patch:
> All frames (12) decoded correctly (visually) with a warning in the header check.
>
> ====
> Input #0, ivf, from 'g2_decoder_case_15865.ivf':
>   Duration: 00:00:00.01, start: 0.000000, bitrate: 100442 kb/s
>     Stream #0:0: Video: vp9 (Profile 0) (VP90 / 0x30395056), yuv420p(tv), 1920x1088, 25 fps, 25 tbr, 1k tbn, 1k tbc
> Stream mapping:
>   Stream #0:0 -> #0:0 (vp9 (native) -> wrapped_avframe (native))
> Press [q] to stop, [?] for help
> [vp9 @ 0x56072d8df080] Invalid ref frame dimensions 1920x1088 for frame size 480x272
>     Last message repeated 1 times
> Output #0, null, to 'pipe:':
>   Metadata:
>     encoder         : Lavf58.41.100
>     Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200 kb/s, 25 fps, 25 tbn, 25 tbc
>     Metadata:
>       encoder         : Lavc58.75.100 wrapped_avframe
> frame=   12 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A speed=1.77x

Then I wonder if the commit message can be improved but that is just
nit-picking.

Thank you for your patience!

Carl Eugen
Fu, Linjie March 20, 2020, 1:36 a.m. UTC | #7 
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Carl Eugen Hoyos
> Sent: Friday, March 20, 2020 00:47
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> Am Do., 19. März 2020 um 16:12 Uhr schrieb Fu, Linjie <linjie.fu@intel.com>:
> >
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Carl Eugen Hoyos
> > > Sent: Thursday, March 19, 2020 22:13
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> > > dimensions check for SINGLE_REFERENCE mode
> > >
> > > Am Do., 19. März 2020 um 14:45 Uhr schrieb Fu, Linjie
> <linjie.fu@intel.com>:
> > >
> > > > This fix [v4] intended to report errors and exit decode_frame() for the
> > > exact frame witch has
> > > > invalid mvscale factors used, and didn't mean to break the decoding for
> > > subsequent frames if
> > > > they could be decoded correctly.
> > >
> > > How does the output look visually before this patch and how does it
> > > look with this patch?
> > >
> > $ ffmpeg -i g2_decoder_case_15865.ivf -f null -
> >
> > * 1. Before this patch:
> > Reported "Invalid ref frame dimensions" in the decode_frame_header()
> and abort the decoding.
> > (only 8 frame decode)
> >
> > ====
> > [vp9 @ 0x55e6496b3050] Invalid ref frame dimensions 1920x1088 for frame
> size 480x272
> > [vp9 @ 0x55e6496baab0] Not all references are available
> > [vp9 @ 0x55e6496c2510] Not all references are available
> > [vp9 @ 0x55e6496c5620] Not all references are available
> > Output #0, null, to 'pipe:':
> >   Metadata:
> >     encoder         : Lavf58.42.100
> >     Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200
> kb/s, 25 fps, 25 tbn, 25 tbc
> >     Metadata:
> >       encoder         : Lavc58.76.100 wrapped_avframe
> > Error while decoding stream #0:0: Invalid data found when processing input
> >     Last message repeated 3 times
> > frame=    8 fps=7.7 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A
> speed=0.464x
> > ====
> >
> > * 2. With this patch:
> > All frames (12) decoded correctly (visually) with a warning in the header
> check.
> >
> > ====
> > Input #0, ivf, from 'g2_decoder_case_15865.ivf':
> >   Duration: 00:00:00.01, start: 0.000000, bitrate: 100442 kb/s
> >     Stream #0:0: Video: vp9 (Profile 0) (VP90 / 0x30395056), yuv420p(tv),
> 1920x1088, 25 fps, 25 tbr, 1k tbn, 1k tbc
> > Stream mapping:
> >   Stream #0:0 -> #0:0 (vp9 (native) -> wrapped_avframe (native))
> > Press [q] to stop, [?] for help
> > [vp9 @ 0x56072d8df080] Invalid ref frame dimensions 1920x1088 for frame
> size 480x272
> >     Last message repeated 1 times
> > Output #0, null, to 'pipe:':
> >   Metadata:
> >     encoder         : Lavf58.41.100
> >     Stream #0:0: Video: wrapped_avframe, yuv420p, 1920x1088, q=2-31, 200
> kb/s, 25 fps, 25 tbn, 25 tbc
> >     Metadata:
> >       encoder         : Lavc58.75.100 wrapped_avframe
> > frame=   12 fps=0.0 q=-0.0 Lsize=N/A time=00:00:00.48 bitrate=N/A
> speed=1.77x
> 
> Then I wonder if the commit message can be improved but that is just
> nit-picking.
> 
> Thank you for your patience!
> 
With pleasure, thank you for the review.

- Linjie
Fu, Linjie March 20, 2020, 1:49 a.m. UTC | #8 
> From: Ronald S. Bultje <rsbultje@gmail.com>
> Sent: Thursday, March 19, 2020 20:10
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode
>
> Hi,
>
> On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu <linjie.fu@intel.com<mailto:linjie.fu@intel.com>> wrote:
> With the description in frame size with refs semantics (SPEC 7.2.5),
> it is a requirement of bitstream conformance that for at least one
> reference frame has the valid dimensions.
>
> Modify the check to make sure the decoder works well in SINGLE_REFERENCE
> mode that not all reference frames have valid dimensions.
>
> Check and error out if invalid reference frame is used in inter_recon.
>
> One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
> with following reference pool:
>
> 0.  960x544    LAST    valid
> 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> 3~7  ...     Unused
>
> Identical logic in libvpx:
> <https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_decodeframe.c#L736>
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com<mailto:linjie.fu@intel.com>>
> ---
> [v3]: replace assert with check/return, tested in both multi frame/slice mode
> [v4]: clear error_info to make decoding still work for other frames in this stream
>
> libavcodec/vp9.c      | 20 ++++++++++++++++++--
> libavcodec/vp9dec.h   |  5 +++++
> libavcodec/vp9recon.c | 10 ++++++++++
> 3 files changed, 33 insertions(+), 2 deletions(-)
>
> LGTM, thanks for the revisions. (We have been discussing this on IRC.)

Thanks for the review and valuable suggestions.

-  Linjie
Fu, Linjie April 29, 2020, 12:08 p.m. UTC | #9 
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu,
> Linjie
> Sent: Friday, March 20, 2020 09:49
> To: Ronald S. Bultje <rsbultje@gmail.com>; FFmpeg development
> discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> > From: Ronald S. Bultje <rsbultje@gmail.com>
> > Sent: Thursday, March 19, 2020 20:10
> > To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> > Cc: Fu, Linjie <linjie.fu@intel.com>
> > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> >
> > Hi,
> >
> > On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu
> <linjie.fu@intel.com<mailto:linjie.fu@intel.com>> wrote:
> > With the description in frame size with refs semantics (SPEC 7.2.5),
> > it is a requirement of bitstream conformance that for at least one
> > reference frame has the valid dimensions.
> >
> > Modify the check to make sure the decoder works well in
> SINGLE_REFERENCE
> > mode that not all reference frames have valid dimensions.
> >
> > Check and error out if invalid reference frame is used in inter_recon.
> >
> > One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
> > with following reference pool:
> >
> > 0.  960x544    LAST    valid
> > 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> > 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> > 3~7  ...     Unused
> >
> > Identical logic in libvpx:
> >
> <https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_d
> ecodeframe.c#L736>
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com<mailto:linjie.fu@intel.com>>
> > ---
> > [v3]: replace assert with check/return, tested in both multi frame/slice
> mode
> > [v4]: clear error_info to make decoding still work for other frames in this
> stream
> >
> > libavcodec/vp9.c      | 20 ++++++++++++++++++--
> > libavcodec/vp9dec.h   |  5 +++++
> > libavcodec/vp9recon.c | 10 ++++++++++
> > 3 files changed, 33 insertions(+), 2 deletions(-)
> >
> > LGTM, thanks for the revisions. (We have been discussing this on IRC.)
> 
> Thanks for the review and valuable suggestions.
> 
Ping for merge, thx.

- Linjie
Ronald S. Bultje May 5, 2020, 4:21 p.m. UTC | #10 
Hi,

On Wed, Apr 29, 2020 at 8:08 AM Fu, Linjie <linjie.fu@intel.com> wrote:

> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Fu,
> > Linjie
> > Sent: Friday, March 20, 2020 09:49
> > To: Ronald S. Bultje <rsbultje@gmail.com>; FFmpeg development
> > discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> > dimensions check for SINGLE_REFERENCE mode
> >
> > > From: Ronald S. Bultje <rsbultje@gmail.com>
> > > Sent: Thursday, March 19, 2020 20:10
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > > Cc: Fu, Linjie <linjie.fu@intel.com>
> > > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> > dimensions check for SINGLE_REFERENCE mode
> > >
> > > Hi,
> > >
> > > On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu
> > <linjie.fu@intel.com<mailto:linjie.fu@intel.com>> wrote:
> > > With the description in frame size with refs semantics (SPEC 7.2.5),
> > > it is a requirement of bitstream conformance that for at least one
> > > reference frame has the valid dimensions.
> > >
> > > Modify the check to make sure the decoder works well in
> > SINGLE_REFERENCE
> > > mode that not all reference frames have valid dimensions.
> > >
> > > Check and error out if invalid reference frame is used in inter_recon.
> > >
> > > One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE
> mode)
> > > with following reference pool:
> > >
> > > 0.  960x544    LAST    valid
> > > 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> > > 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> > > 3~7  ...     Unused
> > >
> > > Identical logic in libvpx:
> > >
> > <https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_d
> > ecodeframe.c#L736>
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com<mailto:
> linjie.fu@intel.com>>
> > > ---
> > > [v3]: replace assert with check/return, tested in both multi
> frame/slice
> > mode
> > > [v4]: clear error_info to make decoding still work for other frames in
> this
> > stream
> > >
> > > libavcodec/vp9.c      | 20 ++++++++++++++++++--
> > > libavcodec/vp9dec.h   |  5 +++++
> > > libavcodec/vp9recon.c | 10 ++++++++++
> > > 3 files changed, 33 insertions(+), 2 deletions(-)
> > >
> > > LGTM, thanks for the revisions. (We have been discussing this on IRC.)
> >
> > Thanks for the review and valuable suggestions.
> >
> Ping for merge, thx.


Applied.

Ronald
diff mbox series

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 7aaae9b..2b79dfd 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -794,6 +794,7 @@  static int decode_frame_header(AVCodecContext *avctx,
 
     /* check reference frames */
     if (!s->s.h.keyframe && !s->s.h.intraonly) {
+        int valid_ref_frame = 0;
         for (i = 0; i < 3; i++) {
             AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f;
             int refw = ref->width, refh = ref->height;
@@ -807,17 +808,25 @@  static int decode_frame_header(AVCodecContext *avctx,
             } else if (refw == w && refh == h) {
                 s->mvscale[i][0] = s->mvscale[i][1] = 0;
             } else {
+                /* Check to make sure at least one of frames that */
+                /* this frame references has valid dimensions     */
                 if (w * 2 < refw || h * 2 < refh || w > 16 * refw || h > 16 * refh) {
-                    av_log(avctx, AV_LOG_ERROR,
+                    av_log(avctx, AV_LOG_WARNING,
                            "Invalid ref frame dimensions %dx%d for frame size %dx%d\n",
                            refw, refh, w, h);
-                    return AVERROR_INVALIDDATA;
+                    s->mvscale[i][0] = s->mvscale[i][1] = REF_INVALID_SCALE;
+                    continue;
                 }
                 s->mvscale[i][0] = (refw << 14) / w;
                 s->mvscale[i][1] = (refh << 14) / h;
                 s->mvstep[i][0] = 16 * s->mvscale[i][0] >> 14;
                 s->mvstep[i][1] = 16 * s->mvscale[i][1] >> 14;
             }
+            valid_ref_frame++;
+        }
+        if (!valid_ref_frame) {
+            av_log(avctx, AV_LOG_ERROR, "No valid reference frame is found, bitstream not supported\n");
+            return AVERROR_INVALIDDATA;
         }
     }
 
@@ -1614,6 +1623,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
             s->td[i].eob = s->td[i].eob_base;
             s->td[i].uveob[0] = s->td[i].uveob_base[0];
             s->td[i].uveob[1] = s->td[i].uveob_base[1];
+            s->td[i].error_info = 0;
         }
 
 #if HAVE_THREADS
@@ -1670,6 +1680,12 @@  FF_ENABLE_DEPRECATION_WARNINGS
     } while (s->pass++ == 1);
     ff_thread_report_progress(&s->s.frames[CUR_FRAME].tf, INT_MAX, 0);
 
+    if (s->td->error_info < 0) {
+        av_log(avctx, AV_LOG_ERROR, "Failed to decode tile data\n");
+        s->td->error_info = 0;
+        return AVERROR_INVALIDDATA;
+    }
+
 finish:
     // ref frame setup
     for (i = 0; i < 8; i++) {
diff --git a/libavcodec/vp9dec.h b/libavcodec/vp9dec.h
index 66573ed..f2585c3 100644
--- a/libavcodec/vp9dec.h
+++ b/libavcodec/vp9dec.h
@@ -36,6 +36,8 @@ 
 #include "vp9dsp.h"
 #include "vp9shared.h"
 
+#define REF_INVALID_SCALE 0xFFFF
+
 enum MVJoint {
     MV_JOINT_ZERO,
     MV_JOINT_H,
@@ -217,6 +219,9 @@  struct VP9TileData {
     struct { int x, y; } min_mv, max_mv;
     int16_t *block_base, *block, *uvblock_base[2], *uvblock[2];
     uint8_t *eob_base, *uveob_base[2], *eob, *uveob[2];
+
+    // error message
+    int error_info;
 };
 
 void ff_vp9_fill_mv(VP9TileData *td, VP56mv *mv, int mode, int sb);
diff --git a/libavcodec/vp9recon.c b/libavcodec/vp9recon.c
index 49bb04e..9a4e7c7 100644
--- a/libavcodec/vp9recon.c
+++ b/libavcodec/vp9recon.c
@@ -572,6 +572,16 @@  static av_always_inline void inter_recon(VP9TileData *td, int bytesperpixel)
     VP9Block *b = td->b;
     int row = td->row, col = td->col;
 
+    if (s->mvscale[b->ref[0]][0] == REF_INVALID_SCALE ||
+        (b->comp && s->mvscale[b->ref[1]][0] == REF_INVALID_SCALE)) {
+        if (!s->td->error_info) {
+            s->td->error_info = AVERROR_INVALIDDATA;
+            av_log(NULL, AV_LOG_ERROR, "Bitstream not supported, "
+                                       "reference frame has invalid dimensions\n");
+        }
+        return;
+    }
+
     if (s->mvscale[b->ref[0]][0] || (b->comp && s->mvscale[b->ref[1]][0])) {
         if (bytesperpixel == 1) {
             inter_pred_scaled_8bpp(td);