Bug 60972

Summary: Audio element doesn't emit the 'playing' event every time it starts playing, after it has finished playing
Product: WebKit Reporter: Gustavo Noronha (kov) <gustavo>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: arko, arun.patole, dglazkov, eric.carlson, pnormand, rcombs, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
patch
eric.carlson: review-
updated patch
webkit.review.bot: commit-queue-
updated patch
pnormand: review-
updated patch
pnormand: review-
updated patch none

Description Gustavo Noronha (kov) 2011-05-17 10:38:20 PDT
Created attachment 93794 [details]
test case

This behaviour seems to be a bit flaky. In a page with a simple audio element with an onplaying attribute that shows an alert I sometimes get the alert every time I click play, other times I only get it the first time. I wrote a simple layout test that reproduces the problem here on WebKitGTK+. A friend has also reproduced it in Chromium so I believe this is a cross-platform issue.
Comment 1 Gustavo Noronha (kov) 2011-05-17 10:41:18 PDT
This is what I get running that layout test:

RUN(mediaElement.src = findMediaFile('audio', 'content/silence'))
RUN(mediaElement.play())
EVENT(playing)
EVENT(ended)
RUN(mediaElement.play())
EVENT(ended)
END OF TEST

I expected to see another EVENT(playing) between the second RUN(...) and EVENT(ended) there.
Comment 2 Arko Saha 2011-09-02 08:18:46 PDT
Created attachment 106138 [details]
patch

Emit 'playing' event every time starts playing, after playback ended.
Comment 3 Eric Carlson 2011-09-02 08:49:44 PDT
Comment on attachment 106138 [details]
patch

This is not correct, the 'playing' event should only fire when the element goes from the paused state to the playing state. Read the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-play
Comment 4 Arko Saha 2011-09-13 03:09:10 PDT
> This is not correct, the 'playing' event should only fire when the element goes from the paused state to the playing state. Read the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-play

Current spec does not say it but I think, the Paused attribute should also be set to true when :
- The current playback position is the end of the media resource, and the media element does not have a loop attribute specified.

