Adjust video-durationchange unit test for new behavior

Adjust video-durationchange unit test for new behavior

Previous change that introduced this unit test caused MSE playback
regressions. So I'm making some adjustments to media element behavior
at the end of stream, see this CL for details:
https://codereview.chromium.org/1069253004/
After that CL we will no longer do Pipeline::SetDuration when ending
playback, instead we'll rely on currentTime() being the same as media
duration after WebMediaPlayerImpl detects the actual playback end event
via OnPipelineEnded callback.

BUG=475244

[servolk, 2015-04-09 00:57:37]

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

[DaleCurtis, 2015-04-09 01:22:31]

This will require a 3 sided patch unfortunately. You should just disable the
blink test for now and then make the change to chromium after blink rolls. Then
re-enable the test with the right values afterward.

[servolk, 2015-04-09 01:34:12]

On 2015/04/09 01:22:31, DaleCurtis wrote:
> This will require a 3 sided patch unfortunately. You should just disable the
> blink test for now and then make the change to chromium after blink rolls.
Then
> re-enable the test with the right values afterward.

Yeah,you are right, here is a CL to disable that test for now
https://codereview.chromium.org/1071823002/

[wolenetz, 2015-04-09 17:29:06]

https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
LayoutTests/media/video-durationchange-on-ended.html:25:
testExpected("video.duration <= initialReportedDuration", true);
Though I understand how this causes the test to pass, it contradicts the goal of
this layout test, which is to verify that durationchange fires on truncated
duration files, along with confirming the situation of an actually-truncated
duration.

