Bug 123879 – Ended event should be triggered also when playback rate is negative

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Piotr Grad]

Ended event was triggered only when payback was going forward. According to W3C specification ended event should be triggered also when playback rate is negative and reaches the earliest position.

[Piotr Grad]

Created attachment 216147 [details]
Patch & layout test

[Piotr Grad]

Created attachment 216149 [details]
Patch & layout test

[Philippe Normand]

For which version of the spec is this?

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html
Last updated on the 1st of November doesn't have that change.

[Piotr Grad]

(In reply to comment #3)
> For which version of the spec is this?
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html
> Last updated on the 1st of November doesn't have that change.
4.7.10.16 Event summary
...
ended 	Event 	All the slaved media elements have newly ended playback.

and:
4.8.10.8 Playing the media resource
...
A media element is said to have ended playback when:
...
    The current playback position is the earliest possible position, and
    The direction of playback is backwards.

[Eric Carlson]

(In reply to comment #0)
> Ended event was triggered only when payback was going forward. According to W3C specification ended event should be triggered also when playback rate is negative and reaches the earliest position.

This is correct, the latest editor's draft says [1]:

A media element is said to have ended playback when:

    The element's readyState attribute is HAVE_METADATA or greater, and
    Either:

        The current playback position is the end of the media resource, and
        The direction of playback is forwards, and
        Either the media element does not have a loop attribute specified, or the media element has a current media controller.

    Or:

        The current playback position is the earliest possible position, and
        The direction of playback is backwards.

[1] http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#ended-playback

[Eric Carlson]

Comment on attachment 216149 [details]
Patch & layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=216149&action=review

> LayoutTests/media/video-ended-event-negative-playback.html:17
> +                    function ()
> +                    {
> +                        video.currentTime = 2;
> +                        video.playbackRate = -1;
> +                        video.play();
> +                        failTestIn(2500);
> +                    });

This test will always take at least 2 seconds to complete.This seems excessive, can you make it something like .5 instead?

> Source/WebCore/html/HTMLMediaElement.cpp:3679
>      double dur = duration();

Nit: can you please get rid of the "dur" abbreviation while you are here: double duration = this->duration();

[Philippe Normand]

Thanks for the link Eric.

[Piotr Grad]

Created attachment 216268 [details]
Patch & layout test

[Build Bot]

Comment on attachment 216268 [details]
Patch & layout test

Attachment 216268 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22598177

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216276 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Piotr Grad]

Created attachment 216278 [details]
Patch & layout test

loop attribute should be omitted for negative playback rate.

[Build Bot]

Comment on attachment 216278 [details]
Patch & layout test

Attachment 216278 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22728179

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216283 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216278 [details]
Patch & layout test

Attachment 216278 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22608167

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216291 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216278 [details]
Patch & layout test

Attachment 216278 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22508197

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216296 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Eric Carlson]

This is failing on the Mac because the results have an extra blank line:

--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/media/video-ended-event-negative-playback-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/media/video-ended-event-negative-playback-actual.txt
@@ -1,3 +1,4 @@
+
 EVENT(canplaythrough)
 EVENT(ended)
 END OF TEST

[Eric Carlson]

Comment on attachment 216278 [details]
Patch & layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review

> LayoutTests/media/video-ended-event-negative-playback.html:22
> +        <video loop/>

I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag

> Source/WebCore/html/HTMLMediaElement.cpp:3679
>      double dur = duration();

As before, can you please get rid of the "dur" abbreviation while you are here: 

double duration = this->duration();

[Piotr Grad]

Created attachment 216376 [details]
Patch & layout test

[Piotr Grad]

