Bug 60972 – Audio element doesn't emit the 'playing' event every time it starts playing, after it has finished playing

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

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

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.

[Arko Saha]

Created attachment 106138 [details]
patch

Emit 'playing' event every time starts playing, after playback ended.

[Eric Carlson]

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

[Arko Saha]

> 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.

[Eric Carlson]

(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.

[Arun Patole]

(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."

[Arun Patole]

Created attachment 108471 [details]
updated patch

[WebKit Review Bot]

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

[Arun Patole]

Created attachment 108631 [details]
updated patch

[Philippe Normand]

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.

[Arun Patole]

(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.

[Arun Patole]

Created attachment 108793 [details]
updated patch

[Philippe Normand]

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.

[Arun Patole]

(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.

[Arun Patole]

Created attachment 108795 [details]
updated patch

[WebKit Review Bot]

Comment on attachment 108795 [details]
updated patch

Clearing flags on attachment: 108795

Committed r96082: <http://trac.webkit.org/changeset/96082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Not that the test added by this test but the one that's rebaselined: LayoutTests/media/video-loop.html

[Arun Patole]

(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

[Eric Carlson]

*** Bug 42037 has been marked as a duplicate of this bug. ***

[Eric Carlson]

*** Bug 45203 has been marked as a duplicate of this bug. ***