MSE: Relax the 'media segment must begin with keyframe' requirement

media/filters/frame_processor_unittest.cc

Index: media/filters/frame_processor_unittest.cc
diff --git a/media/filters/frame_processor_unittest.cc b/media/filters/frame_processor_unittest.cc
index b7fc48d3240089fbc73a299199c86244ad245b2d..3b8d1d0669e313133c9d4fdb30fa7711ac7f36d7 100644
--- a/media/filters/frame_processor_unittest.cc
+++ b/media/filters/frame_processor_unittest.cc
@@ -67,7 +67,6 @@ class FrameProcessorTest : public testing::TestWithParam<bool> {
                 base::Unretained(&callbacks_)),
             new MediaLog())),
         append_window_end_(kInfiniteDuration()),
-        new_media_segment_(false),
         audio_id_(FrameProcessor::kAudioTrackId),
         video_id_(FrameProcessor::kVideoTrackId),
         frame_duration_(base::TimeDelta::FromMilliseconds(10)) {}
@@ -157,9 +156,8 @@ class FrameProcessorTest : public testing::TestWithParam<bool> {
     ASSERT_TRUE(frame_processor_->ProcessFrames(
         StringToBufferQueue(audio_timestamps, audio_id_, DemuxerStream::AUDIO),
         StringToBufferQueue(video_timestamps, video_id_, DemuxerStream::VIDEO),
-        empty_text_buffers_,
-        append_window_start_, append_window_end_,
-        &new_media_segment_, &timestamp_offset_));
+        empty_text_buffers_, append_window_start_, append_window_end_,
+        &timestamp_offset_));
   }
 
   void CheckExpectedRangesByTimestamp(ChunkDemuxerStream* stream,
@@ -246,13 +244,16 @@ class FrameProcessorTest : public testing::TestWithParam<bool> {
     CheckReadStalls(stream);
   }
 
+  bool in_coded_frame_group() {

[chcunningham, 2016/01/22 01:26:37]

Edit:

I that the test I described (and many more) live in chunk demuxer unit tests...
I'm fine with that. 

I still think these tests would be better off using the mocked stream and
checking for the notify call instead of checking this private flag though. 

[chcunningham, 2016/01/22 01:26:38]

I feel like this is testing implementation details rather than the public
interface. Can you instead replace this with checks that the ChunkDemuxerStream
(possibly mocked) receives the notification of a new coded frame group at the
points when it should and not when it shouldn't?

Unless I'm missing it, I think a test thats definitely worth adding would be:

1. Append media segment that doesn't start with a keyframe and has none 
2. verify no notification to ChunkDemuxerStream
3. Append media segment that still doesn't start with KF, but has one in middle
4. verify notification to ChunkDemuxerStrea, but careful to verify the timestamp
notifies matches that of the KF in middle of segment

[wolenetz, 2016/01/22 01:50:03]

SGTM. Good idea (around mocked stream; checking for notify call/lack thereof).
I'll see if such a mock implementation is reasonably workable for inclusion in
this CL (IOW, not too complex for a potential merge-to-m49.) If complex, would
you be ok with a TODO and crbug to follow-up with the recommended rework? (I
think it'll fit within this CL, just getting an idea to how strong you're
feeling about this is.)

[wolenetz, 2016/01/22 01:50:03]

Acknowledged (your edit).

[wolenetz, 2016/01/22 18:59:31]

From my investigation and offline chat w/chcunningham@, it's not a simple mock
method addition since FrameProcessorTests rely on ChunkDemuxerStream to do all
the things like buffering and reading/stalling reads, etc. Rather, probably one
of at least two more complex alternatives would be needed:
1) Determine a (or create a new) subset of FrameProcessorTests that verify
expected new-coded-frame-group calls on a mocked ChunkDemuxerStream, with
limited scope on the mock otherwise (no buffered ranges, reading back of
buffers, etc).
2) Make the ChunkDemuxerStream new-coded-frame-group callback method virtual and
subclass it specifically for these tests.
3) Find some other way to mock/verify the signal.
Note that at least the "media-segmented-ness" is no longer the responsibility at
all of FrameProcessor (solely owned now by StreamParsers and their interaction
with MediaSourceState callbacks, and verified by ChunkDemuxerTests). Only the
"new coded frame group signalling" verification needs some cleanup.

