Bug 208250

Summary: [iOS] The GPU process never runs as a foreground process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, eric.carlson, jer.noble, jonlee, nham, peng.liu6, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=203547
Bug Depends on:    
Bug Blocks: 205123    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Per Arne Vollan 2020-02-26 11:04:28 PST 
Currently, the GPU process always runs in the background, and never goes into the foreground mode, which is required for media playback on iOS.
Comment 1 Radar WebKit Bug Importer 2020-02-26 13:27:27 PST 
<rdar://problem/59818767>
Comment 2 Per Arne Vollan 2020-02-26 15:22:53 PST 
Created attachment 391787 [details]
Patch
Comment 3 Per Arne Vollan 2020-02-26 15:40:43 PST 
Created attachment 391791 [details]
Patch
Comment 4 Per Arne Vollan 2020-02-26 16:30:14 PST 
Created attachment 391805 [details]
Patch
Comment 5 Brent Fulgham 2020-02-27 09:41:38 PST 
Comment on attachment 391805 [details]
Patch

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

This looks good, but I think you should have Ben and Chris review.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> +    , m_throttler(*this, true)

Check with Ben Nham to make sure this doesn't impact page load performance.
Comment 6 Chris Dumez 2020-02-27 09:48:59 PST 
gtk & wpe bubbles are red.
Comment 7 Per Arne Vollan 2020-02-27 10:13:33 PST 
Created attachment 391888 [details]
Patch
Comment 8 Per Arne Vollan 2020-02-27 10:17:18 PST 
(In reply to Brent Fulgham from comment #5)
> Comment on attachment 391805 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391805&action=review
> 
> This looks good, but I think you should have Ben and Chris review.
> 
> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> > +    , m_throttler(*this, true)
> 
> Check with Ben Nham to make sure this doesn't impact page load performance.

Ben, do you think this change will have an impact on PLT?

Thanks for reviewing!
Comment 9 Per Arne Vollan 2020-02-27 10:53:38 PST 
Created attachment 391892 [details]
Patch
Comment 10 Chris Dumez 2020-02-27 12:07:38 PST 
Comment on attachment 391892 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:2045
> +            if (auto gpuProcess = GPUProcessProxy::singletonIfCreated())

Now we take process assertions for the GPU process in 2 places, here and in existing GPUProcessProxy::updateProcessAssertion() code. This seems wrong.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> +    , m_throttler(*this, true)

Seems like this should be using processPool.shouldTakeUIBackgroundAssertion()

> Source/WebKit/UIProcess/ios/WKContentView.mm:263
> +- (void)_removeVisibilityPropagationViewForGPUProcess

Who is calling this?
Comment 11 Per Arne Vollan 2020-02-27 14:19:54 PST 
Created attachment 391916 [details]
Patch
Comment 12 Per Arne Vollan 2020-02-27 14:47:16 PST 
(In reply to Chris Dumez from comment #10)
> Comment on attachment 391892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391892&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:2045
> > +            if (auto gpuProcess = GPUProcessProxy::singletonIfCreated())
> 
> Now we take process assertions for the GPU process in 2 places, here and in
> existing GPUProcessProxy::updateProcessAssertion() code. This seems wrong.
> 

I removed this, since there is existing code in place in GPUProcessProxy::updateProcessAssertion(). It does not seem like we update the assertions dynamically though. Should this be addressed in a later patch?

> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> > +    , m_throttler(*this, true)
> 
> Seems like this should be using processPool.shouldTakeUIBackgroundAssertion()
> 

I reverted this since the related code was also reverted.

> > Source/WebKit/UIProcess/ios/WKContentView.mm:263
> > +- (void)_removeVisibilityPropagationViewForGPUProcess
> 
> Who is calling this?

Fixed. This is now being called when the GPU process crash.

Thanks for reviewing!
Comment 13 Chris Dumez 2020-02-27 14:52:30 PST 
Comment on attachment 391916 [details]
Patch

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

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> +    , m_throttler(*this, true)

We have a configuration flag for this: processPool.shouldTakeUIBackgroundAssertion()
If the client says not to take UI assertions, it seems wrong to do so.

NetworkProcessProxy, WebProcessProxy all rely on processPool.shouldTakeUIBackgroundAssertion(). What makes the GPU process different?
Comment 14 Per Arne Vollan 2020-02-27 14:55:04 PST 
(In reply to Chris Dumez from comment #13)
> Comment on attachment 391916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391916&action=review
> 
> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:127
> > +    , m_throttler(*this, true)
> 
> We have a configuration flag for this:
> processPool.shouldTakeUIBackgroundAssertion()
> If the client says not to take UI assertions, it seems wrong to do so.
> 
> NetworkProcessProxy, WebProcessProxy all rely on
> processPool.shouldTakeUIBackgroundAssertion(). What makes the GPU process
> different?

It does not seem we have an associated processPool for the GPU Process, since it is a singleton. Would you prefer to take the value from one of the pools, e.g. the first one?
Comment 15 Per Arne Vollan 2020-02-27 15:01:10 PST 
Created attachment 391925 [details]
Patch
Comment 16 Chris Dumez 2020-02-27 15:01:46 PST 
Comment on attachment 391925 [details]
Patch

r=me
Comment 17 Per Arne Vollan 2020-02-27 17:54:16 PST 
Comment on attachment 391925 [details]
Patch

Thanks for reviewing!
Comment 18 WebKit Commit Bot 2020-02-27 18:42:03 PST 
Comment on attachment 391925 [details]
Patch

Clearing flags on attachment: 391925

Committed r257610: <https://trac.webkit.org/changeset/257610>
Comment 19 WebKit Commit Bot 2020-02-27 18:42:05 PST 
All reviewed patches have been landed.  Closing bug.