diff mbox series

[FFmpeg-devel,01/25] avcodec/photocd: Simplify parsing Huffman tables a bit

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 26, 2020, 10:27 a.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/photocd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Paul B Mahol Sept. 26, 2020, 10:47 a.m. UTC | #1
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".
Paul B Mahol Sept. 26, 2020, 10:48 a.m. UTC | #2
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".
Paul B Mahol Sept. 26, 2020, 10:55 a.m. UTC | #3
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".
Andreas Rheinhardt Sept. 26, 2020, 11:01 a.m. UTC | #4
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
>>
Andreas Rheinhardt Sept. 26, 2020, 11:14 a.m. UTC | #5
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 mbox series

Patch

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);