[iOS] The GPU process never runs as a foreground process

[Per Arne Vollan]

Currently, the GPU process always runs in the background, and never goes into the foreground mode, which is required for media playback on iOS.

[Radar WebKit Bug Importer]

<rdar://problem/59818767>

[Per Arne Vollan]

Created attachment 391787 [details]
Patch

[Per Arne Vollan]

Created attachment 391791 [details]
Patch

[Per Arne Vollan]

Created attachment 391805 [details]
Patch

[Brent Fulgham]

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]

gtk & wpe bubbles are red.

[Per Arne Vollan]

Created attachment 391888 [details]
Patch

[Per Arne Vollan]

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

Created attachment 391892 [details]
Patch

[Chris Dumez]

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]

Created attachment 391916 [details]
Patch

[Per Arne Vollan]

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

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

Created attachment 391925 [details]
Patch

[Chris Dumez]

Comment on attachment 391925 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 391925 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 391925 [details]
Patch

Clearing flags on attachment: 391925

Committed r257610: <https://trac.webkit.org/changeset/257610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.