| Message ID | AM7PR03MB66606E1CA7B99EBF74E10A778F2D9@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 321c31cfe632cf7763e2e09765d59f1ab40abf3a |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/cbs_jpeg: Fix size of huffman symbol table array | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
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
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
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
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
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;
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(-)