Make sure previous seek points in SourceBufferStream are ignored during overlaps

Make sure previous seek points in SourceBufferStream are ignored during overlaps

BUG=136463
TEST=media_unittests,seek-sourceabort-crash.html

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

[vrk (LEFT CHROMIUM), 2012-07-10 03:10:23]

This CL is based off of https://chromiumcodereview.appspot.com/10692053/

[vrk (LEFT CHROMIUM), 2012-07-10 03:19:26]

acolwell: This fixes the DCHECK that you ran into at the end of the day today.
Also added unittests (in several combinations!) to catch the bug.

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:591: selected_range_ = NULL;
Another thing I could do to doubly make sure the old seek points aren't
lingering around is I could "unseek" the previous |selected_range_| here before
setting it to NULL. I think we decided against doing this before though because
that was kind of leaking SourceBufferStream details into SourceBufferRange (the
need to "unselect" a range). Any thoughts on this now?

[ivan_cobs.com.sg, 2012-07-10 03:47:27]

Pls remove the following email adds from your list NOW

ivan@cobs.com.sg
lynn@cobs.com.sg

-----Original Message-----
From: vrk@chromium.org [mailto:vrk@chromium.org] 
Sent: Tuesday, 10 July, 2012 11:10 AM
To: acolwell@chromium.org
Cc: chromium-reviews@chromium.org; feature-media-reviews@chromium.org
Subject: [chromium-reviews] Make sure previous seek points in
SourceBufferStream are ignored during overlaps (issue 10766021)

Reviewers: acolwell,

Message:
This CL is based off of https://chromiumcodereview.appspot.com/10692053/

Description:
Make sure previous seek points in SourceBufferStream are ignored during
overlaps

BUG=136463
TEST=media_unittests,seek-sourceabort-crash.html


Please review this at https://chromiumcodereview.appspot.com/10766021/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
   M media/filters/source_buffer_stream.cc
   M media/filters/source_buffer_stream_unittest.cc

[acolwell GONE FROM CHROMIUM, 2012-07-10 17:05:17]

Looks good. Just a couple minor comments.

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
File media/filters/source_buffer_stream.cc (left):

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:205: if (prev != media::kNoTimestamp() &&
prev >= (*itr)->GetStartTimestamp())
I wonder if it makes sense to test the "only selected_range_ is 'seeked'" in
here?

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:514: DCHECK(!*deleted_next_buffer);
DCHECK_EQ(overlapped_range, selected_range_) just in case?

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:591: selected_range_ = NULL;
On 2012/07/10 03:19:26, Victoria Kirst wrote:
> Another thing I could do to doubly make sure the old seek points aren't
> lingering around is I could "unseek" the previous |selected_range_| here
before
> setting it to NULL. I think we decided against doing this before though
because
> that was kind of leaking SourceBufferStream details into SourceBufferRange
(the
> need to "unselect" a range). Any thoughts on this now?
I think it is a good idea to "unseek". You should probably create a
SourceBufferStream::SetSelectedRange(SourceBufferRange* range) and update all
'selected_range_ = ' sites with this method call. I'm assuming "unseek"
essentially means setting next_buffer_index_ to -1.

[vrk (LEFT CHROMIUM), 2012-07-10 19:50:12]

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
File media/filters/source_buffer_stream.cc (left):

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:205: if (prev != media::kNoTimestamp() &&
prev >= (*itr)->GetStartTimestamp())
On 2012/07/10 17:05:17, acolwell wrote:
> I wonder if it makes sense to test the "only selected_range_ is 'seeked'" in
> here?

Since the method is static and I can't refer to |selected_range_| here, I just
added an OnlySelectedRangeIsSeeked() method. I think that makes more sense
anyway.

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:514: DCHECK(!*deleted_next_buffer);
On 2012/07/10 17:05:17, acolwell wrote:
> DCHECK_EQ(overlapped_range, selected_range_) just in case?

Done.

https://chromiumcodereview.appspot.com/10766021/diff/1/media/filters/source_b...
media/filters/source_buffer_stream.cc:591: selected_range_ = NULL;
On 2012/07/10 17:05:17, acolwell wrote:
> On 2012/07/10 03:19:26, Victoria Kirst wrote:
> > Another thing I could do to doubly make sure the old seek points aren't
> > lingering around is I could "unseek" the previous |selected_range_| here
> before
> > setting it to NULL. I think we decided against doing this before though
> because
> > that was kind of leaking SourceBufferStream details into SourceBufferRange
> (the
> > need to "unselect" a range). Any thoughts on this now?
> I think it is a good idea to "unseek". You should probably create a
> SourceBufferStream::SetSelectedRange(SourceBufferRange* range) and update all
> 'selected_range_ = ' sites with this method call. I'm assuming "unseek"
> essentially means setting next_buffer_index_ to -1.

Done.

[acolwell GONE FROM CHROMIUM, 2012-07-10 20:04:31]

LGTM