(In reply to comment #19)
> (From update of attachment 216278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review
> 
> > LayoutTests/media/video-ended-event-negative-playback.html:22
> > +        <video loop/>
> 
> I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3679
> >      double dur = duration();
> 
> As before, can you please get rid of the "dur" abbreviation while you are here: 
> 
> double duration = this->duration();

I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope.

[Piotr Grad]

Created attachment 216377 [details]
Pathc & layout test ver.2

[Build Bot]

Comment on attachment 216376 [details]
Patch & layout test

Attachment 216376 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/21248580

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216384 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216377 [details]
Pathc & layout test ver.2

Attachment 216377 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22508518

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216385 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Piotr Grad]

(In reply to comment #26)
> Created an attachment (id=216385) [details]
> Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

This fail is because of regression in mac port.

[Build Bot]

Comment on attachment 216377 [details]
Pathc & layout test ver.2

Attachment 216377 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22728471

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216393 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 216376 [details]
Patch & layout test

Attachment 216376 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22588480

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 216396 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Eric Carlson]

(In reply to comment #21)
> (In reply to comment #19)
> > (From update of attachment 216278 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review
> > 
> > > LayoutTests/media/video-ended-event-negative-playback.html:22
> > > +        <video loop/>
> > 
> > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:3679
> > >      double dur = duration();
> > 
> > As before, can you please get rid of the "dur" abbreviation while you are here: 
> > 
> > double duration = this->duration();
> 
> I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope.

Really? Did you actually try my suggestion?

[Eric Carlson]

(In reply to comment #27)
> (In reply to comment #26)
> > Created an attachment (id=216385) [details] [details]
> > Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
> > 
> > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> > Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
> 
> This fail is because of regression in mac port.

Which regression is that? 

When I apply your patch and run on the Mac, the test fails because it has an extra blank line at the top of the results. I assume this is because of bug 123479, which made media elements a replaced element.

[Piotr Grad]

(In reply to comment #32)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > (From update of attachment 216278 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review
> > > 
> > > > LayoutTests/media/video-ended-event-negative-playback.html:22
> > > > +        <video loop/>
> > > 
> > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag
> > > 
> > > > Source/WebCore/html/HTMLMediaElement.cpp:3679
> > > >      double dur = duration();
> > > 
> > > As before, can you please get rid of the "dur" abbreviation while you are here: 
> > > 
> > > double duration = this->duration();
> > 
> > I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope.
> 
> Really? Did you actually try my suggestion?
Yes, i tried to be 100% sure after your second question about it.

[Piotr Grad]

Created attachment 216436 [details]
Patch & layout test with new line

[Piotr Grad]

(In reply to comment #33)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Created an attachment (id=216385) [details] [details] [details]
> > > Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
> > > 
> > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> > > Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
> > 
> > This fail is because of regression in mac port.
> 
> Which regression is that? 
> 
> When I apply your patch and run on the Mac, the test fails because it has an extra blank line at the top of the results. I assume this is because of bug 123479, which made media elements a replaced element.

Yes, i saw it. Wonder why it was passing on other platforms.

[Eric Carlson]

(In reply to comment #34)
> (In reply to comment #32)
> > (In reply to comment #21)
> > > (In reply to comment #19)
> > > > (From update of attachment 216278 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=216278&action=review
> > > > 
> > > > > LayoutTests/media/video-ended-event-negative-playback.html:22
> > > > > +        <video loop/>
> > > > 
> > > > I wonder if the failure on the Mac is because of this: a video element is not a void element, it must have an end tag
> > > > 
> > > > > Source/WebCore/html/HTMLMediaElement.cpp:3679
> > > > >      double dur = duration();
> > > > 
> > > > As before, can you please get rid of the "dur" abbreviation while you are here: 
> > > > 
> > > > double duration = this->duration();
> > > 
> > > I cannot change 'dur' to 'duration' because then i would get compiler error. Var name cannot override function name used in same scope.
> > 
> > Really? Did you actually try my suggestion?
> Yes, i tried to be 100% sure after your second question about it.

Odd, add "this->" to the method call makes it work for me.

[WebKit Commit Bot]

Comment on attachment 216436 [details]
Patch & layout test with new line

Clearing flags on attachment: 216436

Committed r158965: <http://trac.webkit.org/changeset/158965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This behavior is incorrect. The latest WHATWG spec still says: 

"When the current playback position reaches the earliest possible position of the media resource when the direction of playback is backwards, then the user agent must only queue a task to fire a simple event named timeupdate at the element."

So, while the ended property will return 'true' in this case, it is incorrect to fire an 'ended' event.

[Jer Noble]

Ack, worse than that:

"The ended attribute must return true if, the last time the event loop reached step 1, the media element had ended playback and the direction of playback was forwards, and false otherwise."

So this entire behavior is incorrect.  Currently, we fire an 'ended' event, and return false from video.ended.