Fix SourceBufferStream alt-ref frame handling.

Fix SourceBufferStream alt-ref frame handling.


BUG=181094
TEST=SourceBufferStreamTest.AltRefFrame_*


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187537

[acolwell GONE FROM CHROMIUM, 2013-03-11 18:35:15]

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream_unittest.cc:215: EXPECT_EQ(expected,
ss.str());
Moving the expectation check to here makes it easier to debug test failures.

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream_unittest.cc:1850: NewSegmentAppend(15, 2,
&kDataA);
This was a bug in the original test. Since this is supposed to be a start
overlap, it should be a new segment. The new alt-ref frame logic detected this
bug.

[scherkus (not reviewing), 2013-03-11 21:23:40]

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream.cc:127: bool IsEndKeyframe() const;
EndsWithKeyframe?

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream.cc:691: bool is_valid_alt_ref =
prev_timestamp == next_timestamp &&
how does this compare with the if block below that checks for invalid alt-ref
frame constructs?

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream_unittest.cc:215: EXPECT_EQ(expected,
ss.str());
On 2013/03/11 18:35:15, acolwell wrote:
> Moving the expectation check to here makes it easier to debug test failures.

These days my preference is for helper methods to return values and use
EXPECT_XX() in test bodies, defining your own EXPECT macros if things get
unwieldy (e.g., CheckExpectedBuffers("0K 30 61 120K 150") can be
EXPECT_BUFFERS("0K 30 61 120K 150")).

Why? If a test calls multiple check methods it's not immediately obvious which
one failed since they all point to the same line of code. It also gets around
the problems using ASSERT_XX() inside helper methods.

I don't expect you to change this file -- just food for thought.

[acolwell GONE FROM CHROMIUM, 2013-03-11 22:31:38]

Fixed the bug vrk@ discovered and addressed comments by scherkus@

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream.cc:127: bool IsEndKeyframe() const;
On 2013/03/11 21:23:40, scherkus wrote:
> EndsWithKeyframe?
Removed. Method no longer needed.

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream.cc:691: bool is_valid_alt_ref =
prev_timestamp == next_timestamp &&
On 2013/03/11 21:23:40, scherkus wrote:
> how does this compare with the if block below that checks for invalid alt-ref
> frame constructs?
This identifies a valid alt-ref construct that we want to allow & handle in a
special way. The checks below exclude duplicate timestamp situations that we
should never allow. There is a case though where is_valid_alt_ref is false, but
we don't want to return an error. This is when we have an overlap situation
where the last buffer appended gets overlapped by the next append. vrk found a
bug in this code so the new patch, should make this a little clearer.

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/12701008/diff/1/media/filters/source_buffer_s...
media/filters/source_buffer_stream_unittest.cc:215: EXPECT_EQ(expected,
ss.str());
On 2013/03/11 21:23:40, scherkus wrote:
> On 2013/03/11 18:35:15, acolwell wrote:
> > Moving the expectation check to here makes it easier to debug test failures.
> 
> These days my preference is for helper methods to return values and use
> EXPECT_XX() in test bodies, defining your own EXPECT macros if things get
> unwieldy (e.g., CheckExpectedBuffers("0K 30 61 120K 150") can be
> EXPECT_BUFFERS("0K 30 61 120K 150")).
> 
> Why? If a test calls multiple check methods it's not immediately obvious which
> one failed since they all point to the same line of code. It also gets around
> the problems using ASSERT_XX() inside helper methods.
> 
> I don't expect you to change this file -- just food for thought.

Thanks for the tip. I'll consider changing this in a later CL.

[scherkus (not reviewing), 2013-03-11 23:21:50]

lgtm w/ nits

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:692: // Check for invalid alt-ref frame
constructs.
want to format this to use a bullet list / indent for the two cases we're
checking for?

Check for invalid alt-ref frame constructs:
  * a keyframe followed by a non-keyframe or
  * a non-keyframe blah blah ...

also might be nice to link / describe what an alt-ref frame is -- the expected
results for AltRefFrame_Overlap_3 looks a bit funny at first glance due to the
duplicated "90" timestamps (I'm guessing one of those is considered the alt-ref
frame?)

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:705: // this is either a start overlap or
an middle overlap. Delete the buffers
s/or an middle/or a middle/

[acolwell GONE FROM CHROMIUM, 2013-03-12 03:22:49]

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:692: // Check for invalid alt-ref frame
constructs.
On 2013/03/11 23:21:50, scherkus wrote:
> want to format this to use a bullet list / indent for the two cases we're
> checking for?
> 
> Check for invalid alt-ref frame constructs:
>   * a keyframe followed by a non-keyframe or
>   * a non-keyframe blah blah ...
> 
> also might be nice to link / describe what an alt-ref frame is -- the expected
> results for AltRefFrame_Overlap_3 looks a bit funny at first glance due to the
> duplicated "90" timestamps (I'm guessing one of those is considered the
alt-ref
> frame?)

Done.

https://codereview.chromium.org/12701008/diff/7001/media/filters/source_buffe...
media/filters/source_buffer_stream.cc:705: // this is either a start overlap or
an middle overlap. Delete the buffers
On 2013/03/11 23:21:50, scherkus wrote:
> s/or an middle/or a middle/

Done.

[commit-bot: I haz the power, 2013-03-12 03:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/12701008/5002

[commit-bot: I haz the power, 2013-03-12 07:59:04]

Change committed as 187537