Fix mp4 keyframe parsing, removing unused stss parsing.

Fix mp4 keyframe parsing, removing unused stss parsing.

Lots of history below, TLDR:
Keyframes for video will now be those marked as sync samples AND having no known sample dependency. Previously all sync samples were marked as key to downstream but this caused everything to be marked key for certain media (see bugs).

History:
0. Originally, we just trusted the is_non_sync_sample flag (once called is_difference_sample)

1. http://crrev.com/37603002 - adds parsing of sdtp ("depends on") info
 -- http://crbug.com/310712 - working around bad content
 -- no tests
 -- still here today

2. http://crrev.com/134873002 - adds parsing of stss box
 -- no stss box -> all frames are key
 -- seems right given this exceprt from spec: "If the sync sample box is not present, every sample is a sync sample."
 -- internal bug that motivated http://b/12218009

3. http://crrev.com/282513004 - separates "random_access_point" from "key_frame"
 -- in language of this change: RAP=sync sampe, and KF=no known dependencies
 -- idea: "Keyframe and random access point info are now tracked separately so things like SPS/PPS insertion will only occur on actual keyframes instead of anything marked as a random access point."

#3 fixes the issues for sps/pps insertion, but since anything marked as random access point is still bubbled up as a keyframe for seeking/buffered-range-management, these things still break whenever media marks it wrong (I suspected this regressed the issue fixed in #1). To fix these lingering issues, this CL combines the conditions for random_access_point and key_frame into one.

Additionally, #3 added new stss box parsing logic but didn't use it. For fragmented mp4, I don't think stss box can actually be used. The keyframe information comes from various flags which fallback to default values when not set. The final authority on defaults is the trex box, which is mandatory for fragmented mp4, so if nothing else, we will take the flags from the trex and have no chance to include stss. Stss box parsing is now removed.

FYI: the spec is available here:
http://standards.iso.org/ittf/PubliclyAvailableStandards/c061988_ISO_IEC_14496-12_2012.zip

BUG=507916, 310712, 367786
Committed: https://crrev.com/b175d2a7d45930d742b1544f3a0bb9dea9fa4fba
Cr-Commit-Position: refs/heads/master@{#346238}

[chcunningham, 2015-08-26 22:27:47]

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

[wolenetz, 2015-08-27 02:40:36]

I've just parsed the CL description and looked up a little history - I'll do
deeper review tomorrow.

Regarding STSS, see also
https://code.google.com/p/chromium/issues/detail?id=423858#c30

If STSS is in the bytestream, and is well-formed, should we consume it but
ignore it otherwise? (I think so. I haven't looked over your patch to see if it
rejects streams with STSS though. WDYT?)

Thank you for doing the necessary spec and code spelunking :)

[wolenetz, 2015-08-27 02:42:41]

On 2015/08/27 02:40:36, wolenetz wrote:
> I've just parsed the CL description and looked up a little history - I'll do
> deeper review tomorrow.
> 
> Regarding STSS, see also
> https://code.google.com/p/chromium/issues/detail?id=423858#c30
> 
> If STSS is in the bytestream, and is well-formed, should we consume it but
> ignore it otherwise? (I think so. I haven't looked over your patch to see if
it
> rejects streams with STSS though. WDYT?)
> 
> Thank you for doing the necessary spec and code spelunking :)

acolwell@: with respect to my STSS question, the MSE ISO BMFF bytestream says to
emit decode error if "External data references are being used." Does presence of
STSS qualify as such an external data reference, or is something else meant by
that clause in the bytestream spec?

[wolenetz, 2015-08-27 03:01:30]

code looks pretty good. first round of code questions/comments beyond my earlier
CL description questions:

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:159: // the sample does not depend on
others (FFmpeg does to, see mov_read_trun).
nit: s/to/too/

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:166: bool sample_depends_on_others =
sample_depends_on == kSampleDependsOnOthers;
Hmm. This patch drops the automatic compiler check that we're doing something
for each possible SampleDependsOn enumerated case (see the removed switch
statement). Does this patch make the code more fragile?

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:173: << " audio:" << is_audio;
Do we catch non-keyframe audio buffers in parser, coded frame processor or
elsewhere and LIMITED_MEDIA_LOG them at least (if not MEDIA_LOG( ERROR ) + parse
err)?

