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

[James Almer]

On 5/20/2016 10:13 AM, Christophe Gisquet wrote:
> 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,

Pushed.