Fix inserting SPS/PPS after AUD NALU

Fix inserting SPS/PPS after AUD NALU

Currently we fail to insert SPS/PPS if the key frame starts with AUD NALU and has one or more clear-text NALUs after that (I've seen this issue with a stream where key frames start with AUD+SEI NALU).

BUG=417562
Committed: https://crrev.com/88ea6544feb5796ba44cfde95a9a170472504cf1
Cr-Commit-Position: refs/heads/master@{#298967}

[servolk, 2014-09-20 00:25:43]

servolk@chromium.org changed reviewers:
+ acolwell@chromium.org, wolenetz@chromium.org

[wolenetz, 2014-09-25 00:41:53]

Thanks for putting this CL together. It appears to comply with  Section
7.4.1.2.3 of ISO/IEC 14496-10 and simplifies the unnecessary encrypted subsample
logic in SPS/PPS conversion to AnnexB.

Please file a bug and reference it in the CL description.
I would also like to see some test coverage around this, too. Would that be
possible?

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

https://codereview.chromium.org/591713002/diff/20001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:95: DCHECK(config_insert_point <=
(*subsamples)[0].clear_bytes);
If this DCHECK's condition fails in release build, should processing continue or
should we return false here?

[servolk, 2014-09-30 21:44:00]

On 2014/09/25 00:41:53, wolenetz wrote:
> Thanks for putting this CL together. It appears to comply with  Section
> 7.4.1.2.3 of ISO/IEC 14496-10 and simplifies the unnecessary encrypted
subsample
> logic in SPS/PPS conversion to AnnexB.
> 
> Please file a bug and reference it in the CL description.
> I would also like to see some test coverage around this, too. Would that be
> possible?
Well, sure, but how should I add a test for this? So far we have only seen this
issue with one video stream on Chromecast. But that video stream is using
PlayReady DRM, so we won't be able to play it even after this is fixed. And more
generally - when adding a unit test for a case like this, am I expected to use a
short piece of actual content, e.g. some .mp4 file. Or should I just embed the
problematic input sample (which in this case is can be made fairly short, a few
hundred bytes I think)?

> 
> https://codereview.chromium.org/591713002/diff/20001/media/formats/mp4/avc.cc
> File media/formats/mp4/avc.cc (right):
> 
>
https://codereview.chromium.org/591713002/diff/20001/media/formats/mp4/avc.cc...
> media/formats/mp4/avc.cc:95: DCHECK(config_insert_point <=
> (*subsamples)[0].clear_bytes);
> If this DCHECK's condition fails in release build, should processing continue
or
> should we return false here?
Yep, we should probably return false. If this DCHECK fails, it means we are
trying to insert PPS/SPS after the clear part of the first subsample, which
would be really strange, since we can't parse that far without decrypting the
remainder of the first subsample.

[wolenetz, 2014-09-30 22:18:18]

On 2014/09/30 21:44:00, servolk wrote:
> On 2014/09/25 00:41:53, wolenetz wrote:
> > I would also like to see some test coverage around this, too. Would that be
> > possible?
> Well, sure, but how should I add a test for this? So far we have only seen
this
> issue with one video stream on Chromecast. But that video stream is using
> PlayReady DRM, so we won't be able to play it even after this is fixed. And
more
> generally - when adding a unit test for a case like this, am I expected to use
a
> short piece of actual content, e.g. some .mp4 file. Or should I just embed the
> problematic input sample (which in this case is can be made fairly short, a
few
> hundred bytes I think)?

Please pardon if I'm misunderstanding something in your question.

In preference order:
1. If you can isolate the repro data to a publicly releasable mp4 file, great!
(This is pretty much what the existing mp4_stream_parser_unittests do).
2. A hybrid case where an mp4 preliminary is appended followed by explicit short
segment of info to trigger AUD+SEI code path.
3. A fully explicit DecoderBuffer with content created from byte array inside
mp4_stream_parser_unittest.cc.

[acolwell GONE FROM CHROMIUM, 2014-09-30 22:21:18]

acolwell@chromium.org changed reviewers:
- acolwell@chromium.org

[servolk, 2014-10-04 00:17:19]

On 2014/09/30 22:18:18, wolenetz wrote:
> On 2014/09/30 21:44:00, servolk wrote:
> > On 2014/09/25 00:41:53, wolenetz wrote:
> > > I would also like to see some test coverage around this, too. Would that
be
> > > possible?
> > Well, sure, but how should I add a test for this? So far we have only seen
> this
> > issue with one video stream on Chromecast. But that video stream is using
> > PlayReady DRM, so we won't be able to play it even after this is fixed. And
> more
> > generally - when adding a unit test for a case like this, am I expected to
use
> a
> > short piece of actual content, e.g. some .mp4 file. Or should I just embed
the
> > problematic input sample (which in this case is can be made fairly short, a
> few
> > hundred bytes I think)?
> 
> Please pardon if I'm misunderstanding something in your question.
> 
> In preference order:
> 1. If you can isolate the repro data to a publicly releasable mp4 file, great!
> (This is pretty much what the existing mp4_stream_parser_unittests do).
> 2. A hybrid case where an mp4 preliminary is appended followed by explicit
short
> segment of info to trigger AUD+SEI code path.
> 3. A fully explicit DecoderBuffer with content created from byte array inside
> mp4_stream_parser_unittest.cc.

Ok, I couldn't find an easy way to generate a test file, and the repro that we
have uses PlayReady-protected content and I'm not clear on it's licensing
status.
But I looked at the existing AVC/h264 tests, and luckily we have some useful
helpers in there that allow generating arbitrary valid-looking h264 bitstream. I
think we can extend the supported syntax in there a little bit to allow placing
multiple NALUs in the same subsample:
https://codereview.chromium.org/626193003/

[wolenetz, 2014-10-08 00:58:16]

On 2014/10/04 00:17:19, servolk wrote:
> On 2014/09/30 22:18:18, wolenetz wrote:
> > On 2014/09/30 21:44:00, servolk wrote:
> > > On 2014/09/25 00:41:53, wolenetz wrote:
> > > > I would also like to see some test coverage around this, too. Would that
> be
> > > > possible?
> > > Well, sure, but how should I add a test for this? So far we have only seen
> > this
> > > issue with one video stream on Chromecast. But that video stream is using
> > > PlayReady DRM, so we won't be able to play it even after this is fixed.
And
> > more
> > > generally - when adding a unit test for a case like this, am I expected to
> use
> > a
> > > short piece of actual content, e.g. some .mp4 file. Or should I just embed
> the
> > > problematic input sample (which in this case is can be made fairly short,
a
> > few
> > > hundred bytes I think)?
> > 
> > Please pardon if I'm misunderstanding something in your question.
> > 
> > In preference order:
> > 1. If you can isolate the repro data to a publicly releasable mp4 file,
great!
> > (This is pretty much what the existing mp4_stream_parser_unittests do).
> > 2. A hybrid case where an mp4 preliminary is appended followed by explicit
> short
> > segment of info to trigger AUD+SEI code path.
> > 3. A fully explicit DecoderBuffer with content created from byte array
inside
> > mp4_stream_parser_unittest.cc.
> 
> Ok, I couldn't find an easy way to generate a test file, and the repro that we
> have uses PlayReady-protected content and I'm not clear on it's licensing
> status.
> But I looked at the existing AVC/h264 tests, and luckily we have some useful
> helpers in there that allow generating arbitrary valid-looking h264 bitstream.
I
> think we can extend the supported syntax in there a little bit to allow
placing
> multiple NALUs in the same subsample:
> https://codereview.chromium.org/626193003/

sgtm. Thanks for looking into adding tests.

[servolk, 2014-10-08 01:37:25]

On 2014/10/08 00:58:16, wolenetz wrote:
> On 2014/10/04 00:17:19, servolk wrote:
> > On 2014/09/30 22:18:18, wolenetz wrote:
> > > On 2014/09/30 21:44:00, servolk wrote:
> > > > On 2014/09/25 00:41:53, wolenetz wrote:
> > > > > I would also like to see some test coverage around this, too. Would
that
> > be
> > > > > possible?
> > > > Well, sure, but how should I add a test for this? So far we have only
seen
> > > this
> > > > issue with one video stream on Chromecast. But that video stream is
using
> > > > PlayReady DRM, so we won't be able to play it even after this is fixed.
> And
> > > more
> > > > generally - when adding a unit test for a case like this, am I expected
to
> > use
> > > a
> > > > short piece of actual content, e.g. some .mp4 file. Or should I just
embed
> > the
> > > > problematic input sample (which in this case is can be made fairly
short,
> a
> > > few
> > > > hundred bytes I think)?
> > > 
> > > Please pardon if I'm misunderstanding something in your question.
> > > 
> > > In preference order:
> > > 1. If you can isolate the repro data to a publicly releasable mp4 file,
> great!
> > > (This is pretty much what the existing mp4_stream_parser_unittests do).
> > > 2. A hybrid case where an mp4 preliminary is appended followed by explicit
> > short
> > > segment of info to trigger AUD+SEI code path.
> > > 3. A fully explicit DecoderBuffer with content created from byte array
> inside
> > > mp4_stream_parser_unittest.cc.
> > 
> > Ok, I couldn't find an easy way to generate a test file, and the repro that
we
> > have uses PlayReady-protected content and I'm not clear on it's licensing
> > status.
> > But I looked at the existing AVC/h264 tests, and luckily we have some useful
> > helpers in there that allow generating arbitrary valid-looking h264
bitstream.
> I
> > think we can extend the supported syntax in there a little bit to allow
> placing
> > multiple NALUs in the same subsample:
> > https://codereview.chromium.org/626193003/
> 
> sgtm. Thanks for looking into adding tests.

Ok, now that the extended syntax for unit tests has landed, I've updated this CL
and added a unit test covering the case that we are handling here.
A few more comments:
1. I've moved the FindSubsampleIndex from test code to avc.cc, it's useful there
as well.
2. When I was doing some testing around this CL I was getting a bit unexpected
results from my earlier unit test attempts. But after debugging h264 bitstream
parser I've updated the unit test and made slight modifications in the param
insertion logic instead of that earlier DCHECK that ensured we always insert
PPS/SPS into the first subsample. Let me explain why.
It turns out all h264 annex b parser cares about is start codes for NALUs. And
it's careful enough to avoid detecting start codes in the encrypted data. So in
theory it might be possible to insert SPS/PPS into any subsample, not just the
very first one. In practice it would probably never happen, since we insert the
subsample either at the beginning of the buffer or after the AUD NALU. But in
the unit tests it's happening in a few cases, because of the way we generate
Annex B buffers. Our StringToAnnexB function used in unit tests generates AUD
NALUs that look like they the contain some encrypted payload (since the test
code always inserts an encrypted subsample after each clear one). But in
practice I think AUD NALUs will always be fully in clear text.
In any case, this new version of the code should handle all possible insertion
locations correctly, so please take another look.

[servolk, 2014-10-09 17:35:26]

On 2014/10/08 01:37:25, servolk wrote:
> On 2014/10/08 00:58:16, wolenetz wrote:
> > On 2014/10/04 00:17:19, servolk wrote:
> > > On 2014/09/30 22:18:18, wolenetz wrote:
> > > > On 2014/09/30 21:44:00, servolk wrote:
> > > > > On 2014/09/25 00:41:53, wolenetz wrote:
> > > > > > I would also like to see some test coverage around this, too. Would
> that
> > > be
> > > > > > possible?
> > > > > Well, sure, but how should I add a test for this? So far we have only
> seen
> > > > this
> > > > > issue with one video stream on Chromecast. But that video stream is
> using
> > > > > PlayReady DRM, so we won't be able to play it even after this is
fixed.
> > And
> > > > more
> > > > > generally - when adding a unit test for a case like this, am I
expected
> to
> > > use
> > > > a
> > > > > short piece of actual content, e.g. some .mp4 file. Or should I just
> embed
> > > the
> > > > > problematic input sample (which in this case is can be made fairly
> short,
> > a
> > > > few
> > > > > hundred bytes I think)?
> > > > 
> > > > Please pardon if I'm misunderstanding something in your question.
> > > > 
> > > > In preference order:
> > > > 1. If you can isolate the repro data to a publicly releasable mp4 file,
> > great!
> > > > (This is pretty much what the existing mp4_stream_parser_unittests do).
> > > > 2. A hybrid case where an mp4 preliminary is appended followed by
explicit
> > > short
> > > > segment of info to trigger AUD+SEI code path.
> > > > 3. A fully explicit DecoderBuffer with content created from byte array
> > inside
> > > > mp4_stream_parser_unittest.cc.
> > > 
> > > Ok, I couldn't find an easy way to generate a test file, and the repro
that
> we
> > > have uses PlayReady-protected content and I'm not clear on it's licensing
> > > status.
> > > But I looked at the existing AVC/h264 tests, and luckily we have some
useful
> > > helpers in there that allow generating arbitrary valid-looking h264
> bitstream.
> > I
> > > think we can extend the supported syntax in there a little bit to allow
> > placing
> > > multiple NALUs in the same subsample:
> > > https://codereview.chromium.org/626193003/
> > 
> > sgtm. Thanks for looking into adding tests.
> 
> Ok, now that the extended syntax for unit tests has landed, I've updated this
CL
> and added a unit test covering the case that we are handling here.
> A few more comments:
> 1. I've moved the FindSubsampleIndex from test code to avc.cc, it's useful
there
> as well.
> 2. When I was doing some testing around this CL I was getting a bit unexpected
> results from my earlier unit test attempts. But after debugging h264 bitstream
> parser I've updated the unit test and made slight modifications in the param
> insertion logic instead of that earlier DCHECK that ensured we always insert
> PPS/SPS into the first subsample. Let me explain why.
> It turns out all h264 annex b parser cares about is start codes for NALUs. And
> it's careful enough to avoid detecting start codes in the encrypted data. So
in
> theory it might be possible to insert SPS/PPS into any subsample, not just the
> very first one. In practice it would probably never happen, since we insert
the
> subsample either at the beginning of the buffer or after the AUD NALU. But in
> the unit tests it's happening in a few cases, because of the way we generate
> Annex B buffers. Our StringToAnnexB function used in unit tests generates AUD
> NALUs that look like they the contain some encrypted payload (since the test
> code always inserts an encrypted subsample after each clear one). But in
> practice I think AUD NALUs will always be fully in clear text.
> In any case, this new version of the code should handle all possible insertion
> locations correctly, so please take another look.

Ping, PTAL

[wolenetz, 2014-10-09 18:50:09]

lgtm % nits. Thank you!

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

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:55: if (p > ptr) {
nit: {} unnecessary here.

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:122: RCHECK(AVC::ConvertConfigToAnnexB(avc_config,
nit: this should all fit on one line now.

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
File media/formats/mp4/avc_unittest.cc (right):

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
media/formats/mp4/avc_unittest.cc:380: { "AUD,SPS,PPS,I",
"AUD,SPS,SPS,PPS,SPS,PPS,I" },  // Note: params placed
nit: this is similar to the AUD,SEI case that is listed next. Does the AUD,SEI
comment also apply to this test case?

[servolk, 2014-10-09 19:04:37]

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

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:55: if (p > ptr) {
On 2014/10/09 18:50:09, wolenetz wrote:
> nit: {} unnecessary here.

Done.

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc.cc...
media/formats/mp4/avc.cc:122: RCHECK(AVC::ConvertConfigToAnnexB(avc_config,
On 2014/10/09 18:50:09, wolenetz wrote:
> nit: this should all fit on one line now.

Done.

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
File media/formats/mp4/avc_unittest.cc (right):

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
media/formats/mp4/avc_unittest.cc:380: { "AUD,SPS,PPS,I",
"AUD,SPS,SPS,PPS,SPS,PPS,I" },  // Note: params placed
On 2014/10/09 18:50:09, wolenetz wrote:
> nit: this is similar to the AUD,SEI case that is listed next. Does the AUD,SEI
> comment also apply to this test case?

Well, no, it might look a bit similar, but it's not exactly the same. The key
difference is that in this case all the NALUs are in the same subsample. Whereas
in the case below AUD and SEI are in the first subsamples, I-slice is in the
second, and the SPS/PPS are still inserted in the the first subsample, right
after AUD NALU.

[wolenetz, 2014-10-09 19:15:38]

still lgtm. thanks

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
File media/formats/mp4/avc_unittest.cc (right):

https://codereview.chromium.org/591713002/diff/40001/media/formats/mp4/avc_un...
media/formats/mp4/avc_unittest.cc:380: { "AUD,SPS,PPS,I",
"AUD,SPS,SPS,PPS,SPS,PPS,I" },  // Note: params placed
On 2014/10/09 19:04:37, servolk wrote:
> On 2014/10/09 18:50:09, wolenetz wrote:
> > nit: this is similar to the AUD,SEI case that is listed next. Does the
AUD,SEI
> > comment also apply to this test case?
> 
> Well, no, it might look a bit similar, but it's not exactly the same. The key
> difference is that in this case all the NALUs are in the same subsample.
Whereas
> in the case below AUD and SEI are in the first subsamples, I-slice is in the
> second, and the SPS/PPS are still inserted in the the first subsample, right
> after AUD NALU.

Acknowledged.

[servolk, 2014-10-09 19:17:00]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2014-10-09 19:18:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/591713002/110001

[commit-bot: I haz the power, 2014-10-09 20:31:38]

Committed patchset #4 (id:110001)

[commit-bot: I haz the power, 2014-10-09 20:32:26]

Patchset 4 (id:??) landed as
https://crrev.com/88ea6544feb5796ba44cfde95a9a170472504cf1
Cr-Commit-Position: refs/heads/master@{#298967}