Please let me know your suggestion.
Comment 5 Eric Carlson 2011-09-13 10:47:03 PDT
(In reply to comment #4)
> > This is not correct, the 'playing' event should only fire when the element goes from the paused state to the playing state. Read the spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-play
> 
> Current spec does not say it but I think, the Paused attribute should also be set to true when :
> - The current playback position is the end of the media resource, and the media element does not have a loop attribute specified.
> 
> Please let me know your suggestion.
> 
If you think the spec is incorrect, please file a bug against the spec. You can do this directly from inline issues box in the spec itself, or from http://www.w3.org/Bugs/Public/enter_bug.cgi if you have a W3C account.
Comment 6 Arun Patole 2011-09-23 06:58:48 PDT
(In reply to comment #5)
> If you think the spec is incorrect, please file a bug against the spec. You can do this directly from inline issues box in the spec itself, or from http://www.w3.org/Bugs/Public/enter_bug.cgi if you have a W3C account.

The spec is changed now, as per the latest spec:
http://www.whatwg.org/specs/web-apps/current-work/#ended-playback

"Queue a task that, if the media element does not have a current media controller, and the media element has still ended playback, and the direction of playback is still forwards, and paused is false, changes paused to true and fires a simple event named pause at the media element."
Comment 7 Arun Patole 2011-09-23 06:59:47 PDT
Created attachment 108471 [details]
updated patch
Comment 8 WebKit Review Bot 2011-09-23 09:29:22 PDT
Comment on attachment 108471 [details]
updated patch

Attachment 108471 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9822095

New failing tests:
media/event-attributes.html
svg/custom/svg-fonts-word-spacing.html
media/video-loop.html
Comment 9 Arun Patole 2011-09-26 00:16:56 PDT
Created attachment 108631 [details]
updated patch
Comment 10 Philippe Normand 2011-09-26 08:29:35 PDT
Comment on attachment 108631 [details]
updated patch

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

Looks good in general, just some nits and a speed improvement suggestion for the test.

> Source/WebCore/ChangeLog:8
> +        As per the latest spec, paused attribute should be set to true and fire 'pause' event on end of media playback.

"latest" is vague, if you can't mention the spec revision that introduced this change.
For the rest of the sentence here's my take :)
the paused attribute should be set to true and the media element should emit a 'paused' at the end of playback.

> Source/WebCore/ChangeLog:13
> +        (WebCore::HTMLMediaElement::mediaPlayerTimeChanged): set m_paused to true and schedule 'pause' event if end of playback.

s/if end of playback/when playback ended/

> LayoutTests/ChangeLog:8
> +        As per the latest spec, paused attribute should be set to true and fire 'pause' event on end of media playback.

No need to copy/paste from the other ChangeLog entry.

> LayoutTests/ChangeLog:14
> +        * media/event-attributes-expected.txt: Modified as the 'pause' event is fired on end of media playback.
> +        * media/event-attributes.html: Modified as the 'pause' event is fired on end of media playback.
> +        * media/media-element-play-after-eos-expected.txt: Added.
> +        * media/media-element-play-after-eos.html: Added.
> +        * media/video-loop-expected.txt: Modified as the 'pause' event is fired on end of media playback.

Instead of copy/pasting 3 times the same sentence let's just put it as a general note above.

> LayoutTests/media/media-element-play-after-eos.html:12
> +                run("mediaElement.src = findMediaFile('audio', 'content/silence')");

We usually don't log this, no need for run()

> LayoutTests/media/media-element-play-after-eos.html:32
> +                 run("mediaElement.play()");
> +                 repeated = true;

So the test plays the silence file twice? It's about 1.1 seconds so this test runs in more than 2 seconds. Maybe it'd be worth to perform a seek near the end of the media when the "playing" event is received.
Comment 11 Arun Patole 2011-09-26 23:48:15 PDT
(In reply to comment #10)
Thanks for the review!
> 
> "latest" is vague, if you can't mention the spec revision that introduced this change.
> For the rest of the sentence here's my take :)
> the paused attribute should be set to true and the media element should emit a 'paused' at the end of playback.
Done! Added revision no. and change set url.

> s/if end of playback/when playback ended/
Done!

> No need to copy/paste from the other ChangeLog entry.
> Instead of copy/pasting 3 times the same sentence let's just put it as a general note above.
Ok, modified accordingly.

 
> We usually don't log this, no need for run()
> 
Done!
 
> So the test plays the silence file twice? It's about 1.1 seconds so this test runs in more than 2 seconds. Maybe it'd be worth to perform a seek near the end of the media when the "playing" event is received.
Modified test to seek by 1 sec on 'playing' event.
Comment 12 Arun Patole 2011-09-26 23:49:06 PDT
Created attachment 108793 [details]
updated patch
Comment 13 Philippe Normand 2011-09-27 00:01:52 PDT
Comment on attachment 108793 [details]
updated patch

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

Much better, thanks! Just a last small issue to fix.

> LayoutTests/media/media-element-play-after-eos.html:21
> +                mediaElement.currentTime = 1;    

Usually to seek near the end we use duration - 0.2 or something like that. It's more explicit and more reliable if the test uses another media file in the future.
Comment 14 Arun Patole 2011-09-27 00:21:14 PDT
(In reply to comment #13)
> Usually to seek near the end we use duration - 0.2 or something like that. It's more explicit and more reliable if the test uses another media file in the future.
Yes, you are right. I have modified the test.
Comment 15 Arun Patole 2011-09-27 00:21:55 PDT
Created attachment 108795 [details]
updated patch
Comment 16 WebKit Review Bot 2011-09-27 01:32:27 PDT
Comment on attachment 108795 [details]
updated patch

Clearing flags on attachment: 108795

Committed r96082: <http://trac.webkit.org/changeset/96082>
Comment 17 WebKit Review Bot 2011-09-27 01:32:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Ryosuke Niwa 2011-09-28 17:51:55 PDT
The test added by this test has been failing on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r96082%20(33415)/media/video-does-not-loop-pretty-diff.html
Comment 19 Ryosuke Niwa 2011-09-28 17:54:18 PDT
Not that the test added by this test but the one that's rebaselined: LayoutTests/media/video-loop.html
Comment 20 Arun Patole 2011-09-29 00:34:25 PDT
(In reply to comment #19)
> Not that the test added by this test but the one that's rebaselined: LayoutTests/media/video-loop.html

As discussed, media/video-does-not-loop.html is failing and not the rebaselined: LayoutTests/media/video-loop.html

I have opened bug for this:
https://bugs.webkit.org/show_bug.cgi?id=69067
Comment 21 Eric Carlson 2011-10-10 15:25:02 PDT
*** Bug 42037 has been marked as a duplicate of this bug. ***
Comment 22 Eric Carlson 2011-10-10 15:34:23 PDT
*** Bug 45203 has been marked as a duplicate of this bug. ***