| 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: | Media | Assignee: | 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: |
|
||||||||||||||||
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.
Created attachment 106138 [details]
patch
Emit 'playing' event every time starts playing, after playback ended.
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 > 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.
(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. (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." Created attachment 108471 [details]
updated patch
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 Created attachment 108631 [details]
updated patch
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. (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. Created attachment 108793 [details]
updated patch
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. (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. Created attachment 108795 [details]
updated patch
Comment on attachment 108795 [details] updated patch Clearing flags on attachment: 108795 Committed r96082: <http://trac.webkit.org/changeset/96082> All reviewed patches have been landed. Closing bug. 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 Not that the test added by this test but the one that's rebaselined: LayoutTests/media/video-loop.html (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 *** Bug 42037 has been marked as a duplicate of this bug. *** *** Bug 45203 has been marked as a duplicate of this bug. *** |
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.