Bug 170299

Summary: We should pause silent WebAudio rendering in background tabs
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, eric.carlson, jer.noble
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-03-30 13:40:13 PDT
We should pause silent WebAudio rendering in background tabs since it uses CPU and is not observable by the user.
Comment 1 Chris Dumez 2017-03-30 13:40:25 PDT
<rdar://problem/31289132>
Comment 2 Chris Dumez 2017-03-30 14:29:33 PDT
Created attachment 305902 [details]
WIP Patch
Comment 3 Chris Dumez 2017-03-30 14:33:26 PDT
Created attachment 305904 [details]
WIP Patch
Comment 4 Chris Dumez 2017-03-30 15:47:59 PDT
Created attachment 305915 [details]
WIP Patch
Comment 5 Chris Dumez 2017-03-30 15:48:37 PDT
Created attachment 305917 [details]
WIP Patch
Comment 6 Chris Dumez 2017-03-30 16:25:44 PDT
Created attachment 305923 [details]
Patch
Comment 7 Chris Dumez 2017-03-30 18:25:46 PDT
Created attachment 305933 [details]
Patch
Comment 8 Eric Carlson 2017-04-01 08:23:21 PDT
Comment on attachment 305933 [details]
Patch

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

> Source/WebCore/dom/VisibilityChangeClient.h:35
> +class VisibilityChangeClient {
> +public:
> +    virtual ~VisibilityChangeClient() { }
> +
> +    virtual void visibilityStateChanged() = 0;
> +};

Anders recently suggested I use a lambda rather than a new "client" class for an asynch callback. Maybe you should do the same thing here as long as you have to change all of the call sites?
Comment 9 Chris Dumez 2017-04-01 09:04:47 PDT
(In reply to Eric Carlson from comment #8)
> Comment on attachment 305933 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305933&action=review
> 
> > Source/WebCore/dom/VisibilityChangeClient.h:35
> > +class VisibilityChangeClient {
> > +public:
> > +    virtual ~VisibilityChangeClient() { }
> > +
> > +    virtual void visibilityStateChanged() = 0;
> > +};
> 
> Anders recently suggested I use a lambda rather than a new "client" class
> for an asynch callback. Maybe you should do the same thing here as long as
> you have to change all of the call sites?

Doesn't this make unregistering from the document a bit more awkward though? Previous, we did:
document()->unregisterForVisibilityStateChangedCallbacks(this);

But now, I think we would need to unregister the lambda, which I think would require HTMLMediaElement / AudioContext to keep the lambda they register in a member so they can unregister it later. Or am I missing something easier?
Comment 10 Chris Dumez 2017-04-01 09:06:27 PDT
(In reply to Eric Carlson from comment #8)
> Comment on attachment 305933 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305933&action=review
> 
> > Source/WebCore/dom/VisibilityChangeClient.h:35
> > +class VisibilityChangeClient {
> > +public:
> > +    virtual ~VisibilityChangeClient() { }
> > +
> > +    virtual void visibilityStateChanged() = 0;
> > +};
> 
> Anders recently suggested I use a lambda rather than a new "client" class
> for an asynch callback. Maybe you should do the same thing here as long as
> you have to change all of the call sites?

Note that I totally agree lambdas are better for callback to async operations. But this is not exactly the case for this code. We just want to register / unregister a listener for a specific event (visibility change).
Comment 11 Eric Carlson 2017-04-01 16:51:40 PDT
Comment on attachment 305933 [details]
Patch

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

>>>> Source/WebCore/dom/VisibilityChangeClient.h:35
>>>> +};
>>> 
>>> Anders recently suggested I use a lambda rather than a new "client" class for an asynch callback. Maybe you should do the same thing here as long as you have to change all of the call sites?
>> 
>> Doesn't this make unregistering from the document a bit more awkward though? Previous, we did:
>> document()->unregisterForVisibilityStateChangedCallbacks(this);
>> 
>> But now, I think we would need to unregister the lambda, which I think would require HTMLMediaElement / AudioContext to keep the lambda they register in a member so they can unregister it later. Or am I missing something easier?
> 
> Note that I totally agree lambdas are better for callback to async operations. But this is not exactly the case for this code. We just want to register / unregister a listener for a specific event (visibility change).

I agree, it would be more awkward to use a lambda in a case like this.
Comment 12 Chris Dumez 2017-04-01 17:55:25 PDT
Comment on attachment 305933 [details]
Patch

Clearing flags on attachment: 305933

Committed r214721: <http://trac.webkit.org/changeset/214721>
Comment 13 Chris Dumez 2017-04-01 17:55:26 PDT
All reviewed patches have been landed.  Closing bug.