| Message ID | 20230726235916.30058-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/rtv1: Check if the minimal size is available in decode_rtv1() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/rtv1.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) will apply 1-3 of this patchset [...]
On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/rtv1.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) > > will apply 1-3 of this patchset Are you sure this does not break decoding? It would not be first time you intentionally break decoders because of hacks. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > He who knows, does not speak. He who speaks, does not know. -- Lao Tsu >
On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: > On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavcodec/rtv1.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > > > > will apply 1-3 of this patchset > > Are you sure this does not break decoding? Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum check looks correct. There are 2 end of bitstream checks for early exit but they look like error handling not some normal exit as they leave the frame uninitialized Do you have some files so i can double check this is not breaking anything ? thx [...]
On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> --- >> >> libavcodec/rtv1.c | 6 +++++- >> >> 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > will apply 1-3 of this patchset >> >> Are you sure this does not break decoding? > > Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum > check looks correct. > There are 2 end of bitstream checks for early exit but they look like > error handling not some normal exit as they leave the frame uninitialized > FFmpeg default initialization code for AVFrame's buffers does it twice, so they are always zeroed or previous values of previous buffers in pool. > Do you have some files so i can double check this is not breaking anything Search trac tickets. > ? > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Those who would give up essential Liberty, to purchase a little > temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin >
On Fri, Sep 22, 2023 at 11:30:37PM +0200, Paul B Mahol wrote: > On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: > >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: > >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> --- > >> >> libavcodec/rtv1.c | 6 +++++- > >> >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > > >> > will apply 1-3 of this patchset > >> > >> Are you sure this does not break decoding? > > > > Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum > > check looks correct. > > There are 2 end of bitstream checks for early exit but they look like > > error handling not some normal exit as they leave the frame uninitialized > > > > FFmpeg default initialization code for AVFrame's buffers does it > twice, so they are always zeroed or previous values of previous > buffers in pool. its rare that correct frame decoding depends on internal AVFrame buffer ordering > > > Do you have some files so i can double check this is not breaking anything > > Search trac tickets. thanks, found a zip with 2 video files ill check it [...]
On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Sep 22, 2023 at 11:30:37PM +0200, Paul B Mahol wrote: >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: >> >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: >> >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> >> --- >> >> >> libavcodec/rtv1.c | 6 +++++- >> >> >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> > >> >> > will apply 1-3 of this patchset >> >> >> >> Are you sure this does not break decoding? >> > >> > Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum >> > check looks correct. >> > There are 2 end of bitstream checks for early exit but they look like >> > error handling not some normal exit as they leave the frame >> > uninitialized >> > >> >> FFmpeg default initialization code for AVFrame's buffers does it >> twice, so they are always zeroed or previous values of previous >> buffers in pool. > > its rare that correct frame decoding depends on internal AVFrame buffer > ordering > Users are supposed to use error checking. And I think decoder returns error on missing frame data. When we lost interest in preserving all decoded frame pixels as much as possible? > >> >> > Do you have some files so i can double check this is not breaking >> > anything >> >> Search trac tickets. > > thanks, found a zip with 2 video files > ill check it > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > "I am not trying to be anyone's saviour, I'm trying to think about the > future and not be sad" - Elon Musk > >
On Sat, Sep 23, 2023 at 12:01:17AM +0200, Paul B Mahol wrote: > On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, Sep 22, 2023 at 11:30:37PM +0200, Paul B Mahol wrote: > >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: > >> >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> >> > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer wrote: > >> >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> >> --- > >> >> >> libavcodec/rtv1.c | 6 +++++- > >> >> >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> >> > > >> >> > will apply 1-3 of this patchset > >> >> > >> >> Are you sure this does not break decoding? > >> > > >> > Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum > >> > check looks correct. > >> > There are 2 end of bitstream checks for early exit but they look like > >> > error handling not some normal exit as they leave the frame > >> > uninitialized > >> > > >> > >> FFmpeg default initialization code for AVFrame's buffers does it > >> twice, so they are always zeroed or previous values of previous > >> buffers in pool. > > > > its rare that correct frame decoding depends on internal AVFrame buffer > > ordering > > > > Users are supposed to use error checking. And I think decoder returns > error on missing frame data. yes, the rtv1 decoder looks a bit sloppy written, not returning error codes on what looks like error checks. Its not the only code doing that, ive seen this in other files too > > When we lost interest in preserving all decoded frame pixels as much > as possible? when patches using discard_damaged_percentage where getting blocked in review while simpler but less ideal solutions made all reviewers happy I can implement this using discard_damaged_percentage, then the user can decide at which point a frame would be too damaged to decode/return and also to drop none or all with damage as the user prefers thx [...]
On 9/23/23, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Sep 23, 2023 at 12:01:17AM +0200, Paul B Mahol wrote: >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Fri, Sep 22, 2023 at 11:30:37PM +0200, Paul B Mahol wrote: >> >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > On Fri, Sep 22, 2023 at 09:32:47PM +0200, Paul B Mahol wrote: >> >> >> On 9/22/23, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> >> > On Thu, Jul 27, 2023 at 01:59:13AM +0200, Michael Niedermayer >> >> >> > wrote: >> >> >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> >> >> --- >> >> >> >> libavcodec/rtv1.c | 6 +++++- >> >> >> >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> >> > >> >> >> > will apply 1-3 of this patchset >> >> >> >> >> >> Are you sure this does not break decoding? >> >> > >> >> > Well, its a loop over 4x4 blocks, a 16bit "skip" run so the minimum >> >> > check looks correct. >> >> > There are 2 end of bitstream checks for early exit but they look >> >> > like >> >> > error handling not some normal exit as they leave the frame >> >> > uninitialized >> >> > >> >> >> >> FFmpeg default initialization code for AVFrame's buffers does it >> >> twice, so they are always zeroed or previous values of previous >> >> buffers in pool. >> > >> > its rare that correct frame decoding depends on internal AVFrame buffer >> > ordering >> > >> >> Users are supposed to use error checking. And I think decoder returns >> error on missing frame data. > > yes, the rtv1 decoder looks a bit sloppy written, not returning error > codes on what looks like error checks. > Its not the only code doing that, ive seen this in other files too You are last person here to call some decoder(s) are sloppy written. > > >> >> When we lost interest in preserving all decoded frame pixels as much >> as possible? > > when patches using discard_damaged_percentage where getting blocked in > review > while simpler but less ideal solutions made all reviewers happy > > > I can implement this using discard_damaged_percentage, then the user can > decide at which point a frame would be too damaged to decode/return > and also to drop none or all with damage as the user prefers > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Homeopathy is like voting while filling the ballot out with transparent > ink. > Sometimes the outcome one wanted occurs. Rarely its worse than filling out > a ballot properly. >
diff --git a/libavcodec/rtv1.c b/libavcodec/rtv1.c index 4b202e6a21..95fa9210d8 100644 --- a/libavcodec/rtv1.c +++ b/libavcodec/rtv1.c @@ -44,6 +44,8 @@ static int decode_rtv1(GetByteContext *gb, uint8_t *dst, ptrdiff_t linesize, uint8_t block[8] = { 0 }; int run = 0; + if (bytestream2_get_bytes_left(gb) < (width / 4) * (height / 4) / 0xFFFF * 4) + return AVERROR_INVALIDDATA; for (int y = 0; y < height; y += 4) { for (int x = 0; x < width * 4; x += 16) { int mode = 0; @@ -126,7 +128,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p, dst = p->data[0] + p->linesize[0] * (avctx->coded_height - 1); linesize = -p->linesize[0]; - decode_rtv1(&gb, dst, linesize, width, height, flags, dsp->dxt1_block); + ret = decode_rtv1(&gb, dst, linesize, width, height, flags, dsp->dxt1_block); + if (ret < 0) + return ret; p->pict_type = AV_PICTURE_TYPE_I; p->flags |= AV_FRAME_FLAG_KEY;
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/rtv1.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)