GNOME Bugzilla – Bug 685890
codecparsers: h264: fix rbsp_more_data() implementation
Last modified: 2014-05-20 12:17:28 UTC
Created attachment 226177 [details] [review] codecparsers: h264: fix rbsp_more_data() implementation Hi, this patch accounts for trailing zero bits when checking for bsp_more_data(). In particular, fix an hypothetical stream whereby rbsp_more_data() is called in the following conditions for PPS header: NalReader reached position 20, 12 bits are remaining and trailing data at current byte position is c8 00. rbsp_more_data() used to return TRUE whereas it should obviously return FALSE because x8 00 represents a valid rbsp_trailing_bits() structure. The stream is not so hypothetical since this is the following one: http://samples.mplayerhq.hu/V-codecs/h264/sony-hdr-cx6-avchd-interlaced-decoding-problem/sony-hdr-cx-6-avchd-1080i-3-seconds.mts
While this patch makes sense, the while() loop seems to pass beyond byte-boundary, whereas spec is as below, which does not suggest doing so (unless there is also something general regarding trailing 0 bits elsewhere): rbsp_trailing_bits( ) { rbsp_stop_one_bit /* equal to 1 */ while (!byte_aligned()) rbsp_alignment_zero_bit /* equal to 0 */ } That said, it should indeed return FALSE in the example given above, but am not sure what to do with trailing 0-bytes.
See https://bugzilla.gnome.org/show_bug.cgi?id=721715#c4 Wouldn't it be better to behave as the reference decoder does here? What's the status of this patch/bug btw?
We have been using that by default for some time now without known regression. Theory of operations of the loop: 1. read the remaining bits in the current byte. If zero, then no stop bit yet. 2. for each iteration, we read the whole byte. If zero, still no stop bit yet. We are only interested in knowing whether there is more_data() in there. But yes, we probably don't actually need the while() loop. Just checking whether the remaining bits in the current byte is zero should be enough.
Can you update the patch like that then so it conforms more to the spec and also consider comment 1?
Looking at this one with another angle. The spec says that more_rbsp_data() should look for the *last* bit set to 1, which corresponds to the rbsp_stop_one_bit. So a loop is needed. In order to be sure that we parsed the last one bit, this reduces to (i) we parsed one bit set to 1 and (ii) all the remaining bits are set to zero. So, at the end of the day, we can conclude to more_rbsp_data() == FALSE if get_bit() == 1 and all remaining bits == 0. Thus implying more_rbsp_data() == TRUE if get_bit() == 0 or if remaining bits are not zero. So, this is what I had implemented actually, but failed to properly document that. :)
Created attachment 272573 [details] [review] codecparsers: h264: fix rbsp_more_data() implementation Updated patch against git master branch. Added comment for clarity and why it is implemented this way. Thanks.
ommit 80a51a8fab4370fb389e7780e6d5058964c34e4e Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Oct 10 16:19:49 2012 +0200 codecparsers: h264: fix rbsp_more_data() implementation. Account for trailing zero bits when checking for rbsp_more_data(). In particular, fix an hypothetical stream whereby rbsp_more_data() is called in the following conditions for PPS header: NalReader reached position 20, 12 bits are remaining and trailing data at current byte position is c8 00. rbsp_more_data() used to return TRUE whereas it should obviously return FALSE because x8 00 represents a valid rbsp_trailing_bits() structure. https://bugzilla.gnome.org/show_bug.cgi?id=685890