Fix AnnexB validation logic to work with encrypted content.

Fix AnnexB validation logic to work with encrypted content.

AVC::IsValidAnnexB() would fail if encrypted sections happened to
contain something that looked like an Annex B start code. This change
updates the H264Parser so that it will skip byte sequences that look
like start codes in the encrypted sections of the buffer.

BUG=372617
TEST=AVCConversionTests updated to include fake start codes in the
  encrypted sections.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286373

[xhwang, 2014-07-10 06:23:02]

Looking pretty good. Thanks for fixing this! Just a few suggestions/nits.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.cc
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.cc...
media/filters/h264_parser.cc:272: } while (*start_code_size == 0);
Will it be simpler and more efficient if you just call FindStartCode() in the
non-encrypted ranges?

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:347: const std::vector<SubsampleEntry>& subsamples);
nit: The style guide doesn't encourage function overloading... how about
SetEncryptedStream?

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:347: const std::vector<SubsampleEntry>& subsamples);
nit: Add include for vector.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:404: bool FindNextStartCode(const uint8* data, off_t
data_size,
It's not obvious to know the difference b/w FindStartCode and FindNextStartCode
by looking at the function names. How about FindStartCodeInClearRanges?

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:456: Ranges<const uint8*> encrypted_ranges_;
Since this range is only used for searching for start code, how about store a
|clear_ranges_| instead? Then the FindNextStartCode() function can be easier to
implement.

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc.cc#new...
media/formats/mp4/avc.cc:311: AVC::NALUIterator::NALUIterator(const uint8*
buffer, size_t size,
Remove?

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc_unitte...
File media/formats/mp4/avc_unittest.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc_unitte...
media/formats/mp4/avc_unittest.cc:120: 
remove empty lines here

[xhwang, 2014-07-14 21:03:17]

BTW, I just tested this CL with http://goo.gl/1oGQfZ and I am still seeing:
[16134:16134:0714/140042:FATAL:mp4_stream_parser.cc(385)] Check failed:
AVC::IsValidAnnexB(*frame_bu
f, *subsamples).

[acolwell GONE FROM CHROMIUM, 2014-07-15 22:10:44]

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.cc
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.cc...
media/filters/h264_parser.cc:272: } while (*start_code_size == 0);
On 2014/07/10 06:23:01, xhwang wrote:
> Will it be simpler and more efficient if you just call FindStartCode() in the
> non-encrypted ranges?

I've changed the code around to search clear ranges. Let me know what you think.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:347: const std::vector<SubsampleEntry>& subsamples);
On 2014/07/10 06:23:01, xhwang wrote:
> nit: The style guide doesn't encourage function overloading... how about
> SetEncryptedStream?

Done.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:347: const std::vector<SubsampleEntry>& subsamples);
On 2014/07/10 06:23:01, xhwang wrote:
> nit: Add include for vector.

