Disregard trailing null bytes when locating RBSP stop bits.

Disregard trailing null bytes when locating RBSP stop bits.

This is a more faithful implementation of more_rbsp_data().

There are two ways this could happen despite being invalid:
  (1) H264Parser does not strip trailing null bytes from NAL units as it
      should. This is generally not a problem because the conversion to
      Annex B won't introduce them either.
  (2) There could be trailing null bytes before the Annex B conversion.
      This isn't valid (at least for PPS NAL units), but the referenced
      bug includes a sample where the PPS in the avcC record includes a
      trailing null byte.

BUG=479229
Committed: https://crrev.com/86e5caf8d9d8490f645757bc886b942f10d0f7b1
Cr-Commit-Position: refs/heads/master@{#381879}

[sandersd (OOO until July 31), 2015-04-23 21:46:22]

sandersd@chromium.org changed reviewers:
+ posciak@chromium.org

[sandersd (OOO until July 31), 2015-04-23 21:48:13]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 21:49:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1107593004/1

[commit-bot: I haz the power, 2015-04-23 23:09:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 23:09:33]

Dry run: This issue passed the CQ dry run.

[Pawel Osciak, 2015-04-24 07:23:07]

The trailing null bytes are not a part of the NALU per spec, so after giving it
some more thought, perhaps we could consider something slightly different
please?

To make sure we get the correct NALU size, and also to utilize existing code
parsing byte by byte in H264Parser::FindStartCode(), perhaps we could calculate
the number of leading null bytes that come before the next NALU start code in
FindStartCode() (that's where we look for the next NALU's start code)? And then
subtract that from the nalu_size_without_start_code in LocateNALU()? We have
offset there, which we could use to calculate leading null bytes, which would be
trailing null bytes when looking for end of current NALU/next NALU in
LocateNALU()?

And in case a start code of the next NALU in the buffer was not found (second
FindStartCodeInClearRanges() failed in LocaleNALU()), we might need another loop
in there, going from the end of the buffer and decrementing size/discarding null
bytes...

[sandersd (OOO until July 31), 2015-04-24 18:24:16]

On 2015/04/24 07:23:07, Pawel Osciak wrote:
> The trailing null bytes are not a part of the NALU per spec, so after giving
it
> some more thought, perhaps we could consider something slightly different
> please?

That's not quite true; at least for image slices null bytes (well, pairs of
them) are part of the NALU.

While null bytes are not spec'd for PPS, more_rbsp_data() *is* spec'd to handle
them; this CL is not wrong. 

> To make sure we get the correct NALU size, and also to utilize existing code
> parsing byte by byte in H264Parser::FindStartCode(), [...]

This would match the spec better, but with these drawbacks:
  - Annex B specifies that null bytes are not stripped from the last NALU in a
packet, unless there are at least three of them.
  - It requires extra processing for every packet, not just the ones where the
trailing bytes matter.
  - Because our Annex B data is coming from our converter for MP4 files, it's
entirely unnecessary in that case.

The short is that I don't like any of the solutions, but I'm not convinced that
this particular solution is inferior.

[Pawel Osciak, 2015-04-27 01:06:07]

On 2015/04/24 18:24:16, sandersd wrote:
> On 2015/04/24 07:23:07, Pawel Osciak wrote:
> > The trailing null bytes are not a part of the NALU per spec, so after giving
> it
> > some more thought, perhaps we could consider something slightly different
> > please?
> 
> That's not quite true; at least for image slices null bytes (well, pairs of
> them) are part of the NALU.

Null bytes in general, or trailing null/zero bytes in particular? Per spec, a
NALU consists of:

leading zero bytes, start code prefix, nal_unit, trailing zero bytes

The trailing zero bytes do not belong to the the nal_unit syntax element.

> While null bytes are not spec'd for PPS, more_rbsp_data() *is* spec'd to
handle
> them; this CL is not wrong. 

