On 5/20/2016 10:13 AM, Christophe Gisquet wrote: > Hi, > > 2016-05-20 15:09 GMT+02:00 foo86 <fooba...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