IIUC, chcunningham@ was ok with such verification cleanup in a follow-up CL to
ease the merging of this CL (and since we have mostly equivalent coverage
currently, but on the internal state of FPTest's coded-frame-group flag but not
the signalling thereof.)

I've added a TODO and crbug to clean this up.

[chcunningham, 2016/01/22 19:26:02]

Acknowledged.

+    return frame_processor_->in_coded_frame_group_;
+  }
+
   base::MessageLoop message_loop_;
   StrictMock<FrameProcessorTestCallbackHelper> callbacks_;
 
   scoped_ptr<FrameProcessor> frame_processor_;
   base::TimeDelta append_window_start_;
   base::TimeDelta append_window_end_;
-  bool new_media_segment_;
   base::TimeDelta timestamp_offset_;
   scoped_ptr<ChunkDemuxerStream> audio_;
   scoped_ptr<ChunkDemuxerStream> video_;
@@ -316,15 +317,13 @@ class FrameProcessorTest : public testing::TestWithParam<bool> {
 
 TEST_F(FrameProcessorTest, WrongTypeInAppendedBuffer) {
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
+  EXPECT_FALSE(in_coded_frame_group());
 
   ASSERT_FALSE(frame_processor_->ProcessFrames(
-      StringToBufferQueue("0K", audio_id_, DemuxerStream::VIDEO),
-      empty_queue_,
-      empty_text_buffers_,
-      append_window_start_, append_window_end_,
-      &new_media_segment_, &timestamp_offset_));
-  EXPECT_TRUE(new_media_segment_);
+      StringToBufferQueue("0K", audio_id_, DemuxerStream::VIDEO), empty_queue_,
+      empty_text_buffers_, append_window_start_, append_window_end_,
+      &timestamp_offset_));
+  EXPECT_FALSE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ }");
   CheckReadStalls(audio_.get());
@@ -332,15 +331,12 @@ TEST_F(FrameProcessorTest, WrongTypeInAppendedBuffer) {
 
 TEST_F(FrameProcessorTest, NonMonotonicallyIncreasingTimestampInOneCall) {
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
 
   ASSERT_FALSE(frame_processor_->ProcessFrames(
       StringToBufferQueue("10K 0K", audio_id_, DemuxerStream::AUDIO),
-      empty_queue_,
-      empty_text_buffers_,
-      append_window_start_, append_window_end_,
-      &new_media_segment_, &timestamp_offset_));
-  EXPECT_TRUE(new_media_segment_);
+      empty_queue_, empty_text_buffers_, append_window_start_,
+      append_window_end_, &timestamp_offset_));
+  EXPECT_FALSE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ }");
   CheckReadStalls(audio_.get());
