[FFmpeg-devel] [PATCH 2/3] avcodec/mpeg12: Don't pretend reading dct_dc_size_* VLCs can fail

[Andreas Rheinhardt]

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 at 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