IIUC, the Chromium CL (https://codereview.chromium.org/1069253004/) fixes the
ended firing behavior when the truncation is minimal, but regresses reporting
the actual duration in that case.

Ideally, the right fix would be to retain both correct ended firing behavior
(for both MSE and src=) as well as reporting the actual truncated duration (even
if the truncation is tiny). Any ideas how to fix this? Do I misunderstand? If I
don't, please at least file a crbug to fix the minimal truncation not being
reported correctly, and adjust this layout test's "content/truncated.webm" to
have > 250ms duration truncation and retain the strict "<" check here.

[servolk, 2015-04-09 17:54:03]

On 2015/04/09 17:29:06, wolenetz wrote:
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> File LayoutTests/media/video-durationchange-on-ended.html (right):
> 
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> LayoutTests/media/video-durationchange-on-ended.html:25:
> testExpected("video.duration <= initialReportedDuration", true);
> Though I understand how this causes the test to pass, it contradicts the goal
of
> this layout test, which is to verify that durationchange fires on truncated
> duration files, along with confirming the situation of an actually-truncated
> duration.
> 
> IIUC, the Chromium CL (https://codereview.chromium.org/1069253004/) fixes the
> ended firing behavior when the truncation is minimal, but regresses reporting
> the actual duration in that case.
> 
> Ideally, the right fix would be to retain both correct ended firing behavior
> (for both MSE and src=) as well as reporting the actual truncated duration
(even
> if the truncation is tiny). Any ideas how to fix this? Do I misunderstand? If
I
> don't, please at least file a crbug to fix the minimal truncation not being
> reported correctly, and adjust this layout test's "content/truncated.webm" to
> have > 250ms duration truncation and retain the strict "<" check here.

Yes, your understanding is correct. I also had some doubts about how to fix this
best. But I've decided that handling the case of truncated stream is less
important than properly ending MSE playback (crbug.com/475244), so I've decided
to go ahead with CL 1069253004 for now.
In the future I think we'll need to do two more things:
1. Explicitly tell HTMLMediaElement that stream playback has ended, instead of
having HTMLMediaElement rely on comparing currentTime() with duration() (see
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
and crbug.com/409280).
2. I think that in order to properly handle truncated input streams, we need to
handle this on the demuxer level. After all the demuxer is the final authority
on this, it should know for sure both the expected duration of the stream (read
from .mp4/.webm header) and the actual timestamp of the last data buffer that it
was able to read from the stream. So the final duration update should happen on
the demuxer level and not on the media::Pipeline level. The regression described
in crbug.com/475244 happened precisely because of that - the duration was
updated on the media::Pipeline level, but was not updated on the demuxer level.
But we can probably reuse crbug.com/438581 for this - it is still open after
all, the code that I'm changing was intended to be a temporary workaround for
that issue anyway, and it turns out it was a wrong workaround.
Let me know if you think we need to open yet another bug for this.

[DaleCurtis, 2015-04-09 18:06:07]

I don't think we want to add any further complexity to the demuxer, we've had no
end of issues with this ended === currentTime==duration business, so lets just
kill it in favor of an explicit signal.

[wolenetz, 2015-04-09 18:06:54]

On 2015/04/09 17:54:03, servolk wrote:
> On 2015/04/09 17:29:06, wolenetz wrote:
> >
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> > File LayoutTests/media/video-durationchange-on-ended.html (right):
> > 
> >
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> > LayoutTests/media/video-durationchange-on-ended.html:25:
> > testExpected("video.duration <= initialReportedDuration", true);
> > Though I understand how this causes the test to pass, it contradicts the
goal
> of
> > this layout test, which is to verify that durationchange fires on truncated
> > duration files, along with confirming the situation of an actually-truncated
> > duration.
> > 
> > IIUC, the Chromium CL (https://codereview.chromium.org/1069253004/) fixes
the
> > ended firing behavior when the truncation is minimal, but regresses
reporting
> > the actual duration in that case.
> > 
> > Ideally, the right fix would be to retain both correct ended firing behavior
> > (for both MSE and src=) as well as reporting the actual truncated duration
> (even
> > if the truncation is tiny). Any ideas how to fix this? Do I misunderstand?
If
> I
> > don't, please at least file a crbug to fix the minimal truncation not being
> > reported correctly, and adjust this layout test's "content/truncated.webm"
to
> > have > 250ms duration truncation and retain the strict "<" check here.
> 
> Yes, your understanding is correct. I also had some doubts about how to fix
this
> best. But I've decided that handling the case of truncated stream is less
> important than properly ending MSE playback (crbug.com/475244), so I've
decided
> to go ahead with CL 1069253004 for now.
> In the future I think we'll need to do two more things:
> 1. Explicitly tell HTMLMediaElement that stream playback has ended, instead of
> having HTMLMediaElement rely on comparing currentTime() with duration() (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> and crbug.com/409280).
> 2. I think that in order to properly handle truncated input streams, we need
to
> handle this on the demuxer level. After all the demuxer is the final authority
> on this, it should know for sure both the expected duration of the stream
(read
> from .mp4/.webm header) and the actual timestamp of the last data buffer that
it
> was able to read from the stream. So the final duration update should happen
on
> the demuxer level and not on the media::Pipeline level. The regression
described
> in crbug.com/475244 happened precisely because of that - the duration was
> updated on the media::Pipeline level, but was not updated on the demuxer
level.
> But we can probably reuse crbug.com/438581 for this - it is still open after
> all, the code that I'm changing was intended to be a temporary workaround for
> that issue anyway, and it turns out it was a wrong workaround.
> Let me know if you think we need to open yet another bug for this.

crb/438581 sgtm, with a comment added to it speaking to the impact on incorrect
src= duration on slightly-truncated media. Thanks for looking into these bugs!

[wolenetz, 2015-04-09 18:10:17]

Also, please don't forget to update the TestExpectations ;)

[servolk, 2015-04-09 18:28:55]

On 2015/04/09 18:06:54, wolenetz wrote:
> On 2015/04/09 17:54:03, servolk wrote:
> > On 2015/04/09 17:29:06, wolenetz wrote:
> > >
> >
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> > > File LayoutTests/media/video-durationchange-on-ended.html (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
> > > LayoutTests/media/video-durationchange-on-ended.html:25:
> > > testExpected("video.duration <= initialReportedDuration", true);
> > > Though I understand how this causes the test to pass, it contradicts the
> goal
> > of
> > > this layout test, which is to verify that durationchange fires on
truncated
> > > duration files, along with confirming the situation of an
actually-truncated
> > > duration.
> > > 
> > > IIUC, the Chromium CL (https://codereview.chromium.org/1069253004/) fixes
> the
> > > ended firing behavior when the truncation is minimal, but regresses
> reporting
> > > the actual duration in that case.
> > > 
> > > Ideally, the right fix would be to retain both correct ended firing
behavior
> > > (for both MSE and src=) as well as reporting the actual truncated duration
> > (even
> > > if the truncation is tiny). Any ideas how to fix this? Do I misunderstand?
> If
> > I
> > > don't, please at least file a crbug to fix the minimal truncation not
being
> > > reported correctly, and adjust this layout test's "content/truncated.webm"
> to
> > > have > 250ms duration truncation and retain the strict "<" check here.
> > 
> > Yes, your understanding is correct. I also had some doubts about how to fix
> this
> > best. But I've decided that handling the case of truncated stream is less
> > important than properly ending MSE playback (crbug.com/475244), so I've
> decided
> > to go ahead with CL 1069253004 for now.
> > In the future I think we'll need to do two more things:
> > 1. Explicitly tell HTMLMediaElement that stream playback has ended, instead
of
> > having HTMLMediaElement rely on comparing currentTime() with duration() (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> > and crbug.com/409280).
> > 2. I think that in order to properly handle truncated input streams, we need
> to
> > handle this on the demuxer level. After all the demuxer is the final
authority
> > on this, it should know for sure both the expected duration of the stream
> (read
> > from .mp4/.webm header) and the actual timestamp of the last data buffer
that
> it
> > was able to read from the stream. So the final duration update should happen
> on
> > the demuxer level and not on the media::Pipeline level. The regression
> described
> > in crbug.com/475244 happened precisely because of that - the duration was
> > updated on the media::Pipeline level, but was not updated on the demuxer
> level.
> > But we can probably reuse crbug.com/438581 for this - it is still open after
> > all, the code that I'm changing was intended to be a temporary workaround
for
> > that issue anyway, and it turns out it was a wrong workaround.
> > Let me know if you think we need to open yet another bug for this.
> 
> crb/438581 sgtm, with a comment added to it speaking to the impact on
incorrect
> src= duration on slightly-truncated media. Thanks for looking into these bugs!

Ok, I've added a comment to crbug.com/438581 and I agree with Dale - having
explicit 'ended' signal might even eliminate the need to modify the demuxer
(although that would still need to be tested).

[servolk, 2015-04-29 21:31:02]

https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
File LayoutTests/media/video-durationchange-on-ended.html (right):

https://codereview.chromium.org/1074633002/diff/1/LayoutTests/media/video-dur...
LayoutTests/media/video-durationchange-on-ended.html:25:
testExpected("video.duration <= initialReportedDuration", true);
On 2015/04/09 17:29:06, wolenetz wrote:
> Though I understand how this causes the test to pass, it contradicts the goal
of
> this layout test, which is to verify that durationchange fires on truncated
> duration files, along with confirming the situation of an actually-truncated
> duration.
> 
> IIUC, the Chromium CL (https://codereview.chromium.org/1069253004/) fixes the
> ended firing behavior when the truncation is minimal, but regresses reporting
> the actual duration in that case.
> 
> Ideally, the right fix would be to retain both correct ended firing behavior
> (for both MSE and src=) as well as reporting the actual truncated duration
(even
> if the truncation is tiny). Any ideas how to fix this? Do I misunderstand? If
I
> don't, please at least file a crbug to fix the minimal truncation not being
> reported correctly, and adjust this layout test's "content/truncated.webm" to
> have > 250ms duration truncation and retain the strict "<" check here.

Sorry for the delay on this. Yes, your understanding is correct. I think in
order to fix this properly we need to:
a. Have an explicit 'pipeline ended' signal from media pipeline to blink,
instead of having blink rely on currentTime/duration comparison to detect eos
(crbug.com/438581)
b. When blink receives that 'pipeline ended' signal, it should re-check the
stream duration and fire 'durationchanged' event if the actual duration turns
out to be less than expected.

[philipj_slow, 2015-04-29 21:57:43]

On 2015/04/29 21:31:02, servolk wrote:
> Sorry for the delay on this. Yes, your understanding is correct. I think in
> order to fix this properly we need to:
> a. Have an explicit 'pipeline ended' signal from media pipeline to blink,
> instead of having blink rely on currentTime/duration comparison to detect eos
> (crbug.com/438581)
> b. When blink receives that 'pipeline ended' signal, it should re-check the
> stream duration and fire 'durationchanged' event if the actual duration turns
> out to be less than expected.

That sounds great to me, issues around this seem to keep coming up and this
ought to be the cure.

[blink-reviews, 2015-04-29 22:05:41]

See also http://code.google.com/p/chromium/issues/detail?id=475244#c6.
I'm planning to do this, but it would clash with Srirama CL
https://codereview.chromium.org/1055503002/. Let's give it a few more days
and if that CL hasn't landed by then, I guess I can go ahead and still make
the change, even if it causes some conflicts for Srirama.

On Wed, Apr 29, 2015 at 2:57 PM, <philipj@opera.com> wrote:

> On 2015/04/29 21:31:02, servolk wrote:
>
>> Sorry for the delay on this. Yes, your understanding is correct. I think
>> in
>> order to fix this properly we need to:
>> a. Have an explicit 'pipeline ended' signal from media pipeline to blink,
>> instead of having blink rely on currentTime/duration comparison to detect
>> eos
>> (crbug.com/438581)
>> b. When blink receives that 'pipeline ended' signal, it should re-check
>> the
>> stream duration and fire 'durationchanged' event if the actual duration
>> turns
>> out to be less than expected.
>>
>
> That sounds great to me, issues around this seem to keep coming up and this
> ought to be the cure.
>
> https://codereview.chromium.org/1074633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.