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

Unified Diff: media/formats/mp4/track_run_iterator_unittest.cc

Issue 1319813002: Fix mp4 keyframe parsing, removing unused stss parsing. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Fix sizeof nit Created 5 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « media/formats/mp4/track_run_iterator.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: media/formats/mp4/track_run_iterator_unittest.cc
diff --git a/media/formats/mp4/track_run_iterator_unittest.cc b/media/formats/mp4/track_run_iterator_unittest.cc
index f9dde7204f1c169dd313739820a0743b906c2283..39122a56ed9590e35bb88fd29941b4cc56c3e14d 100644
--- a/media/formats/mp4/track_run_iterator_unittest.cc
+++ b/media/formats/mp4/track_run_iterator_unittest.cc
@@ -73,8 +73,6 @@ class TrackRunIteratorTest : public testing::Test {
desc1.audio_entries.push_back(aud_desc);
moov_.extends.tracks[0].track_id = 1;
moov_.extends.tracks[0].default_sample_description_index = 1;
- moov_.tracks[0].media.information.sample_table.sync_sample.is_present =
- false;
moov_.tracks[1].header.track_id = 2;
moov_.tracks[1].media.header.timescale = kVideoScale;
SampleDescription& desc2 =
@@ -86,11 +84,6 @@ class TrackRunIteratorTest : public testing::Test {
desc2.video_entries.push_back(vid_desc);
moov_.extends.tracks[1].track_id = 2;
moov_.extends.tracks[1].default_sample_description_index = 1;
- SyncSample& video_sync_sample =
- moov_.tracks[1].media.information.sample_table.sync_sample;
- video_sync_sample.is_present = true;
- video_sync_sample.entries.resize(1);
- video_sync_sample.entries[0] = 0;
moov_.tracks[2].header.track_id = 3;
moov_.tracks[2].media.information.sample_table.description.type = kHint;
@@ -170,8 +163,6 @@ class TrackRunIteratorTest : public testing::Test {
while (iter->IsSampleValid()) {
ss << " " << (iter->is_keyframe() ? "K" : "P");
- if (iter->is_random_access_point())
- ss << "R";
iter->AdvanceSample();
}
@@ -381,10 +372,10 @@ TEST_F(TrackRunIteratorTest, FirstSampleFlagTest) {
SetFlagsOnSamples("US", &moof.tracks[1].runs[0]);
ASSERT_TRUE(iter_->Init(moof));
- EXPECT_EQ("1 KR KR KR KR KR KR KR KR KR KR", KeyframeAndRAPInfo(iter_.get()));
+ EXPECT_EQ("1 K K K K K K K K K K", KeyframeAndRAPInfo(iter_.get()));
iter_->AdvanceRun();
- EXPECT_EQ("2 KR P P P P P P P P P", KeyframeAndRAPInfo(iter_.get()));
+ EXPECT_EQ("2 K P P P P P P P P P", KeyframeAndRAPInfo(iter_.get()));
}
// Verify that parsing fails if a reserved value is in the sample flags.
@@ -672,45 +663,43 @@ TEST_F(TrackRunIteratorTest, UnexpectedOrderingTest) {
EXPECT_EQ(iter_->GetMaxClearOffset(), 10000);
}
-TEST_F(TrackRunIteratorTest, MissingAndEmptyStss) {
+TEST_F(TrackRunIteratorTest, KeyFrameFlagCombinations) {
+ // Setup both audio and video tracks to each have 6 samples covering all the
+ // combinations of mp4 "sync sample" and "depends on" relationships.
MovieFragment moof = CreateFragment();
-
- // Setup track 0 to not have an stss box, which means that all samples should
- // be marked as random access points unless the kSampleIsNonSyncSample flag is
- // set in the sample flags.
- moov_.tracks[0].media.information.sample_table.sync_sample.is_present = false;
- moov_.tracks[0].media.information.sample_table.sync_sample.entries.resize(0);
moof.tracks[0].runs.resize(1);
- moof.tracks[0].runs[0].sample_count = 6;
- moof.tracks[0].runs[0].data_offset = 100;
- SetFlagsOnSamples("US UN OS ON NS NN", &moof.tracks[0].runs[0]);
-
- // Setup track 1 to have an stss box with no entries, which normally means
- // that none of the samples should be random access points. If the
- // kSampleIsNonSyncSample flag is NOT set though, the sample should be
- // considered a random access point.
- moov_.tracks[1].media.information.sample_table.sync_sample.is_present = true;
- moov_.tracks[1].media.information.sample_table.sync_sample.entries.resize(0);
moof.tracks[1].runs.resize(1);
+ moof.tracks[0].runs[0].sample_count = 6;
moof.tracks[1].runs[0].sample_count = 6;
- moof.tracks[1].runs[0].data_offset = 200;
+ SetFlagsOnSamples("US UN OS ON NS NN", &moof.tracks[0].runs[0]);
SetFlagsOnSamples("US UN OS ON NS NN", &moof.tracks[1].runs[0]);
-
iter_.reset(new TrackRunIterator(&moov_, media_log_));
ASSERT_TRUE(iter_->Init(moof));
EXPECT_TRUE(iter_->IsRunValid());
- // Verify that all samples except for the ones that have the
- // kSampleIsNonSyncSample flag set are marked as random access points.
- EXPECT_EQ("1 KR P PR P KR K", KeyframeAndRAPInfo(iter_.get()));
+ // Keyframes should be marked according to downstream's expectations that
+ // keyframes serve as points of random access for seeking.
+
+ // For audio, any sync sample should be marked as a key frame. Whether a
+ // sample "depends on" other samples is not considered. Unlike video samples,
+ // audio samples are often marked as depending on other samples but are still
+ // workable for random access. While we allow for parsing of audio samples
+ // that are non-sync samples, we generally expect all audio samples to be sync
+ // samples and downstream will log and discard any non-sync audio samples.
+ EXPECT_EQ("1 K P K P K P", KeyframeAndRAPInfo(iter_.get()));
iter_->AdvanceRun();
- // Verify that nothing is marked as a random access point.
- EXPECT_EQ("2 KR P PR P KR K", KeyframeAndRAPInfo(iter_.get()));
+ // For video, any key frame should be both a sync sample and have no known
+ // dependents. Ideally, a video sync sample should always be marked as having
+ // no dependents, but we occasionally encounter media where all samples are
+ // marked "sync" and we must rely on combining the two flags to pick out the
+ // true key frames. See http://crbug.com/310712 and http://crbug.com/507916.
+ // Realiably knowing the keyframes for video is also critical to SPS PPS
+ // insertion.
+ EXPECT_EQ("2 K P P P K P", KeyframeAndRAPInfo(iter_.get()));
}
-
} // namespace mp4
} // namespace media
« no previous file with comments | « media/formats/mp4/track_run_iterator.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698