| Message ID | CAPYw7P7KfEnFGJO=F2-oHKx=p-ARxpsFb7SzdZ8zNUvWAmCyeA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/photocd: allow decoding vlc with errors | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | fail | Make failed |
Quoting Paul B Mahol (2023-09-27 21:53:54) > Attached. > > From 6b7e51e2b4facead9c45fb94647c85b58e6fdf22 Mon Sep 17 00:00:00 2001 > From: Paul B Mahol <onemda@gmail.com> > Date: Wed, 27 Sep 2023 21:48:49 +0200 > Subject: [PATCH] avcodec/photocd: allow decoding vlc with errors > > Also rename option that no longer works. > Also make tables finding more robust. Why do all these things in a single patch? And why are there no tests for this decoder? > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/photocd.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c > index 07e8d460bd..a9e42bb86a 100644 > --- a/libavcodec/photocd.c > +++ b/libavcodec/photocd.c > @@ -277,8 +277,6 @@ static av_noinline int decode_huff(AVCodecContext *avctx, AVFrame *frame, > if (get_bits_left(&g) <= 0) > return AVERROR_INVALIDDATA; > m = get_vlc2(&g, s->vlc[idx].table, s->vlc[idx].bits, 2); > - if (m < 0) > - return AVERROR_INVALIDDATA; Shouldn't this error be at least logged? Perhaps it should still fail on AV_EF_EXPLODE. > m = sign_extend(m, 8); > data[x] = av_clip_uint8(data[x] + m); > } > @@ -389,6 +387,7 @@ static int photocd_decode_frame(AVCodecContext *avctx, AVFrame *p, > return AVERROR_INVALIDDATA; > > if (s->resolution == 4) { > + s->streampos = (AV_RB16(avpkt->data + 2048 + 1536 + 3) + 12) * 2048; Where do these magic numbers come from? Same below.
On 9/28/23, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2023-09-27 21:53:54) >> Attached. >> >> From 6b7e51e2b4facead9c45fb94647c85b58e6fdf22 Mon Sep 17 00:00:00 2001 >> From: Paul B Mahol <onemda@gmail.com> >> Date: Wed, 27 Sep 2023 21:48:49 +0200 >> Subject: [PATCH] avcodec/photocd: allow decoding vlc with errors >> >> Also rename option that no longer works. >> Also make tables finding more robust. > > Why do all these things in a single patch? > I can boost my commits if you prefer that way. > And why are there no tests for this decoder? Its complicated. > >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavcodec/photocd.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c >> index 07e8d460bd..a9e42bb86a 100644 >> --- a/libavcodec/photocd.c >> +++ b/libavcodec/photocd.c >> @@ -277,8 +277,6 @@ static av_noinline int decode_huff(AVCodecContext >> *avctx, AVFrame *frame, >> if (get_bits_left(&g) <= 0) >> return AVERROR_INVALIDDATA; >> m = get_vlc2(&g, s->vlc[idx].table, s->vlc[idx].bits, 2); >> - if (m < 0) >> - return AVERROR_INVALIDDATA; > > Shouldn't this error be at least logged? Perhaps it should still fail on > AV_EF_EXPLODE. > >> m = sign_extend(m, 8); >> data[x] = av_clip_uint8(data[x] + m); >> } >> @@ -389,6 +387,7 @@ static int photocd_decode_frame(AVCodecContext *avctx, >> AVFrame *p, >> return AVERROR_INVALIDDATA; >> >> if (s->resolution == 4) { >> + s->streampos = (AV_RB16(avpkt->data + 2048 + 1536 + 3) + 12) >> * 2048; > > Where do these magic numbers come from? Same below. From offset from internal image file bitstream table structure. > > -- > Anton Khirnov > _______________________________________________ > 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". >
Quoting Paul B Mahol (2023-09-28 11:48:14) > On 9/28/23, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Paul B Mahol (2023-09-27 21:53:54) > >> Attached. > >> > >> From 6b7e51e2b4facead9c45fb94647c85b58e6fdf22 Mon Sep 17 00:00:00 2001 > >> From: Paul B Mahol <onemda@gmail.com> > >> Date: Wed, 27 Sep 2023 21:48:49 +0200 > >> Subject: [PATCH] avcodec/photocd: allow decoding vlc with errors > >> > >> Also rename option that no longer works. > >> Also make tables finding more robust. > > > > Why do all these things in a single patch? > > > > I can boost my commits if you prefer that way. Feel welcome. > >> + s->streampos = (AV_RB16(avpkt->data + 2048 + 1536 + 3) + 12) > >> * 2048; > > > > Where do these magic numbers come from? Same below. > > From offset from internal image file bitstream table structure. Please describe that in a comment above that line then.
From 6b7e51e2b4facead9c45fb94647c85b58e6fdf22 Mon Sep 17 00:00:00 2001 From: Paul B Mahol <onemda@gmail.com> Date: Wed, 27 Sep 2023 21:48:49 +0200 Subject: [PATCH] avcodec/photocd: allow decoding vlc with errors Also rename option that no longer works. Also make tables finding more robust. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/photocd.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c index 07e8d460bd..a9e42bb86a 100644 --- a/libavcodec/photocd.c +++ b/libavcodec/photocd.c @@ -277,8 +277,6 @@ static av_noinline int decode_huff(AVCodecContext *avctx, AVFrame *frame, if (get_bits_left(&g) <= 0) return AVERROR_INVALIDDATA; m = get_vlc2(&g, s->vlc[idx].table, s->vlc[idx].bits, 2); - if (m < 0) - return AVERROR_INVALIDDATA; m = sign_extend(m, 8); data[x] = av_clip_uint8(data[x] + m); } @@ -389,6 +387,7 @@ static int photocd_decode_frame(AVCodecContext *avctx, AVFrame *p, return AVERROR_INVALIDDATA; if (s->resolution == 4) { + s->streampos = (AV_RB16(avpkt->data + 2048 + 1536 + 3) + 12) * 2048; interp_pixels(ptr, p->linesize[0], img_info[4].width, img_info[4].height); interp_lines (ptr, p->linesize[0], img_info[4].width, img_info[4].height); @@ -396,7 +395,7 @@ static int photocd_decode_frame(AVCodecContext *avctx, AVFrame *p, if ((ret = read_hufftable(avctx, &s->vlc[n])) < 0) return ret; } - s->streampos = (s->streampos + 2047) & ~0x3ff; + s->streampos = (AV_RB16(avpkt->data + 2048 + 1536 + 3) + 14) * 2048; if (decode_huff(avctx, p, 4, 4) < 0) return AVERROR_INVALIDDATA; } @@ -447,7 +446,7 @@ static av_cold int photocd_decode_close(AVCodecContext *avctx) #define VD AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM static const AVOption options[] = { - { "lowres", "Lower the decoding resolution by a power of two", + { "reduction", "Lower the decoding resolution by a power of two", OFFSET(lowres), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 4, VD }, { NULL }, }; -- 2.42.0