Bug 216017 – <video> element prevents screen from sleeping even after playback finishes

• Component: Media
• Status: RESOLVED FIXED

[Chris Dumez]

<video> element prevents screen from sleeping even after playback finishes.

[Chris Dumez]

<rdar://problem/66665846>

[Chris Dumez]

Created attachment 407634 [details]
Patch

[Chris Dumez]

Created attachment 407636 [details]
Patch

[Darin Adler]

Comment on attachment 407636 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        To address the problem, we now call updateSleepDisabling() when

This says updateSleepDisabling, which makes logical sense.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3171
> +    rateChanged();

This says rateChanged, which makes less sense.

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 407636 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407636&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        To address the problem, we now call updateSleepDisabling() when
> 
> This says updateSleepDisabling, which makes logical sense.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3171
> > +    rateChanged();
> 
> This says rateChanged, which makes less sense.

rateChanged() is what Jer recommended I call. This ends up calling HTMLMediaElement::mediaPlayerRateChanged() which calls updateSleepDisabling(). I initially introduced a new delegate just for this but Jer suggested I rely on the pre-existing rateChanged() instead.

[Jer Noble]

lgtm.

The KVO property which AVFoundation changes does double duty, representing whether the underlying player is playing or paused, but also whether this is due to a network stall or a direct user action.  So the confusion is understandable.  But sending a "rateChanged()" notification is definitely the correct thing here.

[Darin Adler]

(In reply to Jer Noble from comment #6)
> The KVO property which AVFoundation changes does double duty, representing
> whether the underlying player is playing or paused, but also whether this is
> due to a network stall or a direct user action.  So the confusion is
> understandable.  But sending a "rateChanged()" notification is definitely
> the correct thing here.

WebKit doesn’t have to follow the AVFoundation naming. We could come up with a name other than "rate" for this if we like.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> (In reply to Jer Noble from comment #6)
> > The KVO property which AVFoundation changes does double duty, representing
> > whether the underlying player is playing or paused, but also whether this is
> > due to a network stall or a direct user action.  So the confusion is
> > understandable.  But sending a "rateChanged()" notification is definitely
> > the correct thing here.
> 
> WebKit doesn’t have to follow the AVFoundation naming. We could come up with
> a name other than "rate" for this if we like.

That was my original proposal, something like a new pausedStateChanged(). Jer still preferred we call the existing rateChanged() instead of introducing a new delegate so I will let him comment before landing.

[Darin Adler]

We should land this change and debate the naming thing at our leisure separately.

[Jer Noble]

(In reply to Chris Dumez from comment #8)
> (In reply to Darin Adler from comment #7)
> > (In reply to Jer Noble from comment #6)
> > > The KVO property which AVFoundation changes does double duty, representing
> > > whether the underlying player is playing or paused, but also whether this is
> > > due to a network stall or a direct user action.  So the confusion is
> > > understandable.  But sending a "rateChanged()" notification is definitely
> > > the correct thing here.
> > 
> > WebKit doesn’t have to follow the AVFoundation naming. We could come up with
> > a name other than "rate" for this if we like.
> 
> That was my original proposal, something like a new pausedStateChanged().
> Jer still preferred we call the existing rateChanged() instead of
> introducing a new delegate so I will let him comment before landing.

We can't change the behavior here without involving all the other ports; the callbacks are part of an abstraction layer used by all the MediaPlayer subclasses, and just adding new behavior in HTMLMediaElement runs the risk of breaking those ports. Adding a new message only for one engine just because the name of one of our underlying framework's properties is inconsistent is risky, both for us with other MediaPlayer subclasses who would have to start calling that new callback, and for other ports.

I'd be fine with renaming "rateChanged()" to "pausedChanged()" or "playingChanged()". But that's refactoring work that can happen later, coordinated with other ports, in unison.

[Jer Noble]

(In reply to Darin Adler from comment #9)
> We should land this change and debate the naming thing at our leisure
> separately.

Totally.

[EWS]

Committed r266410: <https://trac.webkit.org/changeset/266410>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407636 [details].