[chcunningham, 2015-08-27 17:42:24]

On 2015/08/27 02:40:36, wolenetz wrote:
> I've just parsed the CL description and looked up a little history - I'll do
> deeper review tomorrow.
> 
> Regarding STSS, see also
> https://code.google.com/p/chromium/issues/detail?id=423858#c30
> 
> If STSS is in the bytestream, and is well-formed, should we consume it but
> ignore it otherwise? (I think so. I haven't looked over your patch to see if
it
> rejects streams with STSS though. WDYT?)

I might be confused by your question. By consume, do you mean parse? I searched
for a way to use the stss (some sort of fallback for key frame signalling), but
I could not find a way to incorporate it given that trex will always be there
and will always have sample flag defaults. If we don't have any use for stss,
I'd prefer to rip out code that parses it. Its still fine for content to include
it, we will just skip over it. 

>
> Thank you for doing the necessary spec and code spelunking :)

You bet :)

[chcunningham, 2015-08-27 18:59:39]

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:159: // the sample does not depend on
others (FFmpeg does to, see mov_read_trun).
On 2015/08/27 03:01:30, wolenetz wrote:
> nit: s/to/too/

Done.

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:166: bool sample_depends_on_others =
sample_depends_on == kSampleDependsOnOthers;
On 2015/08/27 03:01:30, wolenetz wrote:
> Hmm. This patch drops the automatic compiler check that we're doing something
> for each possible SampleDependsOn enumerated case (see the removed switch
> statement). Does this patch make the code more fragile?

I don't think this is more fragile. There are just 4 possible values (just 2
bits). 

1. reserved - logs an error and bails
2. depends on others -> !keyframe
3. depends on unknown -> keyframe if is_sync
4. depends on no others -> keyframe if is sync

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:173: << " audio:" << is_audio;
On 2015/08/27 03:01:30, wolenetz wrote:
> Do we catch non-keyframe audio buffers in parser, coded frame processor or
> elsewhere and LIMITED_MEDIA_LOG them at least (if not MEDIA_LOG( ERROR ) +
parse
> err)?

We used to (https://b.corp.google.com/issues/12218009#comment6), but thats not
the case today. I can add a warning log somewhere... does the all-audio-is-key
assumption hold for all mse codecs? if so, I lean toward putting it downstream
of mp4 parsing to catch other codecs as well.

[acolwell GONE FROM CHROMIUM, 2015-08-28 15:38:50]

On 2015/08/27 02:42:41, wolenetz wrote:
> On 2015/08/27 02:40:36, wolenetz wrote:
> > I've just parsed the CL description and looked up a little history - I'll do
> > deeper review tomorrow.
> > 
> > Regarding STSS, see also
> > https://code.google.com/p/chromium/issues/detail?id=423858#c30
> > 
> > If STSS is in the bytestream, and is well-formed, should we consume it but
> > ignore it otherwise? (I think so. I haven't looked over your patch to see if
> it
> > rejects streams with STSS though. WDYT?)
> > 
> > Thank you for doing the necessary spec and code spelunking :)
> 
> acolwell@: with respect to my STSS question, the MSE ISO BMFF bytestream says
to
> emit decode error if "External data references are being used." Does presence
of
> STSS qualify as such an external data reference, or is something else meant by
> that clause in the bytestream spec?

No. "External data references" is intended to refer to dref boxes that contain
URLs to resources outside of the ISOBMFF file. MSE requires all the sample data
to be contained with the same bytestream so these sorts of external references
to other resources must generate an error.

[acolwell GONE FROM CHROMIUM, 2015-08-28 15:39:01]

lgtm

https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:94: stream << std::setfill('0') <<
std::setw(sizeof(uint32)*2) << std::hex
nit:sizeof(flags) just in case its size changes at some point?

[wolenetz, 2015-08-28 18:08:19]

The link to spec in CL description is not a link to a spec, it's a link to
sample files apparently. Please update (or remove...)

[chcunningham, 2015-08-28 18:11:01]

