Return EOS when selected_range_ is NULL in SourceBufferStream

Return EOS when selected_range_ is NULL in SourceBufferStream

Fixes the issue where an OnSetDuration call truncates the selected_range_ and resets the
next_buffer_position. kEndOfStream is now returned instead of kNeedBuffer from
GetNextBufferInternal when selected_ranges_ is NULL.

BUG=484992
Committed: https://crrev.com/0fcb1283c641e321545fa809b62bd3b285e8b7ca
Cr-Commit-Position: refs/heads/master@{#329753}

[landell, 2015-05-05 12:48:10]

landell@opera.com changed reviewers:
+ dalecurtis@chromium.org, scherkus@chromium.org, wolenetz@chromium.org

[landell, 2015-05-05 12:55:39]

@reviewers: Please have a look if this make sense.

I have reproduced this issue with our custom media player with this test case: 

http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2015.html...

The issue is timing dependent and does not show itself every time. I did not
reproduct it with content_shell.

This patch is very tentative and I might be all wrong with how this should be
fixed properly. In any case this makes the symptoms go away.

[DaleCurtis, 2015-05-05 17:44:42]

dalecurtis@chromium.org changed reviewers:
+ chcunningham@chromium.org
- dalecurtis@chromium.org, scherkus@chromium.org

[DaleCurtis, 2015-05-05 17:44:43]

-scherkus,+chcunningham

[wolenetz, 2015-05-05 20:51:35]

Thanks for digging into this and finding a potential fix! Would you please also
file a crbug with the repro steps and link it in this CLs description?

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

https://codereview.chromium.org/1119163005/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:1102: if (end_of_stream_ &&
(!selected_range_ || IsEndSelected()))
Interesting. Since IsEndSelected() is only used in two places (here, and in
IsSeekPending()), ISTM that IsSeekPending() might need similar support for
!selected_range_ (&& track_buffer_.empty()).
Would refactoring IsEndSelected() to instead be IsEndOfStreamReached() (and
include conditions involving track_buffer_.empty(), !selected_range_,
end_of_stream_) make both this line and IsSeekPending() more readable?

Ideally, SourceBufferStream unit tests could demonstrate the problem here (with
at least GetNextBufferInternal(), if not also IsSeekPending()). WDYT?

[landell, 2015-05-06 08:59:53]

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

https://codereview.chromium.org/1119163005/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:1102: if (end_of_stream_ &&
(!selected_range_ || IsEndSelected()))
On 2015/05/05 20:51:35, wolenetz wrote:
> Interesting. Since IsEndSelected() is only used in two places (here, and in
> IsSeekPending()), ISTM that IsSeekPending() might need similar support for
> !selected_range_ (&& track_buffer_.empty()).
> Would refactoring IsEndSelected() to instead be IsEndOfStreamReached() (and
> include conditions involving track_buffer_.empty(), !selected_range_,
> end_of_stream_) make both this line and IsSeekPending() more readable?
> 

I agree. That is how I wanted to do it but I went for the less intrusive change
since I was unsure if this check could mess things up in the seeking case. I'll
make the change. 

> Ideally, SourceBufferStream unit tests could demonstrate the problem here
(with
> at least GetNextBufferInternal(), if not also IsSeekPending()). WDYT?

I'll see if I can come up with a unit test that demonstrates this. Thanks for
the input.

[landell, 2015-05-06 10:46:53]

Let's see if these changes was what you had in mind.

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

https://codereview.chromium.org/1119163005/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:1102: if (end_of_stream_ &&
(!selected_range_ || IsEndSelected()))
On 2015/05/05 20:51:35, wolenetz wrote:
> Interesting. Since IsEndSelected() is only used in two places (here, and in
> IsSeekPending()), ISTM that IsSeekPending() might need similar support for
> !selected_range_ (&& track_buffer_.empty()).
> Would refactoring IsEndSelected() to instead be IsEndOfStreamReached() (and
> include conditions involving track_buffer_.empty(), !selected_range_,
> end_of_stream_) make both this line and IsSeekPending() more readable?
> 
> Ideally, SourceBufferStream unit tests could demonstrate the problem here
(with
> at least GetNextBufferInternal(), if not also IsSeekPending()). WDYT?

Done.

[landell, 2015-05-12 09:29:50]

Ping. Do you have time to look at my latest changes?

[wolenetz, 2015-05-12 14:17:45]

