| Message ID | 20201008195313.471755-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 7800cc6e82068c6dfb5af53817f03dfda794c568 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/mpeg12: Reduce size of motion-vector VLC | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, Oct 08, 2020 at 09:53:12PM +0200, Andreas Rheinhardt wrote: > It can't because the corresponding trees don't have any loose ends. > > Removing the checks also removed an instance of av_log(NULL (with a > nonsense message) from the codebase. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/mdec.c | 2 -- > libavcodec/mpeg12.h | 4 ---- > libavcodec/mpeg12dec.c | 4 ---- > 3 files changed, 10 deletions(-) It is possible to encode nonsensical dc differnces, these could be checked for and trigger errors. Not in the current codebase no, but one could add a check for it Without such checks, your patch is probably ok thx [...]
Michael Niedermayer: > On Thu, Oct 08, 2020 at 09:53:12PM +0200, Andreas Rheinhardt wrote: >> It can't because the corresponding trees don't have any loose ends. >> >> Removing the checks also removed an instance of av_log(NULL (with a >> nonsense message) from the codebase. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/mdec.c | 2 -- >> libavcodec/mpeg12.h | 4 ---- >> libavcodec/mpeg12dec.c | 4 ---- >> 3 files changed, 10 deletions(-) > > It is possible to encode nonsensical dc differnces, these could be checked > for and trigger errors. Not in the current codebase no, but one could add > a check for it > Without such checks, your patch is probably ok > It is not possible to encode a nonsense difference. The standard has a requirement that predictor (last_dc[component] in our code) + diff be in a certain range, but that is a check that should be performed at the place where the addition is performed; decode_dc() (in its current form) simply lacks the context to know whether the diff it parsed will make the sum violate this requirement. - Andreas
On Sat, Oct 10, 2020 at 12:08:20AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Thu, Oct 08, 2020 at 09:53:12PM +0200, Andreas Rheinhardt wrote: > >> It can't because the corresponding trees don't have any loose ends. > >> > >> Removing the checks also removed an instance of av_log(NULL (with a > >> nonsense message) from the codebase. > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> libavcodec/mdec.c | 2 -- > >> libavcodec/mpeg12.h | 4 ---- > >> libavcodec/mpeg12dec.c | 4 ---- > >> 3 files changed, 10 deletions(-) > > > > It is possible to encode nonsensical dc differnces, these could be checked > > for and trigger errors. Not in the current codebase no, but one could add > > a check for it > > Without such checks, your patch is probably ok > > > It is not possible to encode a nonsense difference. The standard has a > requirement that predictor (last_dc[component] in our code) + diff be in > a certain range, but that is a check that should be performed at the > place where the addition is performed; decode_dc() (in its current form) > simply lacks the context to know whether the diff it parsed will make > the sum violate this requirement. Some VLC codecs are invalid in lower bit per DC files (IIRC thats most) These could be removed from the VLC table used to decode them. no additional context is needed in decode_dc() for that Of course checking the DC outside works better and we should prefer that [...]
Michael Niedermayer: > On Sat, Oct 10, 2020 at 12:08:20AM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> On Thu, Oct 08, 2020 at 09:53:12PM +0200, Andreas Rheinhardt wrote: >>>> It can't because the corresponding trees don't have any loose ends. >>>> >>>> Removing the checks also removed an instance of av_log(NULL (with a >>>> nonsense message) from the codebase. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavcodec/mdec.c | 2 -- >>>> libavcodec/mpeg12.h | 4 ---- >>>> libavcodec/mpeg12dec.c | 4 ---- >>>> 3 files changed, 10 deletions(-) >>> >>> It is possible to encode nonsensical dc differnces, these could be checked >>> for and trigger errors. Not in the current codebase no, but one could add >>> a check for it >>> Without such checks, your patch is probably ok >>> >> It is not possible to encode a nonsense difference. The standard has a >> requirement that predictor (last_dc[component] in our code) + diff be in >> a certain range, but that is a check that should be performed at the >> place where the addition is performed; decode_dc() (in its current form) >> simply lacks the context to know whether the diff it parsed will make >> the sum violate this requirement. > > Some VLC codecs are invalid in lower bit per DC files (IIRC thats most) > These could be removed from the VLC table used to decode them. > no additional context is needed in decode_dc() for that > While some codes (namely the long ones with big dct_dc_size_*) will automatically lead to a violation of this requirement if intra_dc_precision is small, checking for it in decode_dc() would require exactly the context I spoke of. And it would still not make sense, as one would still have to check for whether the actual new coefficient is within the allowed range afterwards. And using different VLC tables depending upon intra_dc_precision and removing certain VLC codes from certain tables makes even less sense: One would have to add new tables, more code for initializing the tables, more code to actually use the right table (one would have to add context to decode_dc() for it) and then one would have to add checks that the code is actually valid (or rather, one would have to keep the checks that I intend to remove). But even then it is not guaranteed that the new coefficient is actually within the allowed range, so one would still need to perform checks outside of decode_dc() and absolutely nothing would be gained by this. > Of course checking the DC outside works better and we should prefer that > > [...] - Andreas
diff --git a/libavcodec/mdec.c b/libavcodec/mdec.c index 7e34ec568e..b16cbb6a79 100644 --- a/libavcodec/mdec.c +++ b/libavcodec/mdec.c @@ -71,8 +71,6 @@ static inline int mdec_decode_block_intra(MDECContext *a, int16_t *block, int n) } else { component = (n <= 3 ? 0 : n - 4 + 1); diff = decode_dc(&a->gb, component); - if (diff >= 0xffff) - return AVERROR_INVALIDDATA; a->last_dc[component] += diff; block[0] = a->last_dc[component] * (1 << 3); } diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h index 1ec99f17e1..345d473d3a 100644 --- a/libavcodec/mpeg12.h +++ b/libavcodec/mpeg12.h @@ -47,10 +47,6 @@ static inline int decode_dc(GetBitContext *gb, int component) } else { code = get_vlc2(gb, ff_dc_chroma_vlc.table, DC_VLC_BITS, 2); } - if (code < 0){ - av_log(NULL, AV_LOG_ERROR, "invalid dc code at\n"); - return 0xffff; - } if (code == 0) { diff = 0; } else { diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index 2494226aa3..7b448d3648 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -490,8 +490,6 @@ static inline int mpeg2_decode_block_intra(MpegEncContext *s, component = (n & 1) + 1; } diff = decode_dc(&s->gb, component); - if (diff >= 0xffff) - return AVERROR_INVALIDDATA; dc = s->last_dc[component]; dc += diff; s->last_dc[component] = dc; @@ -578,8 +576,6 @@ static inline int mpeg2_fast_decode_block_intra(MpegEncContext *s, component = (n & 1) + 1; } diff = decode_dc(&s->gb, component); - if (diff >= 0xffff) - return AVERROR_INVALIDDATA; dc = s->last_dc[component]; dc += diff; s->last_dc[component] = dc;
It can't because the corresponding trees don't have any loose ends. Removing the checks also removed an instance of av_log(NULL (with a nonsense message) from the codebase. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/mdec.c | 2 -- libavcodec/mpeg12.h | 4 ---- libavcodec/mpeg12dec.c | 4 ---- 3 files changed, 10 deletions(-)