On 2015/08/28 18:08:19, wolenetz wrote:
> The link to spec in CL description is not a link to a spec, it's a link to
> sample files apparently. Please update (or remove...)

fixed.

[wolenetz, 2015-08-28 18:23:24]

On 2015/08/28 15:38:50, acolwell GONE FROM CHROMIUM wrote:
> On 2015/08/27 02:42:41, wolenetz wrote:
> > On 2015/08/27 02:40:36, wolenetz wrote:
> > > I've just parsed the CL description and looked up a little history - I'll
do
> > > deeper review tomorrow.
> > > 
> > > Regarding STSS, see also
> > > https://code.google.com/p/chromium/issues/detail?id=423858#c30
> > > 
> > > If STSS is in the bytestream, and is well-formed, should we consume it but
> > > ignore it otherwise? (I think so. I haven't looked over your patch to see
if
> > it
> > > rejects streams with STSS though. WDYT?)
> > > 
> > > Thank you for doing the necessary spec and code spelunking :)
> > 
> > acolwell@: with respect to my STSS question, the MSE ISO BMFF bytestream
says
> to
> > emit decode error if "External data references are being used." Does
presence
> of
> > STSS qualify as such an external data reference, or is something else meant
by
> > that clause in the bytestream spec?
> 
> No. "External data references" is intended to refer to dref boxes that contain
> URLs to resources outside of the ISOBMFF file. MSE requires all the sample
data
> to be contained with the same bytestream so these sorts of external references
> to other resources must generate an error.

Thanks Aaron for clarifying external data references doesn't mean something like
"boxes that contain absolute frame references to frames that may (or may not)
exist within the bytestream", rather a dref with external URL reference.

[wolenetz, 2015-08-28 18:23:37]

lgtm % acolwell@'s nit

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:166: bool sample_depends_on_others =
sample_depends_on == kSampleDependsOnOthers;
On 2015/08/27 18:59:39, chcunningham wrote:
> On 2015/08/27 03:01:30, wolenetz wrote:
> > Hmm. This patch drops the automatic compiler check that we're doing
something
> > for each possible SampleDependsOn enumerated case (see the removed switch
> > statement). Does this patch make the code more fragile?
> 
> I don't think this is more fragile. There are just 4 possible values (just 2
> bits). 
> 
> 1. reserved - logs an error and bails
> 2. depends on others -> !keyframe
> 3. depends on unknown -> keyframe if is_sync
> 4. depends on no others -> keyframe if is sync

Acknowledged.

https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:173: << " audio:" << is_audio;
On 2015/08/27 18:59:39, chcunningham wrote:
> On 2015/08/27 03:01:30, wolenetz wrote:
> > Do we catch non-keyframe audio buffers in parser, coded frame processor or
> > elsewhere and LIMITED_MEDIA_LOG them at least (if not MEDIA_LOG( ERROR ) +
> parse
> > err)?
> 
> We used to (https://b.corp.google.com/issues/12218009#comment6), but thats not
> the case today. I can add a warning log somewhere... does the all-audio-is-key
> assumption hold for all mse codecs? if so, I lean toward putting it downstream
> of mp4 parsing to catch other codecs as well.

I'll take a look at this (audio frames potentially not marked as keyframes by
Chrome MSE parsers) separately. It could be causing issues that I'm already
looking into since this CR started...

[chcunningham, 2015-08-28 20:34:47]

Thanks guys.

https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track...
media/formats/mp4/track_run_iterator.cc:94: stream << std::setfill('0') <<
std::setw(sizeof(uint32)*2) << std::hex
On 2015/08/28 15:39:01, acolwell GONE FROM CHROMIUM wrote:
> nit:sizeof(flags) just in case its size changes at some point?

Done.

[chcunningham, 2015-08-28 20:35:39]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2015-08-28 20:35:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
acolwell@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1319813002/#ps80001
(title: "Fix sizeof nit")

[commit-bot: I haz the power, 2015-08-28 20:36:06]

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

[commit-bot: I haz the power, 2015-08-28 21:38:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-28 21:39:21]

Patchset 5 (id:??) landed as
https://crrev.com/b175d2a7d45930d742b1544f3a0bb9dea9fa4fba
Cr-Commit-Position: refs/heads/master@{#346238}