Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(139)

Issue 1319813002: Fix mp4 keyframe parsing, removing unused stss parsing. (Closed)

Created:
5 years, 3 months ago by chcunningham
Modified:
5 years, 3 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Removing silly stss test. #

Patch Set 3 : Minor typo/fixes. #

Total comments: 8

Patch Set 4 : Typo #

Total comments: 2

Patch Set 5 : Fix sizeof nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -130 lines) Patch
M media/formats/mp4/box_definitions.h View 2 chunks +0 lines, -12 lines 0 comments Download
M media/formats/mp4/box_definitions.cc View 1 chunk +1 line, -43 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.cc View 1 2 2 chunks +1 line, -5 lines 0 comments Download
M media/formats/mp4/track_run_iterator.h View 1 chunk +0 lines, -1 line 0 comments Download
M media/formats/mp4/track_run_iterator.cc View 1 2 3 4 7 chunks +38 lines, -33 lines 0 comments Download
M media/formats/mp4/track_run_iterator_unittest.cc View 1 2 5 chunks +25 lines, -36 lines 0 comments Download

Messages

Total messages: 18 (3 generated)
wolenetz
I've just parsed the CL description and looked up a little history - I'll do ...
5 years, 3 months ago (2015-08-27 02:40:36 UTC) #2
wolenetz
On 2015/08/27 02:40:36, wolenetz wrote: > I've just parsed the CL description and looked up ...
5 years, 3 months ago (2015-08-27 02:42:41 UTC) #3
wolenetz
code looks pretty good. first round of code questions/comments beyond my earlier CL description questions: ...
5 years, 3 months ago (2015-08-27 03:01:30 UTC) #4
chcunningham
On 2015/08/27 02:40:36, wolenetz wrote: > I've just parsed the CL description and looked up ...
5 years, 3 months ago (2015-08-27 17:42:24 UTC) #5
chcunningham
https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track_run_iterator.cc File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track_run_iterator.cc#newcode159 media/formats/mp4/track_run_iterator.cc:159: // the sample does not depend on others (FFmpeg ...
5 years, 3 months ago (2015-08-27 18:59:39 UTC) #6
acolwell GONE FROM CHROMIUM
On 2015/08/27 02:42:41, wolenetz wrote: > On 2015/08/27 02:40:36, wolenetz wrote: > > I've just ...
5 years, 3 months ago (2015-08-28 15:38:50 UTC) #7
acolwell GONE FROM CHROMIUM
lgtm https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track_run_iterator.cc File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track_run_iterator.cc#newcode94 media/formats/mp4/track_run_iterator.cc:94: stream << std::setfill('0') << std::setw(sizeof(uint32)*2) << std::hex nit:sizeof(flags) ...
5 years, 3 months ago (2015-08-28 15:39:01 UTC) #8
wolenetz
The link to spec in CL description is not a link to a spec, it's ...
5 years, 3 months ago (2015-08-28 18:08:19 UTC) #9
chcunningham
On 2015/08/28 18:08:19, wolenetz wrote: > The link to spec in CL description is not ...
5 years, 3 months ago (2015-08-28 18:11:01 UTC) #10
wolenetz
On 2015/08/28 15:38:50, acolwell GONE FROM CHROMIUM wrote: > On 2015/08/27 02:42:41, wolenetz wrote: > ...
5 years, 3 months ago (2015-08-28 18:23:24 UTC) #11
wolenetz
lgtm % acolwell@'s nit https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track_run_iterator.cc File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1319813002/diff/40001/media/formats/mp4/track_run_iterator.cc#newcode166 media/formats/mp4/track_run_iterator.cc:166: bool sample_depends_on_others = sample_depends_on == ...
5 years, 3 months ago (2015-08-28 18:23:37 UTC) #12
chcunningham
Thanks guys. https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track_run_iterator.cc File media/formats/mp4/track_run_iterator.cc (right): https://codereview.chromium.org/1319813002/diff/60001/media/formats/mp4/track_run_iterator.cc#newcode94 media/formats/mp4/track_run_iterator.cc:94: stream << std::setfill('0') << std::setw(sizeof(uint32)*2) << std::hex ...
5 years, 3 months ago (2015-08-28 20:34:47 UTC) #13
commit-bot: I haz the power
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
5 years, 3 months ago (2015-08-28 20:36:06 UTC) #16
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 3 months ago (2015-08-28 21:38:47 UTC) #17
commit-bot: I haz the power
5 years, 3 months ago (2015-08-28 21:39:21 UTC) #18
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/b175d2a7d45930d742b1544f3a0bb9dea9fa4fba
Cr-Commit-Position: refs/heads/master@{#346238}

Powered by Google App Engine
This is Rietveld 408576698