@@ -350,13 +346,12 @@ TEST_P(FrameProcessorTest, AudioOnly_SingleFrame) {
   // Tests A: P(A) -> (a)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_));
   ProcessFrames("0K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,10) }");
   CheckReadsThenReadStalls(audio_.get(), "0");
@@ -366,13 +361,12 @@ TEST_P(FrameProcessorTest, VideoOnly_SingleFrame) {
   // Tests V: P(V) -> (v)
   InSequence s;
   AddTestTracks(HAS_VIDEO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_));
   ProcessFrames("", "0K");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(video_.get(), "{ [0,10) }");
   CheckReadsThenReadStalls(video_.get(), "0");
@@ -382,13 +376,12 @@ TEST_P(FrameProcessorTest, AudioOnly_TwoFrames) {
   // Tests A: P(A0, A10) -> (a0, a10)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 2));
   ProcessFrames("0K 10K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,20) }");
   CheckReadsThenReadStalls(audio_.get(), "0 10");
@@ -398,7 +391,6 @@ TEST_P(FrameProcessorTest, AudioOnly_SetOffsetThenSingleFrame) {
   // Tests A: STSO(50)+P(A0) -> TSO==50,(a0@50)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
@@ -406,7 +398,7 @@ TEST_P(FrameProcessorTest, AudioOnly_SetOffsetThenSingleFrame) {
   SetTimestampOffset(fifty_ms);
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ + fifty_ms));
   ProcessFrames("0K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(fifty_ms, timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [50,60) }");
 
@@ -421,7 +413,6 @@ TEST_P(FrameProcessorTest, AudioOnly_SetOffsetThenFrameTimestampBelowOffset) {
   //   if segments mode: TSO==50,(a20@70)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   bool using_sequence_mode = GetParam();
   if (using_sequence_mode)
     frame_processor_->SetSequenceMode(true);
@@ -439,7 +430,7 @@ TEST_P(FrameProcessorTest, AudioOnly_SetOffsetThenFrameTimestampBelowOffset) {
   }
 
   ProcessFrames("20K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
 
   // We do not stall on reading without seeking to 50ms / 70ms due to
   // SourceBufferStream::kSeekToStartFudgeRoom().
@@ -458,19 +449,18 @@ TEST_P(FrameProcessorTest, AudioOnly_SequentialProcessFrames) {
   // Tests A: P(A0,A10)+P(A20,A30) -> (a0,a10,a20,a30)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 2));
   ProcessFrames("0K 10K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,20) }");
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 4));
   ProcessFrames("20K 30K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,40) }");
 
@@ -484,7 +474,6 @@ TEST_P(FrameProcessorTest, AudioOnly_NonSequentialProcessFrames) {
   //   if segments mode: TSO==0,(a0,a10,a20,a30)
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   bool using_sequence_mode = GetParam();
   if (using_sequence_mode) {
     frame_processor_->SetSequenceMode(true);
@@ -494,7 +483,7 @@ TEST_P(FrameProcessorTest, AudioOnly_NonSequentialProcessFrames) {
   }
 
   ProcessFrames("20K 30K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
 
   if (using_sequence_mode) {
     CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,20) }");
@@ -507,7 +496,7 @@ TEST_P(FrameProcessorTest, AudioOnly_NonSequentialProcessFrames) {
   }
 
   ProcessFrames("0K 10K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
 
   if (using_sequence_mode) {
     CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,40) }");
@@ -531,20 +520,19 @@ TEST_P(FrameProcessorTest, AudioVideo_SequentialProcessFrames) {
   //   (a0,a10,a20,a30,a40);(v0,v10,v20,v30)
   InSequence s;
   AddTestTracks(HAS_AUDIO | HAS_VIDEO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 3));
   ProcessFrames("0K 10K", "0K 10 20");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,20) }");
   CheckExpectedRangesByTimestamp(video_.get(), "{ [0,30) }");
 
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_ * 5));
   ProcessFrames("20K 30K 40K", "30");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,50) }");
   CheckExpectedRangesByTimestamp(video_.get(), "{ [0,40) }");
@@ -561,7 +549,6 @@ TEST_P(FrameProcessorTest, AudioVideo_Discontinuity) {
   // MergeBufferQueues() behavior.
   InSequence s;
   AddTestTracks(HAS_AUDIO | HAS_VIDEO);
-  new_media_segment_ = true;
   bool using_sequence_mode = GetParam();
   if (using_sequence_mode) {
     frame_processor_->SetSequenceMode(true);
@@ -571,7 +558,7 @@ TEST_P(FrameProcessorTest, AudioVideo_Discontinuity) {
   }
 
   ProcessFrames("0K 10K 30K 40K 50K", "0K 10 40 50K");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
 
   if (using_sequence_mode) {
     EXPECT_EQ(frame_duration_, timestamp_offset_);
@@ -596,14 +583,13 @@ TEST_P(FrameProcessorTest,
        AppendWindowFilterOfNegativeBufferTimestampsWithPrerollDiscard) {
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
 
   SetTimestampOffset(frame_duration_ * -2);
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_));
   ProcessFrames("0K 10K 20K", "");
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   EXPECT_EQ(frame_duration_ * -2, timestamp_offset_);
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,10) }");
   CheckReadsThenReadStalls(audio_.get(), "0:10P 0:20");
