[FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[Andreas Rheinhardt]

L[i] can be in the range of 0-255, see table B.5 of ITU T.81.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/cbs_jpeg.h                 | 2 +-
 libavcodec/cbs_jpeg_syntax_template.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavcodec/cbs_jpeg.h b/libavcodec/cbs_jpeg.h
index 6305f0ee86..9dbebd259f 100644
--- a/libavcodec/cbs_jpeg.h
+++ b/libavcodec/cbs_jpeg.h
@@ -99,7 +99,7 @@ typedef struct JPEGRawHuffmanTable {
     uint8_t  Tc;
     uint8_t  Th;
     uint8_t  L[16];
-    uint8_t  V[224];
+    uint8_t  V[256];
 } JPEGRawHuffmanTable;
 
 typedef struct JPEGRawHuffmanTableSpecification {
diff --git a/libavcodec/cbs_jpeg_syntax_template.c b/libavcodec/cbs_jpeg_syntax_template.c
index 6eda56d623..e06abdc674 100644
--- a/libavcodec/cbs_jpeg_syntax_template.c
+++ b/libavcodec/cbs_jpeg_syntax_template.c
@@ -84,12 +84,12 @@ static int FUNC(huffman_table)(CodedBitstreamContext *ctx, RWContext *rw,
     u(4, Th, 0, 3);
 
     for (i = 0; i < 16; i++)
-        us(8, L[i], i, 0, 224);
+        us(8, L[i], i, 0, 255);
 
     ij = 0;
     for (i = 0; i < 16; i++) {
         for (j = 0; j < current->L[i]; j++) {
-            if (ij >= 224)
+            if (ij >= FF_ARRAY_ELEMS(current->V))
                 return AVERROR_INVALIDDATA;
             us(8, V[ij], ij, 0, 255);
             ++ij;
-- 
2.32.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[Andreas Rheinhardt]

Andreas Rheinhardt:
> L[i] can be in the range of 0-255, see table B.5 of ITU T.81.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/cbs_jpeg.h                 | 2 +-
>  libavcodec/cbs_jpeg_syntax_template.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/cbs_jpeg.h b/libavcodec/cbs_jpeg.h
> index 6305f0ee86..9dbebd259f 100644
> --- a/libavcodec/cbs_jpeg.h
> +++ b/libavcodec/cbs_jpeg.h
> @@ -99,7 +99,7 @@ typedef struct JPEGRawHuffmanTable {
>      uint8_t  Tc;
>      uint8_t  Th;
>      uint8_t  L[16];
> -    uint8_t  V[224];
> +    uint8_t  V[256];
>  } JPEGRawHuffmanTable;
>  
>  typedef struct JPEGRawHuffmanTableSpecification {
> diff --git a/libavcodec/cbs_jpeg_syntax_template.c b/libavcodec/cbs_jpeg_syntax_template.c
> index 6eda56d623..e06abdc674 100644
> --- a/libavcodec/cbs_jpeg_syntax_template.c
> +++ b/libavcodec/cbs_jpeg_syntax_template.c
> @@ -84,12 +84,12 @@ static int FUNC(huffman_table)(CodedBitstreamContext *ctx, RWContext *rw,
>      u(4, Th, 0, 3);
>  
>      for (i = 0; i < 16; i++)
> -        us(8, L[i], i, 0, 224);
> +        us(8, L[i], i, 0, 255);
>  
>      ij = 0;
>      for (i = 0; i < 16; i++) {
>          for (j = 0; j < current->L[i]; j++) {
> -            if (ij >= 224)
> +            if (ij >= FF_ARRAY_ELEMS(current->V))
>                  return AVERROR_INVALIDDATA;
>              us(8, V[ij], ij, 0, 255);
>              ++ij;

Will apply this later tonight unless there are objections.

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[Mark Thompson]

On 08/02/2022 09:41, Andreas Rheinhardt wrote:
> L[i] can be in the range of 0-255, see table B.5 of ITU T.81.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/cbs_jpeg.h                 | 2 +-
>   libavcodec/cbs_jpeg_syntax_template.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

Do you have a valid file showing this?  Not all values are allowed.

I guess I must have written it, but I have no idea where 224 came from.  As far as I know the worst case is in AC tables: 10 category values * 16 run lengths + 2 special cases = 162 (which could indeed all be dumped in the same code length if you want to be pathological).

- Mark
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[Andreas Rheinhardt]

Mark Thompson:
> On 08/02/2022 09:41, Andreas Rheinhardt wrote:
>> L[i] can be in the range of 0-255, see table B.5 of ITU T.81.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/cbs_jpeg.h                 | 2 +-
>>   libavcodec/cbs_jpeg_syntax_template.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> Do you have a valid file showing this?  Not all values are allowed.
> 

Where is this said in the spec?
The file jpg/12bpp.jpg from the FATE-suite triggers this. It has a
Huffman table with 226 entries.
(Sorry, should have mentioned the sample in the commit message.)

> I guess I must have written it, but I have no idea where 224 came from. 
> As far as I know the worst case is in AC tables: 10 category values * 16
> run lengths + 2 special cases = 162 (which could indeed all be dumped in
> the same code length if you want to be pathological).

I have never heard of these restrictions. Would you care to elaborate
which part of the spec they refer to?
Anyway, IIRC there is no restriction against duplicates in the Huffman
table, so one could use even more than 256 values (i.e. there might be
spec-compliant pictures that are not supported by both our decoder and
the current version of cbs_jpeg); it just makes no sense. Notice that
the sample mentioned above has no duplicate values in any Huffman table.

- Andreas
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cbs_jpeg: Fix size of huffman symbol table array

[Mark Thompson]

On 30/04/2022 19:38, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 08/02/2022 09:41, Andreas Rheinhardt wrote:
>>> L[i] can be in the range of 0-255, see table B.5 of ITU T.81.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>    libavcodec/cbs_jpeg.h                 | 2 +-
>>>    libavcodec/cbs_jpeg_syntax_template.c | 4 ++--
>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> Do you have a valid file showing this?  Not all values are allowed.
>>
> 
> Where is this said in the spec?
> The file jpg/12bpp.jpg from the FATE-suite triggers this. It has a
> Huffman table with 226 entries.
> (Sorry, should have mentioned the sample in the commit message.)
> 
>> I guess I must have written it, but I have no idea where 224 came from.
>> As far as I know the worst case is in AC tables: 10 category values * 16
>> run lengths + 2 special cases = 162 (which could indeed all be dumped in
>> the same code length if you want to be pathological).
> 
> I have never heard of these restrictions. Would you care to elaborate
> which part of the spec they refer to?

Urgh.  I was thinking of F.1.2.2.1, defining 10 categories (figure F.1 illustrates the 162 possible values).

F.1.5.2 for 12-bit extends that with four additional categories for a total of 226 values.  Maybe that's where 224 came from, except typoed.

> Anyway, IIRC there is no restriction against duplicates in the Huffman
> table, so one could use even more than 256 values (i.e. there might be
> spec-compliant pictures that are not supported by both our decoder and
> the current version of cbs_jpeg); it just makes no sense. Notice that
> the sample mentioned above has no duplicate values in any Huffman table.

If duplicates were allowed then the whole thing could have a lot more than 256 entries (e.g. 255 entries in each of 9-16 bit length (covering ~half the remaining space in each case) is 2040).  I feel like there must be a prohibition against this somewhere, though I don't see it.

- Mark
_______________________________________________
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".