On Fri, May 20, 2016 at 02:35:53PM +0200, Christophe Gisquet wrote: > 2016-05-13 11:48 GMT+02:00 foo86 <fooba...@gmail.com>: > > - unsigned int v = get_unary(gb, 1, 128); > > + unsigned int v = get_unary(gb, 1, get_bits_left(gb)); > > 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). > 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. 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). _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