@@ -612,7 +598,6 @@ TEST_P(FrameProcessorTest,
 TEST_P(FrameProcessorTest, AppendWindowFilterWithInexactPreroll) {
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
   SetTimestampOffset(-frame_duration_);
@@ -625,7 +610,6 @@ TEST_P(FrameProcessorTest, AppendWindowFilterWithInexactPreroll) {
 TEST_P(FrameProcessorTest, AppendWindowFilterWithInexactPreroll_2) {
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
   SetTimestampOffset(-frame_duration_);
@@ -638,7 +622,6 @@ TEST_P(FrameProcessorTest, AppendWindowFilterWithInexactPreroll_2) {
 TEST_P(FrameProcessorTest, AllowNegativeFramePTSAndDTSBeforeOffsetAdjustment) {
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   bool using_sequence_mode = GetParam();
   if (using_sequence_mode) {
     frame_processor_->SetSequenceMode(true);
@@ -666,7 +649,6 @@ TEST_P(FrameProcessorTest, PartialAppendWindowFilterNoDiscontinuity) {
   // trimmed frame.
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   if (GetParam())
     frame_processor_->SetSequenceMode(true);
   EXPECT_CALL(callbacks_,
@@ -686,7 +668,6 @@ TEST_P(FrameProcessorTest,
   // frame that originally had DTS > PTS.
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   bool using_sequence_mode = GetParam();
   if (using_sequence_mode) {
     frame_processor_->SetSequenceMode(true);
@@ -725,7 +706,6 @@ TEST_P(FrameProcessorTest, PartialAppendWindowFilterNoNewMediaSegment) {
   // discontinuity.
   InSequence s;
   AddTestTracks(HAS_AUDIO | HAS_VIDEO);
-  new_media_segment_ = true;
   frame_processor_->SetSequenceMode(GetParam());
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_));
   ProcessFrames("", "0K");
@@ -735,7 +715,7 @@ TEST_P(FrameProcessorTest, PartialAppendWindowFilterNoNewMediaSegment) {
   ProcessFrames("", "10");
 
   EXPECT_EQ(base::TimeDelta(), timestamp_offset_);
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,5) }");
   CheckExpectedRangesByTimestamp(video_.get(), "{ [0,20) }");
   CheckReadsThenReadStalls(audio_.get(), "0:-5");
@@ -745,7 +725,6 @@ TEST_P(FrameProcessorTest, PartialAppendWindowFilterNoNewMediaSegment) {
 TEST_F(FrameProcessorTest, AudioOnly_SequenceModeContinuityAcrossReset) {
   InSequence s;
   AddTestTracks(HAS_AUDIO);
-  new_media_segment_ = true;
   frame_processor_->SetSequenceMode(true);
   EXPECT_CALL(callbacks_, PossibleDurationIncrease(frame_duration_));
   ProcessFrames("0K", "");
@@ -754,7 +733,7 @@ TEST_F(FrameProcessorTest, AudioOnly_SequenceModeContinuityAcrossReset) {
   ProcessFrames("100K", "");
 
   EXPECT_EQ(frame_duration_ * -9, timestamp_offset_);
-  EXPECT_FALSE(new_media_segment_);
+  EXPECT_TRUE(in_coded_frame_group());
   CheckExpectedRangesByTimestamp(audio_.get(), "{ [0,20) }");
   CheckReadsThenReadStalls(audio_.get(), "0 10:100");
 }