| Message ID | 20200926102804.228089-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | a902c24994dea7cbdf84bff70ad8709958b1008d |
| Headers | show |
| Series | [FFmpeg-devel,01/25] avcodec/photocd: Simplify parsing Huffman tables a bit | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sat, Sep 26, 2020 at 12:27:40PM +0200, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/photocd.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > should be ok if tested. > diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c > index 057c9d33d4..8fd4536a65 100644 > --- a/libavcodec/photocd.c > +++ b/libavcodec/photocd.c > @@ -245,21 +245,20 @@ static av_noinline int decode_huff(AVCodecContext *avctx, AVFrame *frame, > int x2, idx; > > for (; get_bits_left(&g) > 0;) { > - if ((show_bits(&g, 24) & 0xfff000) == 0xfff000) > + if (show_bits(&g, 12) == 0xfff) > break; > skip_bits(&g, 8); > } > > - shiftreg = show_bits_long(&g, 32) & 0xffffff00; > - while (shiftreg != 0xfffffe00) { > + shiftreg = show_bits(&g, 24); > + while (shiftreg != 0xfffffe) { > if (get_bits_left(&g) <= 0) > return AVERROR_INVALIDDATA; > skip_bits(&g, 1); > - shiftreg = show_bits_long(&g, 32) & 0xffffff00; > + shiftreg = show_bits(&g, 24); > } > - skip_bits(&g, 16); > - y = show_bits_long(&g, 23) & 0x1fff; > - skip_bits(&g, 8); > + skip_bits(&g, 24); > + y = show_bits(&g, 15) & 0x1fff; > if (y >= height) > break; > type = get_bits(&g, 2); > -- > 2.25.1 > > _______________________________________________ > 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 Sat, Sep 26, 2020 at 12:27:41PM +0200, Andreas Rheinhardt wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/bytestream.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) lgtm > > diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h > index 0516a6e3dc..d0033f14f3 100644 > --- a/libavcodec/bytestream.h > +++ b/libavcodec/bytestream.h > @@ -77,11 +77,15 @@ static av_always_inline type bytestream2_get_ ## name(GetByteContext *g) \ > } \ > return bytestream2_get_ ## name ## u(g); \ > } \ > +static av_always_inline type bytestream2_peek_ ## name ## u(GetByteContext *g) \ > +{ \ > + return read(g->buffer); \ > +} \ > static av_always_inline type bytestream2_peek_ ## name(GetByteContext *g) \ > { \ > if (g->buffer_end - g->buffer < bytes) \ > return 0; \ > - return read(g->buffer); \ > + return bytestream2_peek_ ## name ## u(g); \ > } > > DEF(uint64_t, le64, 8, AV_RL64, AV_WL64) > -- > 2.25.1 > > _______________________________________________ > 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 Sat, Sep 26, 2020 at 12:27:49PM +0200, Andreas Rheinhardt wrote: > The MagicYUV format stores Huffman tables in its bitstream by coding > the length of a given symbol; it does not code the actual code directly, > instead this is to be inferred by the rule that a symbol is to the left > of every shorter symbol in the Huffman tree and that for symbols of the > same length the symbol is ascending from left to right. With one > exception, this is also what our decoder did. > > The exception only matters when there are codes of length 32, because > in this case the first symbol of this length did not get the code 0, > but 1; e.g. if there were exactly two nodes of length 32, then they > would get assigned the codes 1 and 2 and a node of length 31 will get > the 31-bit code 1 which is a prefix of the 32 bit code 2, making the > Huffman table invalid. On the other hand, if there were only one symbol > with the length 32, the earlier code would accept this un-Huffman-tree. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/magicyuv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) should be ok with subject fixed, it is not fix, its just fixes single case that never happens in reality. > > diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c > index 1b3f4cfc6b..17dea69d76 100644 > --- a/libavcodec/magicyuv.c > +++ b/libavcodec/magicyuv.c > @@ -86,7 +86,7 @@ static int huff_build(HuffEntry he[], VLC *vlc, int nb_elems) > > AV_QSORT(he, nb_elems, HuffEntry, huff_cmp_len); > > - code = 1; > + code = 0; > for (unsigned i = 0; i < nb_elems; i++) { > he[i].code = code >> (32 - he[i].len); > code += 0x80000000u >> (he[i].len - 1); > -- > 2.25.1 > > _______________________________________________ > 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".
Paul B Mahol: > On Sat, Sep 26, 2020 at 12:27:40PM +0200, Andreas Rheinhardt wrote: >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/photocd.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> > > should be ok if tested. > Tested with the (NSFW) files from http://cd.textfiles.com/prettywomen/IMAGES/ >> diff --git a/libavcodec/photocd.c b/libavcodec/photocd.c >> index 057c9d33d4..8fd4536a65 100644 >> --- a/libavcodec/photocd.c >> +++ b/libavcodec/photocd.c >> @@ -245,21 +245,20 @@ static av_noinline int decode_huff(AVCodecContext *avctx, AVFrame *frame, >> int x2, idx; >> >> for (; get_bits_left(&g) > 0;) { >> - if ((show_bits(&g, 24) & 0xfff000) == 0xfff000) >> + if (show_bits(&g, 12) == 0xfff) >> break; >> skip_bits(&g, 8); >> } >> >> - shiftreg = show_bits_long(&g, 32) & 0xffffff00; >> - while (shiftreg != 0xfffffe00) { >> + shiftreg = show_bits(&g, 24); >> + while (shiftreg != 0xfffffe) { >> if (get_bits_left(&g) <= 0) >> return AVERROR_INVALIDDATA; >> skip_bits(&g, 1); >> - shiftreg = show_bits_long(&g, 32) & 0xffffff00; >> + shiftreg = show_bits(&g, 24); >> } >> - skip_bits(&g, 16); >> - y = show_bits_long(&g, 23) & 0x1fff; >> - skip_bits(&g, 8); >> + skip_bits(&g, 24); >> + y = show_bits(&g, 15) & 0x1fff; >> if (y >= height) >> break; >> type = get_bits(&g, 2); >> -- >> 2.25.1 >>
Paul B Mahol: > On Sat, Sep 26, 2020 at 12:27:49PM +0200, Andreas Rheinhardt wrote: >> The MagicYUV format stores Huffman tables in its bitstream by coding >> the length of a given symbol; it does not code the actual code directly, >> instead this is to be inferred by the rule that a symbol is to the left >> of every shorter symbol in the Huffman tree and that for symbols of the >> same length the symbol is ascending from left to right. With one >> exception, this is also what our decoder did. >> >> The exception only matters when there are codes of length 32, because >> in this case the first symbol of this length did not get the code 0, >> but 1; e.g. if there were exactly two nodes of length 32, then they >> would get assigned the codes 1 and 2 and a node of length 31 will get >> the 31-bit code 1 which is a prefix of the 32 bit code 2, making the >> Huffman table invalid. On the other hand, if there were only one symbol >> with the length 32, the earlier code would accept this un-Huffman-tree. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/magicyuv.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > should be ok with subject fixed, it is not fix, its just fixes single > case that never happens in reality. > I know that this does not affect normal files, but I have trouble finding an accurate commit message that avoids "fix". So how about "Fix edge case when building Huffman tables"? >> >> diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c >> index 1b3f4cfc6b..17dea69d76 100644 >> --- a/libavcodec/magicyuv.c >> +++ b/libavcodec/magicyuv.c >> @@ -86,7 +86,7 @@ static int huff_build(HuffEntry he[], VLC *vlc, int nb_elems) >> >> AV_QSORT(he, nb_elems, HuffEntry, huff_cmp_len); >> >> - code = 1; >> + code = 0; >> for (unsigned i = 0; i < nb_elems; i++) { >> he[i].code = code >> (32 - he[i].len); >> code += 0x80000000u >> (he[i].len - 1); >> -- >> 2.25.1 >> >> _______________________________________________ >> 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/photocd.c b/libavcodec/photocd.c index 057c9d33d4..8fd4536a65 100644 --- a/libavcodec/photocd.c +++ b/libavcodec/photocd.c @@ -245,21 +245,20 @@ static av_noinline int decode_huff(AVCodecContext *avctx, AVFrame *frame, int x2, idx; for (; get_bits_left(&g) > 0;) { - if ((show_bits(&g, 24) & 0xfff000) == 0xfff000) + if (show_bits(&g, 12) == 0xfff) break; skip_bits(&g, 8); } - shiftreg = show_bits_long(&g, 32) & 0xffffff00; - while (shiftreg != 0xfffffe00) { + shiftreg = show_bits(&g, 24); + while (shiftreg != 0xfffffe) { if (get_bits_left(&g) <= 0) return AVERROR_INVALIDDATA; skip_bits(&g, 1); - shiftreg = show_bits_long(&g, 32) & 0xffffff00; + shiftreg = show_bits(&g, 24); } - skip_bits(&g, 16); - y = show_bits_long(&g, 23) & 0x1fff; - skip_bits(&g, 8); + skip_bits(&g, 24); + y = show_bits(&g, 15) & 0x1fff; if (y >= height) break; type = get_bits(&g, 2);
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/photocd.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)