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

Per Arne Vollan
Reported 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.
Attachments
Patch (25.53 KB, patch)
2020-02-26 15:22 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (24.64 KB, patch)
2020-02-26 15:40 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (24.68 KB, patch)
2020-02-26 16:30 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (24.67 KB, patch)
2020-02-27 10:13 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (24.76 KB, patch)
2020-02-27 10:53 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (22.12 KB, patch)
2020-02-27 14:19 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Patch (21.79 KB, patch)
2020-02-27 15:01 PST, Per Arne Vollan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-02-26 13:27:27 PST
<rdar://problem/59818767>
Per Arne Vollan
Comment 2 2020-02-26 15:22:53 PST
Created attachment 391787 [details] Patch
Per Arne Vollan
Comment 3 2020-02-26 15:40:43 PST
Created attachment 391791 [details] Patch
Per Arne Vollan
Comment 4 2020-02-26 16:30:14 PST
Created attachment 391805 [details] Patch
Brent Fulgham
Comment 5 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.
Chris Dumez
Comment 6 2020-02-27 09:48:59 PST
gtk & wpe bubbles are red.
Per Arne Vollan
Comment 7 2020-02-27 10:13:33 PST
Created attachment 391888 [details] Patch
Per Arne Vollan
Comment 8 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!
Per Arne Vollan
Comment 9 2020-02-27 10:53:38 PST
Created attachment 391892 [details] Patch
Chris Dumez
Comment 10 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?
Per Arne Vollan
Comment 11 2020-02-27 14:19:54 PST
Created attachment 391916 [details] Patch
Per Arne Vollan
Comment 12 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!
Chris Dumez
Comment 13 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?
Per Arne Vollan
Comment 14 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?
Per Arne Vollan
Comment 15 2020-02-27 15:01:10 PST
Created attachment 391925 [details] Patch
Chris Dumez
Comment 16 2020-02-27 15:01:46 PST
Comment on attachment 391925 [details] Patch r=me
Per Arne Vollan
Comment 17 2020-02-27 17:54:16 PST
Comment on attachment 391925 [details] Patch Thanks for reviewing!
WebKit Commit Bot
Comment 18 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>
WebKit Commit Bot
Comment 19 2020-02-27 18:42:05 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.