Fix MSE playback regression by not changing pipeline duration at EOS

Fix MSE playback regression by not changing pipeline duration at EOS

There has been a regression in MSE stream playback, that I tracked down
to this CL:
https://codereview.chromium.org/740663002/
I think the root cause of the issue is that
Pipeline::RunEndedCallbackIfNeeded invokes SetDuration(media_time) -
this sets media duration on the media::Pipeline object level, but not
on the demuxer level. In case of URL playback that works, since for URL
playback HTMLMediaElement::duration gets the actual duration from the
media::Pipeline (via WebMediaPlayerImpl::duration). But in case of MSE
playback HTMLMediaElement::duration gets duration directly from the
demuxer (via WebMediaSourceImpl::duration) and that causes mismatch
between the HTMLMediaElement::duration and
HTMLMediaElement::currentTime, which causes HTMLMediaElement to never
send the 'ended' event.

I think we shouldn't need to do this SetDuration on the pipeline
level, since WebMediaPlayerImpl::currentTime will return duration()
value after end-of-stream has been processed by media::Pipeline (see
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmediaplayer_impl.cc&rcl=1428421466&l=446)
And another part of CL 740663002 will make sure Pipeline::GetMediaTime
also returns duration_ after end-of-stream has been reached by renderer

I've verified this CL by running media_unittests and internal
Chromecast MSE unit tests.

BUG=475244
Committed: https://crrev.com/86a0801b082cb38f4c6f80d52336fca620d0d4fe
Cr-Commit-Position: refs/heads/master@{#324470}

[servolk, 2015-04-08 22:23:23]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, lcwu@chromium.org, srirama.m@samsung.com,
wolenetz@chromium.org

[DaleCurtis, 2015-04-08 22:27:03]

Hmm, do layout tests still pass after this?

[servolk, 2015-04-08 22:33:40]

On 2015/04/08 22:27:03, DaleCurtis wrote:
> Hmm, do layout tests still pass after this?

Haven't tried layout tests yet, just kicked off 'git cl try'. We'll see.
But I've confirmed that this CL fixes Chromecast MSE unit test failure.

[servolk, 2015-04-09 00:46:25]

On 2015/04/08 22:33:40, servolk wrote:
> On 2015/04/08 22:27:03, DaleCurtis wrote:
> > Hmm, do layout tests still pass after this?
> 
> Haven't tried layout tests yet, just kicked off 'git cl try'. We'll see.
> But I've confirmed that this CL fixes Chromecast MSE unit test failure.

Yeah, you were right, media/video-durationchange-on-ended.html fails after this
change. This test was added by Srirama soon after making the change that I'm
trying to partially revert here. The test expects that the actual video.duration
value at the time when 'ended' event is fired will be strictly less than the
initially reported duration (apparently the test uses a truncated .webm file as
input, so it expects the initial duration to be read from the .webm header and
the actual duration to be reported at the end of playback). So as far as I
understand this CL might break playback of truncated media files (but only if
they are truncated by 250ms or less), but on the other hand it will restore
proper logic for sending the 'ended' event at the end of MSE streams. So I'd
still like to go ahead with this change and fix the unit test for now (I'll do
it in a separate CL, since that unit test is in Blink). Let me know if you
disagree.

[servolk, 2015-04-09 00:56:55]

On 2015/04/09 00:46:25, servolk wrote:
> On 2015/04/08 22:33:40, servolk wrote:
> > On 2015/04/08 22:27:03, DaleCurtis wrote:
> > > Hmm, do layout tests still pass after this?
> > 
> > Haven't tried layout tests yet, just kicked off 'git cl try'. We'll see.
> > But I've confirmed that this CL fixes Chromecast MSE unit test failure.
> 
> Yeah, you were right, media/video-durationchange-on-ended.html fails after
this
> change. This test was added by Srirama soon after making the change that I'm
> trying to partially revert here. The test expects that the actual
video.duration
> value at the time when 'ended' event is fired will be strictly less than the
> initially reported duration (apparently the test uses a truncated .webm file
as
> input, so it expects the initial duration to be read from the .webm header and
> the actual duration to be reported at the end of playback). So as far as I
> understand this CL might break playback of truncated media files (but only if
> they are truncated by 250ms or less), but on the other hand it will restore
> proper logic for sending the 'ended' event at the end of MSE streams. So I'd
> still like to go ahead with this change and fix the unit test for now (I'll do
> it in a separate CL, since that unit test is in Blink). Let me know if you
> disagree.

A CL to update the unit test:
https://codereview.chromium.org/1074633002

[servolk, 2015-04-09 16:52:51]

On 2015/04/09 00:56:55, servolk wrote:
> On 2015/04/09 00:46:25, servolk wrote:
> > On 2015/04/08 22:33:40, servolk wrote:
> > > On 2015/04/08 22:27:03, DaleCurtis wrote:
> > > > Hmm, do layout tests still pass after this?
> > > 
> > > Haven't tried layout tests yet, just kicked off 'git cl try'. We'll see.
> > > But I've confirmed that this CL fixes Chromecast MSE unit test failure.
> > 
> > Yeah, you were right, media/video-durationchange-on-ended.html fails after
> this
> > change. This test was added by Srirama soon after making the change that I'm
> > trying to partially revert here. The test expects that the actual
> video.duration
> > value at the time when 'ended' event is fired will be strictly less than the
> > initially reported duration (apparently the test uses a truncated .webm file
> as
> > input, so it expects the initial duration to be read from the .webm header
and
> > the actual duration to be reported at the end of playback). So as far as I
> > understand this CL might break playback of truncated media files (but only
if
> > they are truncated by 250ms or less), but on the other hand it will restore
> > proper logic for sending the 'ended' event at the end of MSE streams. So I'd
> > still like to go ahead with this change and fix the unit test for now (I'll
do
> > it in a separate CL, since that unit test is in Blink). Let me know if you
> > disagree.
> 
> A CL to update the unit test:
> https://codereview.chromium.org/1074633002

Ok, now that the blink unit test for this is temporarily disabled
(https://codereview.chromium.org/1071823002/), can I get approval for this CL?

[DaleCurtis, 2015-04-09 16:55:55]

lgtm, since the code which needs this isn't in yet.

[DaleCurtis, 2015-04-09 16:56:05]

(/ was reverted)

[servolk, 2015-04-09 16:58:42]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-04-09 16:59:13]

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

[wolenetz, 2015-04-09 17:34:03]

lgtm also, though please see my comments on your layout test CL for filing
follow-up crbug to fix even the minor truncation issue.

[commit-bot: I haz the power, 2015-04-09 17:54:52]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-09 17:55:42]

Patchset 1 (id:??) landed as
https://crrev.com/86a0801b082cb38f4c6f80d52336fca620d0d4fe
Cr-Commit-Position: refs/heads/master@{#324470}