[FFmpeg-devel] [PATCH 01/10] avcodec/dca: remove Rice code length limit

[Christophe Gisquet]

Hi,

2016-05-20 15:09 GMT+02:00 foo86 <foobaz86 at gmail.com>:

>> Not that the patch is not ok, but I have a few uneducated questions:
>> 1) Given the get_bits_long(gb, k) afterwards, won't that code cause
>> overreads for corrupted bitstreams?
>
> This will cause overread, but that should not be a problem for checked
> bitstream reader.
>
>> 2) I haven't checked the calling code, but consequently, wouldn't it
>> be better to first check that at least k+1 bits are available?
>
> I don't think this is necessarry. Fixed length overread is safe; adding
> explicit check will make code less readable IMO (and possibly slower).

I think the checked bitstream reader takes care of these.

>> 3) 128 is already fairly large; is the new code for valid bitstreams
>> (in the sense of specs and actually generated) or for corrupted
>> bitstreams? I don't know where the parsing is validated afterwards
>> (e.g. if there have been overreads or invalid values parsed)
>
> This is for valid bitstreams. There is no indication of limit on maximum
> Rice code length in the XLL spec, hence existing constant is not
> strictly "valid" (but it always worked in practice with existing
> encoders). Reference decoder also loops indefinitely until it sees stop
> bit here.

OK. Out of curiosity, what are classical values there? "common" and max (128?).

> Parsing is validated at the end of chs_parse_band_data(), there is an
> explicit check whether overread has occured (and if it has, entire
> segment is zeroed out).

OK, so I think this patch is completely fine.

Thanks,
-- 
Christophe