On 2015/05/12 09:29:50, landell wrote:
> Ping. Do you have time to look at my latest changes?

Sorry, yes, I'll take a look today. Was OOO end of last week and am catching up.
Thank you for your patience on this one.

[wolenetz, 2015-05-12 20:36:23]

wolenetz@chromium.org changed reviewers:
- chcunningham@chromium.org

[wolenetz, 2015-05-12 20:36:24]

lgtm % nits.
Thanks for adding tests!
R-=chcunningham@ (I ok'ed this R with Chris offline.)

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:931: return !IsEndOfStreamReached() &&
seek_pending_;
nit: please swap the order of the evaluation here to enable rapid short-circuit
evaluation in the common case of seek_pending_ == false.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1100: 
nit: probably good for protection against later regression to add
DCHECK(track_buffer_.empty()) here now.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1212: if (ranges_.empty())
nit: I think it might be possible in an edge case (or in later regression) to
have something in track_buffer_, but for ranges_ to be empty. In this case, we
haven't reached end of stream since a non-empty track_buffer_ has more buffers
to be served by GetNextBuffer() still.

Suggestion: change line 1209 to be:
  if (!end_of_stream_ || !track_buffer_.empty())
and remove the track_buffer_.empty() condition from line 1221.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1221: if (!selected_range_ &&
track_buffer_.empty())
nit: see my comment, above.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.h:279: // or there is a pending seek beyond
any existing ranges.
nit: Please adjust this comment, too: there are new conditions using
end_of_stream_, track_buffer_.empty(), and !selected_range_.

[wolenetz, 2015-05-12 20:38:07]

Also, (sorry for spam), please update the CL description to remove the last
paragraph, and add something like "Fixes the issue where..." to the beginning of
the first paragraph.

[landell, 2015-05-13 09:55:57]

Fixed the remaining nitpicks. I let you have a final look before comitting.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:931: return !IsEndOfStreamReached() &&
seek_pending_;
On 2015/05/12 20:36:24, wolenetz wrote:
> nit: please swap the order of the evaluation here to enable rapid
short-circuit
> evaluation in the common case of seek_pending_ == false.

Done.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1100: 
On 2015/05/12 20:36:23, wolenetz wrote:
> nit: probably good for protection against later regression to add
> DCHECK(track_buffer_.empty()) here now.

Done.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1212: if (ranges_.empty())
On 2015/05/12 20:36:23, wolenetz wrote:
> nit: I think it might be possible in an edge case (or in later regression) to
> have something in track_buffer_, but for ranges_ to be empty. In this case, we
> haven't reached end of stream since a non-empty track_buffer_ has more buffers
> to be served by GetNextBuffer() still.
> 
> Suggestion: change line 1209 to be:
>   if (!end_of_stream_ || !track_buffer_.empty())
> and remove the track_buffer_.empty() condition from line 1221.

Done.

https://codereview.chromium.org/1119163005/diff/20001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1221: if (!selected_range_ &&
track_buffer_.empty())
On 2015/05/12 20:36:24, wolenetz wrote:
> nit: see my comment, above.

Done.

[wolenetz, 2015-05-13 18:23:02]

One nit was unfixed and another nit was mis-fixed :)
Please fix the following (and lgtm still % these nits):

https://codereview.chromium.org/1119163005/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1119163005/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1103: DCHECK(track_buffer_.empty());
nit: Sorry, I meant to have this DCHECK be stronger by putting it right before
line 1101. Please move it there.

https://codereview.chromium.org/1119163005/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1119163005/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.h:279: // or there is a pending seek beyond
any existing ranges.
nit ping :) :
Please update this comment per my nit on CR of PS2.

[landell, 2015-05-13 19:18:21]

Sorry, I guess I am unfamilitar with both this system and this piece of code.

Hope I got it right this time.

[wolenetz, 2015-05-13 23:00:51]

On 2015/05/13 19:18:21, landell wrote:
> Sorry, I guess I am unfamilitar with both this system and this piece of code.
> 
> Hope I got it right this time.

lgtm thanks. I'll check the CQ bit shortly for you.

[wolenetz, 2015-05-13 23:00:58]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2015-05-13 23:01:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119163005/60001

[commit-bot: I haz the power, 2015-05-14 00:10:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-14 00:11:14]

Patchset 4 (id:??) landed as
https://crrev.com/0fcb1283c641e321545fa809b62bd3b285e8b7ca
Cr-Commit-Position: refs/heads/master@{#329753}