My point is different. Non-RBPS bytes are not a part of a NALU. While you are
right that this CL is correct for HasMoreRBSPData(), in other places (where we
don't call HasMoreRBSPData()) we are treating the trailing zero bytes as part of
the NALU (e.g. we add them to the nalu size).

Since we are already fixing this, I'm suggesting that we could fix that as well,
not only HasMoreRBSPData(), but also the fact that we give the wrong NALU size
for example. And fixing that would also fix HasMoreRBSPData().
 
> > To make sure we get the correct NALU size, and also to utilize existing code
> > parsing byte by byte in H264Parser::FindStartCode(), [...]
> 
> This would match the spec better, but with these drawbacks:
>   - Annex B specifies that null bytes are not stripped from the last NALU in a
> packet, unless there are at least three of them.

Could you point me to the exact page/line in the spec that says that? I could
not find that in Annex B.

>   - It requires extra processing for every packet, not just the ones where the
> trailing bytes matter.

My point is, it actually does matter in other cases. For example, we pass nal
unit size to hardware decoders. Right now it's incorrect unless we fix that.

>   - Because our Annex B data is coming from our converter for MP4 files, it's
> entirely unnecessary in that case.

Sorry, what is?

[sandersd (OOO until July 31), 2015-04-27 18:09:26]

On 2015/04/27 01:06:07, Pawel Osciak wrote:
> On 2015/04/24 18:24:16, sandersd wrote:
> Null bytes in general, or trailing null/zero bytes in particular? Per spec, a
> NALU consists of:
> 
> leading zero bytes, start code prefix, nal_unit, trailing zero bytes
> 
> The trailing zero bytes do not belong to the the nal_unit syntax element.

Sorry, the misunderstanding here is that cabac_zero_word bytes, which occur
after the alignment bits (7.3.2.10), would have emulation prevention bytes.

> Since we are already fixing this, I'm suggesting that we could fix that as
well,
> not only HasMoreRBSPData(), but also the fact that we give the wrong NALU size
> for example. And fixing that would also fix HasMoreRBSPData().

This is true, but has more risks and performance impact, see below.

> >   - Annex B specifies that null bytes are not stripped from the last NALU in
a
> > packet, unless there are at least three of them.
> 
> Could you point me to the exact page/line in the spec that says that? I could
> not find that in Annex B.

This follows from the definition of NumBytesInNALunit (B.2.3).

> >   - Because our Annex B data is coming from our converter for MP4 files,
it's
> > entirely unnecessary in that case.
> 
> Sorry, what is?

Stripping null bytes. Since MP4 data is coming from AVCC-style NAL records (that
is, packets with length headers), extra trailing bytes are not a concern. In
this case the Annex B stream is constructed by our own code, and we don't add
any null bytes.

The only reason the referenced bug can occur is because the avcC atom is
actually invalid (at least as I understand it), not because of a particular
parsing bug. It could equally be fixed in the bitstream converter, where we turn
avcC into SPS/PPS, or we could decide not to fix it at all (not likely since
FFmpeg accepts it).

[Pawel Osciak, 2015-04-30 11:24:31]

On 2015/04/27 18:09:26, sandersd wrote:
> On 2015/04/27 01:06:07, Pawel Osciak wrote:
> > On 2015/04/24 18:24:16, sandersd wrote:
> > Null bytes in general, or trailing null/zero bytes in particular? Per spec,
a
> > NALU consists of:
> > 
> > leading zero bytes, start code prefix, nal_unit, trailing zero bytes
> > 
> > The trailing zero bytes do not belong to the the nal_unit syntax element.
> 
> Sorry, the misunderstanding here is that cabac_zero_word bytes, which occur
> after the alignment bits (7.3.2.10), would have emulation prevention bytes.
> 
> > Since we are already fixing this, I'm suggesting that we could fix that as
> well,
> > not only HasMoreRBSPData(), but also the fact that we give the wrong NALU
size
> > for example. And fixing that would also fix HasMoreRBSPData().
> 
> This is true, but has more risks and performance impact, see below.
> 
> > >   - Annex B specifies that null bytes are not stripped from the last NALU
in
> a
> > > packet, unless there are at least three of them.
> > 
> > Could you point me to the exact page/line in the spec that says that? I
could
> > not find that in Annex B.
> 
> This follows from the definition of NumBytesInNALunit (B.2.3).

Thanks, hm, I guess there is nal_unit(), and byte_stream_nal_unit().

> 
> > >   - Because our Annex B data is coming from our converter for MP4 files,
> it's
> > > entirely unnecessary in that case.
> > 
> > Sorry, what is?
> 
> Stripping null bytes. Since MP4 data is coming from AVCC-style NAL records
(that
> is, packets with length headers), extra trailing bytes are not a concern. In
> this case the Annex B stream is constructed by our own code, and we don't add
> any null bytes.
> 
> The only reason the referenced bug can occur is because the avcC atom is
> actually invalid (at least as I understand it), not because of a particular
> parsing bug. It could equally be fixed in the bitstream converter, where we
turn
> avcC into SPS/PPS, or we could decide not to fix it at all (not likely since
> FFmpeg accepts it).

Would you prefer to have the fixup here or in the bitstream converter? Perhaps
having it in bitstream converter would be less of a performance hit if we are
already going over the bytes in there? I wouldn't be strongly against here
however. Up to you.

[sandersd (OOO until July 31), 2015-04-30 22:59:21]

> Would you prefer to have the fixup here or in the bitstream converter? Perhaps
> having it in bitstream converter would be less of a performance hit if we are
> already going over the bytes in there? I wouldn't be strongly against here
> however. Up to you.

That's a tricky one. I think the answer is here, reasoning as follows:
  - A trailing null byte could be invalid (extra padding) or part of the data
(stop bit missing).
  - Both are parsed "correctly" by this CL, but in the converter we don't parse
the PPS and can't tell the difference.
  - If we can't tell the difference, we will handle the missing-stop-bit case
wrong.

There are arguments for doing it in the bitstream converter though:
  - Only have to do it once, not once per frame (although with some work we can
skip parsing PPS per-frame too).
  - If necessary, we could parse the SPS and PPS in the converter and do this
perfectly.

[sandersd (OOO until July 31), 2016-02-19 23:01:47]

On 2015/04/30 22:59:21, sandersd wrote:
> > Would you prefer to have the fixup here or in the bitstream converter?
Perhaps
> > having it in bitstream converter would be less of a performance hit if we
are
> > already going over the bytes in there? I wouldn't be strongly against here
> > however. Up to you.
> 
> That's a tricky one. I think the answer is here, reasoning as follows:
>   - A trailing null byte could be invalid (extra padding) or part of the data
> (stop bit missing).
>   - Both are parsed "correctly" by this CL, but in the converter we don't
parse
> the PPS and can't tell the difference.
>   - If we can't tell the difference, we will handle the missing-stop-bit case
> wrong.
> 
> There are arguments for doing it in the bitstream converter though:
>   - Only have to do it once, not once per frame (although with some work we
can
> skip parsing PPS per-frame too).
>   - If necessary, we could parse the SPS and PPS in the converter and do this
> perfectly.

posciak@: Ping due to crbug.com/585452. I'll rebase this to be current; do you
want us to consider changing the approach?

[sandersd (OOO until July 31), 2016-02-29 20:44:49]

PTAL.

[Pawel Osciak, 2016-03-01 08:56:22]

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
File media/filters/h264_bit_reader.cc (right):

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
media/filters/h264_bit_reader.cc:107: // them when there is no data. (At least
for PPS NAL units, which is the only
I would perhaps remove the comment in parenthesis, we are not doing anything
specific only to PPS NALUs I believe.

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
media/filters/h264_bit_reader.cc:109: if (bytes_left_) {
This if could be dropped?

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_to_a...
File media/filters/h264_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_to_a...
media/filters/h264_to_annex_b_bitstream_converter.cc:269: uint32_t size =
param_set.size();
s/uint32_t/size_t/

[sandersd (OOO until July 31), 2016-03-17 19:07:51]

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
File media/filters/h264_bit_reader.cc (right):

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
media/filters/h264_bit_reader.cc:107: // them when there is no data. (At least
for PPS NAL units, which is the only
On 2016/03/01 08:56:21, Pawel Osciak wrote:
> I would perhaps remove the comment in parenthesis, we are not doing anything
> specific only to PPS NALUs I believe.

Done.

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_bit_...
media/filters/h264_bit_reader.cc:109: if (bytes_left_) {
On 2016/03/01 08:56:21, Pawel Osciak wrote:
> This if could be dropped?

Done.

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_to_a...
File media/filters/h264_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/1107593004/diff/20001/media/filters/h264_to_a...
media/filters/h264_to_annex_b_bitstream_converter.cc:269: uint32_t size =
param_set.size();
On 2016/03/01 08:56:21, Pawel Osciak wrote:
> s/uint32_t/size_t/

Done.

[Pawel Osciak, 2016-03-18 01:59:10]

lgtm

[sandersd (OOO until July 31), 2016-03-18 02:02:27]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-03-18 02:02:29]

The CQ bit was unchecked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-03-18 02:02:40]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-03-18 02:03:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1107593004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1107593004/40001

[commit-bot: I haz the power, 2016-03-18 03:17:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-18 03:18:48]

Description was changed from

==========
Disregard trailing null bytes when locating RBSP stop bits.

This is a more faithful implementation of more_rbsp_data().

There are two ways this could happen despite being invalid:
  (1) H264Parser does not strip trailing null bytes from NAL units as it
      should. This is generally not a problem because the conversion to
      Annex B won't introduce them either.
  (2) There could be trailing null bytes before the Annex B conversion.
      This isn't valid (at least for PPS NAL units), but the referenced
      bug includes a sample where the PPS in the avcC record includes a
      trailing null byte.

BUG=479229
==========

to

==========
Disregard trailing null bytes when locating RBSP stop bits.

This is a more faithful implementation of more_rbsp_data().

There are two ways this could happen despite being invalid:
  (1) H264Parser does not strip trailing null bytes from NAL units as it
      should. This is generally not a problem because the conversion to
      Annex B won't introduce them either.
  (2) There could be trailing null bytes before the Annex B conversion.
      This isn't valid (at least for PPS NAL units), but the referenced
      bug includes a sample where the PPS in the avcC record includes a
      trailing null byte.

BUG=479229

Committed: https://crrev.com/86e5caf8d9d8490f645757bc886b942f10d0f7b1
Cr-Commit-Position: refs/heads/master@{#381879}
==========

[commit-bot: I haz the power, 2016-03-18 03:18:49]

Patchset 3 (id:??) landed as
https://crrev.com/86e5caf8d9d8490f645757bc886b942f10d0f7b1
Cr-Commit-Position: refs/heads/master@{#381879}