Bug 123879 (endedrate) - Ended event should be triggered also when playback rate is negative
Summary: Ended event should be triggered also when playback rate is negative
Status: RESOLVED FIXED
Alias: endedrate
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 135253
  Show dependency treegraph
 
Reported: 2013-11-06 00:47 PST by Piotr Grad
Modified: 2014-07-24 13:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch & layout test (5.16 KB, patch)
2013-11-06 00:49 PST, Piotr Grad
no flags Details | Formatted Diff | Diff
Patch & layout test (5.20 KB, patch)
2013-11-06 00:54 PST, Piotr Grad
eric.carlson: review+
Details | Formatted Diff | Diff
Patch & layout test (5.20 KB, patch)
2013-11-07 00:35 PST, Piotr Grad
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (453.82 KB, application/zip)
2013-11-07 01:32 PST, Build Bot
no flags Details
Patch & layout test (5.25 KB, patch)
2013-11-07 01:43 PST, Piotr Grad
eric.carlson: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (488.29 KB, application/zip)
2013-11-07 03:13 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (499.14 KB, application/zip)
2013-11-07 04:44 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (449.00 KB, application/zip)
2013-11-07 05:41 PST, Build Bot
no flags Details
Patch & layout test (5.22 KB, patch)
2013-11-08 05:17 PST, Piotr Grad
buildbot: commit-queue-
Details | Formatted Diff | Diff
Pathc & layout test ver.2 (5.23 KB, patch)
2013-11-08 05:22 PST, Piotr Grad
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (555.60 KB, application/zip)
2013-11-08 06:04 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (452.37 KB, application/zip)
2013-11-08 06:12 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (495.44 KB, application/zip)
2013-11-08 06:56 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (451.69 KB, application/zip)
2013-11-08 07:15 PST, Build Bot
no flags Details
Patch & layout test with new line (5.22 KB, patch)
2013-11-08 14:08 PST, Piotr Grad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Grad 2013-11-06 00:47:17 PST
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.
Comment 1 Piotr Grad 2013-11-06 00:49:17 PST
Created attachment 216147 [details]
Patch & layout test
Comment 2 Piotr Grad 2013-11-06 00:54:40 PST
Created attachment 216149 [details]
Patch & layout test
Comment 3 Philippe Normand 2013-11-06 03:19:10 PST
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.
Comment 4 Piotr Grad 2013-11-06 03:46:42 PST
(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.
Comment 5 Eric Carlson 2013-11-06 10:48:04 PST
(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
Comment 6 Eric Carlson 2013-11-06 10:55:35 PST
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();
Comment 7 Philippe Normand 2013-11-06 11:11:09 PST
Thanks for the link Eric.
Comment 8 Piotr Grad 2013-11-07 00:35:35 PST
Created attachment 216268 [details]
Patch & layout test
Comment 9 Build Bot 2013-11-07 01:32:30 PST
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
Comment 10 Build Bot 2013-11-07 01:32:32 PST
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
Comment 11 Piotr Grad 2013-11-07 01:43:51 PST
Created attachment 216278 [details]
Patch & layout test

loop attribute should be omitted for negative playback rate.
Comment 12 Build Bot 2013-11-07 03:13:22 PST
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
Comment 13 Build Bot 2013-11-07 03:13:25 PST
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
Comment 14 Build Bot 2013-11-07 04:44:03 PST
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
Comment 15 Build Bot 2013-11-07 04:44:06 PST
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
Comment 16 Build Bot 2013-11-07 05:41:16 PST
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
Comment 17 Build Bot 2013-11-07 05:41:20 PST
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
Comment 18 Eric Carlson 2013-11-07 08:42:07 PST
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
Comment 19 Eric Carlson 2013-11-07 08:46:43 PST
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();
Comment 20 Piotr Grad 2013-11-08 05:17:46 PST
Created attachment 216376 [details]
Patch & layout test
Comment 21 Piotr Grad 2013-11-08 05:19:37 PST
(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.
Comment 22 Piotr Grad 2013-11-08 05:22:03 PST
Created attachment 216377 [details]
Pathc & layout test ver.2
Comment 23 Build Bot 2013-11-08 06:04:32 PST
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
Comment 24 Build Bot 2013-11-08 06:04:37 PST
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
Comment 25 Build Bot 2013-11-08 06:12:25 PST
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
Comment 26 Build Bot 2013-11-08 06:12:28 PST
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
Comment 27 Piotr Grad 2013-11-08 06:15:02 PST
(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.
Comment 28 Build Bot 2013-11-08 06:56:02 PST
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
Comment 29 Build Bot 2013-11-08 06:56:06 PST
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
Comment 30 Build Bot 2013-11-08 07:15:15 PST
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
Comment 31 Build Bot 2013-11-08 07:15:18 PST
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
Comment 32 Eric Carlson 2013-11-08 09:11:39 PST
(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?
Comment 33 Eric Carlson 2013-11-08 09:20:11 PST
(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.
Comment 34 Piotr Grad 2013-11-08 14:00:34 PST
(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.
Comment 35 Piotr Grad 2013-11-08 14:08:05 PST
Created attachment 216436 [details]
Patch & layout test with new line
Comment 36 Piotr Grad 2013-11-08 14:38:29 PST
(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.
Comment 37 Eric Carlson 2013-11-08 14:53:24 PST
(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.
Comment 38 WebKit Commit Bot 2013-11-08 15:43:24 PST
Comment on attachment 216436 [details]
Patch & layout test with new line

Clearing flags on attachment: 216436

Committed r158965: <http://trac.webkit.org/changeset/158965>
Comment 39 WebKit Commit Bot 2013-11-08 15:43:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 Jer Noble 2014-07-24 12:54:28 PDT
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.
Comment 41 Jer Noble 2014-07-24 13:24:21 PDT
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.