Done.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:404: bool FindNextStartCode(const uint8* data, off_t
data_size,
On 2014/07/10 06:23:01, xhwang wrote:
> It's not obvious to know the difference b/w FindStartCode and
FindNextStartCode
> by looking at the function names. How about FindStartCodeInClearRanges?

Done.

https://codereview.chromium.org/379983002/diff/1/media/filters/h264_parser.h#...
media/filters/h264_parser.h:456: Ranges<const uint8*> encrypted_ranges_;
On 2014/07/10 06:23:01, xhwang wrote:
> Since this range is only used for searching for start code, how about store a
> |clear_ranges_| instead? Then the FindNextStartCode() function can be easier
to
> implement.

Done.

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc.cc#new...
media/formats/mp4/avc.cc:311: AVC::NALUIterator::NALUIterator(const uint8*
buffer, size_t size,
On 2014/07/10 06:23:01, xhwang wrote:
> Remove?
Oops. Done.

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc_unitte...
File media/formats/mp4/avc_unittest.cc (right):

https://codereview.chromium.org/379983002/diff/1/media/formats/mp4/avc_unitte...
media/formats/mp4/avc_unittest.cc:120: 
On 2014/07/10 06:23:01, xhwang wrote:
> remove empty lines here

Done.

[acolwell GONE FROM CHROMIUM, 2014-07-15 22:45:29]

On 2014/07/14 21:03:17, xhwang wrote:
> BTW, I just tested this CL with http://goo.gl/1oGQfZ and I am still seeing:
> [16134:16134:0714/140042:FATAL:mp4_stream_parser.cc(385)] Check failed:
> AVC::IsValidAnnexB(*frame_bu
> f, *subsamples).

It looks like this content isn't providing subsample information so I believe
the DCHECK is valid since a PPS is appearing after the first VCL.

[xhwang, 2014-07-16 05:06:13]

The new code still seems complicated. I guess this is due to how FindStartCode()
works. Can you add some comments briefly explaining how the logic works? Thanks!

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:156: const uint8* end = std::min(start +
subsamples[i].clear_bytes, stream_end);
It seems possible that
start + subsamples[i].clear_bytes > stream_end
?
Is that caused by appending partial buffer? In that case, how will the remaining
encrypted bytes be excluded? Is this what the clear_range_index_ is used for?

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:256: return FindStartCode(data, data_size, offset,
start_code_size);
If there's no clear ranges, we don't need to search?

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:271: }
This seems complicated. Can you add some comments explaining the basics how it
works?

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parser.h
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.h:405: bool FindStartCodeInClearRanges(const uint8*
data, off_t data_size,
Comment about the meaning of the returned value.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.h:458: size_t clear_range_index_;
Comment this variable.

[damienv1, 2014-07-16 14:33:17]

I think the code could be a bit simpler.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:250: bool H264Parser::FindStartCodeInClearRanges(
The likelihood of a start code emulation (although it can exist) is pretty low.
So why not having a linear search (just like FindStartCode) and when we get a
start code, we could have a small utility function which checks given the offset
of the start code whether the start code overlaps with some encrypted content
(using the subsamples vector, no need to have additional clear ranges).

I agree that if there was a fake start codes, this would increase the complexity
but in this case, it seems that code simplicity takes precedence.

[acolwell GONE FROM CHROMIUM, 2014-07-25 16:43:26]

On 2014/07/16 14:33:17, damienv1 wrote:
> I think the code could be a bit simpler.
> 
>
https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
> File media/filters/h264_parser.cc (right):
> 
>
https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
> media/filters/h264_parser.cc:250: bool H264Parser::FindStartCodeInClearRanges(
> The likelihood of a start code emulation (although it can exist) is pretty
low.
> So why not having a linear search (just like FindStartCode) and when we get a
> start code, we could have a small utility function which checks given the
offset
> of the start code whether the start code overlaps with some encrypted content
> (using the subsamples vector, no need to have additional clear ranges).
> 
> I agree that if there was a fake start codes, this would increase the
complexity
> but in this case, it seems that code simplicity takes precedence.

This is essentially what I had in Patch Set 1, but xhwang thought it was too
complex. Personally I liked that implementation better because I thought it was
easier to follow and as you say only comes into play if there happens to be fake
start codes in encrypted sections.

[xhwang, 2014-07-25 17:04:24]

On 2014/07/25 16:43:26, acolwell wrote:
> On 2014/07/16 14:33:17, damienv1 wrote:
> > I think the code could be a bit simpler.
> > 
> >
>
https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
> > File media/filters/h264_parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
> > media/filters/h264_parser.cc:250: bool
H264Parser::FindStartCodeInClearRanges(
> > The likelihood of a start code emulation (although it can exist) is pretty
> low.
> > So why not having a linear search (just like FindStartCode) and when we get
a
> > start code, we could have a small utility function which checks given the
> offset
> > of the start code whether the start code overlaps with some encrypted
content
> > (using the subsamples vector, no need to have additional clear ranges).
> > 
> > I agree that if there was a fake start codes, this would increase the
> complexity
> > but in this case, it seems that code simplicity takes precedence.
> 
> This is essentially what I had in Patch Set 1, but xhwang thought it was too
> complex. Personally I liked that implementation better because I thought it
was
> easier to follow and as you say only comes into play if there happens to be
fake
> start codes in encrypted sections.

Yeah, I agree PS1 looks much simpler now :) I thought only searching in the
clear bytes might be more efficient, but given all the complexities in PS3,
I agree it might not worth the effort.

[acolwell GONE FROM CHROMIUM, 2014-07-28 19:43:12]

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:156: const uint8* end = std::min(start +
subsamples[i].clear_bytes, stream_end);
On 2014/07/16 05:06:13, xhwang wrote:
> It seems possible that
> start + subsamples[i].clear_bytes > stream_end
> ?
This is to protect this code from invalid subsample data in the file.

> Is that caused by appending partial buffer? In that case, how will the
remaining
> encrypted bytes be excluded? Is this what the clear_range_index_ is used for?
Part of this comment no longer applies. Basically we only keep information for
regions that actually exist inside the buffer. Anything outside of that doesn't
really matter since it would result in an invalid memory access if it was
allowed to occur.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:250: bool H264Parser::FindStartCodeInClearRanges(
On 2014/07/16 14:33:17, damienv1 wrote:
> The likelihood of a start code emulation (although it can exist) is pretty
low.
> So why not having a linear search (just like FindStartCode) and when we get a
> start code, we could have a small utility function which checks given the
offset
> of the start code whether the start code overlaps with some encrypted content
> (using the subsamples vector, no need to have additional clear ranges).
> 
> I agree that if there was a fake start codes, this would increase the
complexity
> but in this case, it seems that code simplicity takes precedence.

I've restored the code to what I had in Patch Set 1. This is a linear scan like
you suggested and I think is easier to follow.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:256: return FindStartCode(data, data_size, offset,
start_code_size);
On 2014/07/16 05:06:13, xhwang wrote:
> If there's no clear ranges, we don't need to search?
Yes. A lack of clear ranges, or now encrypted ranges means that none of the data
is encrypted. Now that this is encrypted_ranges_ again, I think that is a little
more intuitive.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.cc:271: }
On 2014/07/16 05:06:13, xhwang wrote:
> This seems complicated. Can you add some comments explaining the basics how it
> works?

Code is no longer here anymore.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parser.h
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.h:405: bool FindStartCodeInClearRanges(const uint8*
data, off_t data_size,
On 2014/07/16 05:06:13, xhwang wrote:
> Comment about the meaning of the returned value.

Done.

https://codereview.chromium.org/379983002/diff/40001/media/filters/h264_parse...
media/filters/h264_parser.h:458: size_t clear_range_index_;
On 2014/07/16 05:06:13, xhwang wrote:
> Comment this variable.
Variable no longer exists.

[xhwang, 2014-07-29 18:28:53]

lgtm, thanks!

https://codereview.chromium.org/379983002/diff/60001/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/379983002/diff/60001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:227: 
extra line not needed?

[acolwell GONE FROM CHROMIUM, 2014-07-29 19:09:27]

https://codereview.chromium.org/379983002/diff/60001/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/379983002/diff/60001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:227: 
On 2014/07/29 18:28:52, xhwang wrote:
> extra line not needed?

Done.

[acolwell GONE FROM CHROMIUM, 2014-07-29 19:09:30]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-07-29 19:10:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/379983002/80001

[commit-bot: I haz the power, 2014-07-30 01:25:38]

Change committed as 286373