has_more_rbsp_data is broken / fix trailing bits handling

[scottlamb]

The problem

The call to PicParameterSetExtra::read is commented-out/broken.

h264-reader/src/nal/pps.rs

Line 252 in aa5bb36

extension: None, // TODO: buggy?: PicParameterSetExtra::read(&mut r, seq_parameter_set)?,

h264-reader/src/nal/pps.rs

Lines 173 to 176 in aa5bb36

	impl PicParameterSetExtra {
	fn read(r: &mut RbspBitReader<'_>, sps: &sps::SeqParameterSet) -> Result<Option<PicParameterSetExtra>,PpsError> {
	Ok(if r.has_more_rbsp_data() {
	let transform_8x8_mode_flag = r.read_bool()?;

I think I understand the problem now. Before I changed it, has_more_rbsp_data did the following:

h264-reader/src/rbsp.rs

Lines 287 to 289 in c96301c

	pub fn has_more_rbsp_data(&self) -> bool {
	self.position() < self.total_size as u64
	}

(position and total_size are in bits. The current version is harder to read but similar in effect.)

This isn't right: it says if there are more bits left in the RBSP, which sounds reasonable, but the spec says it should mean has more non-trailing bits (Rec. ITU-T H.264 (03/2010) section 7.2):

more_rbsp_data( ) is specified as follows.

• If there is no more data in the RBSP, the return value of more_rbsp_data( ) is equal to FALSE.
• Otherwise, the RBSP data is searched for the last (least significant, right-most) bit equal to 1 that is present in the RBSP. Given the position of this bit, which is the first bit (rbsp_stop_one_bit) of the rbsp_trailing_bits( ) syntax structure, the following applies.
  • If there is more data in an RBSP before the rbsp_trailing_bits( ) syntax structure, the return value of more_rbsp_data( ) is equal to TRUE.
  • Otherwise, the return value of more_rbsp_data( ) is equal to FALSE.

The method for enabling determination of whether there is more data in the RBSP is specified by the application
(or in Annex B for applications that use the byte stream format).

For most NALs, the trailing bits are guaranteed to be only in the last byte in the RBSP. For slice NALs, there are sometimes cabac_zero_words afterward. (See rbsp_slice_trailing_bits, section 7.3.2.10.) In either case, it should stop at the last one bit of the RBSP.

Better behavior

• At the very least it should be possible to parse the PPS properly. I think that means has_more_rbsp_data should return the correct result on a completely-buffered non-slice NAL. Ideally it always returns the correct result, including WouldBlock in ambiguous cases on partially-buffered NALs.
• As for other methods,
  • IMHO the most understandable thing would be if all BitStreamReader functions were aware of the trailing bits and returned EOF on hitting them, or (when parsing a partially-buffered NAL) WouldBlock on the last one bit we know of. This might help catch bugs in parsing at least. But it doesn't seem straightforward to implement (see below).
  • Maybe it'd be easier to implement and almost as good for there to be a finish method which errors if it's not positioned exactly at the trailing bits.
  • or maybe I'm overthinking this and no one cares?

Implementation ideas

Unfortunately bitstream_io::BitRead doesn't have a way to peek at the queued unaligned bits non-destructively or when unaligned to access the underlying reader (to check its position). Those might be reasonable feature requests, or we could use bitstream_io::BitQueue directly.

I don't see a straightforward, efficient way to just check there's still a one bit left in the stream via readahead. bitstream_io::BitQueue only supports storing 32 bits, and I think BitReader might be using all of that already. edit: no, I got confused. It supports any numeric type (up to u128). We could probably do this by using it directly but it might require re-implementing more of BitReader than I was hoping for. edit 2: no, but anyway this would require unbounded lookahead, as the RBSP can have several zero bytes in a row. That's no good.

We could look backward in the RBSP to find the trailing bits position (as the spec mentions in 7.4.1.1: "identification of the end of the SODB within the NAL unit by searching the RBSP for the rbsp_stop_one_bit starting at the end of the RBSP"). It'd mean adjusting my interface proposal in #4 because getting the NAL bytes via a BufStream doesn't allow this. And looking backward is slightly messy on slice NALs because there can be an emulation prevention three byte in the CABAC words (see the note on H.264 7.4.2.10) and those don't count. (But we only really care about the headers of slice NALs anyway, not consuming those NALs fully, so maybe it doesn't matter.) But it's at least possible to find the right bit in the NAL bit stream. And I think we could map that into rbsp::RbspBitReader calls doing the right thing if bitstream_io would just let us access the reader (to check if we've hit that NAL byte position in question) and count the number of queued bits.

The finish method seems relatively straightforward to do without even any changes to bitstream_io::BitRead's interface. There's a BitRead::byte_aligned; we can get the bits until then and make sure the first is one and the rest are zero. Then we can use BitReader::into_reader to ensure the remaining RBSP bytes (if any) are zero.

[scottlamb]

Hmm, here's a thought. If all the readers were #![derive(Clone) (the ReadBuf for the NAL, the ReadBuf for the RBSP, and the bitstream_io::BitReader), has_more_rbsp_data would be easy. It just needs to clone and look for two upcoming one bits [edit: minor correction: a one bit that isn't the very next bit]. The NAL and RBSP readers would be easy to make Clone. Making bitstream_io::BitReader Clone would be be a feature request for bitstream_io but I don't think they'd object. iirc just a plain #![derive(Clone)] has an automatic where R: Clone on it so this would be a one-line change to bitstream_io.

finish isn't too hard either as mentioned above.

I still don't see a way I'm in love with of having all the methods be trailing bit-aware. There's the seeking backward I described but that's kind of complicated. We could also have a second RBSP reader that marks the point where we're sure there's another one in the stream and compare their positions, but that's still complicated, and it doubles the RBSP decoding overhead. But maybe it's just not important to have them all be trailing bit-aware.