Bug 224345 – Need to propagate and use 'canShowWhileLocked' in the GPU Process

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

The visibility propagation view in the GPUProcess is currently always using canShowWhileLocked=false, no matter what the value is on the page.

[Chris Dumez]

<rdar://76011262>

[Chris Dumez]

Created attachment 425559 [details]
Patch

[Chris Dumez]

Created attachment 425560 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 425560 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUProcess.cpp:164
>  

Would it instead make sense here to create one for secure and one insecure and send both to UI process.
Then the UI process could select which one to parent based on which canShowWhileLocked the page in the view has?
(This is assuming that the "secure" applies only when the layer is parented)

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:84
> +    m_contextForVisibilityPropagation = LayerHostingContext::createForExternalHostingProcess({

Is there a race for the UI process to parent this vs the first canvas command to be rasterised?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
> +    });

Fwiw, previously GPU Process had a visibility propagation layer always.

After this modification, GPU Process has visibility propagation layer only if the page has created an canvas.

Not sure how relevant this is, e.g. does lack of visibity propagation correctness affect only canvas rendering, or does it affect other APIs too..

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendCreationParameters.h:42
> +#if PLATFORM(IOS_FAMILY)

For whatever it is worth: 
if I understand correctly, this ifdef  is the reason you want to add this file.
I think adding this kind of intermediary struct is a bit cumbersome, as it creates:
 - maintenance burden: you have to manually type up repetitive code that we have a generator already
 - cognitive burden: why is this struct needed, why is it being stored as a member variable, etc.

I wonder if the IPC generator could be improved so that this sort of stuff was not needed or was generated automatically? It actually, exactly is generated already, it's perhaps just problematic to use.
(Does not help this patch, though)

[Tim Horton]

Comment on attachment 425560 [details]
Patch

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

>> Source/WebKit/GPUProcess/GPUProcess.cpp:164
>>  
> 
> Would it instead make sense here to create one for secure and one insecure and send both to UI process.
> Then the UI process could select which one to parent based on which canShowWhileLocked the page in the view has?
> (This is assuming that the "secure" applies only when the layer is parented)

It seems slightly odd to do *both* when in the 99.999999% case, canShowWhileLocked will be false.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:84
>> +    m_contextForVisibilityPropagation = LayerHostingContext::createForExternalHostingProcess({
> 
> Is there a race for the UI process to parent this vs the first canvas command to be rasterised?

I don't think it matters. It's OK to do work before you've got this parented... if you can (i.e. as long as you don't get suspended).

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
>> +    });
> 
> Fwiw, previously GPU Process had a visibility propagation layer always.
> 
> After this modification, GPU Process has visibility propagation layer only if the page has created an canvas.
> 
> Not sure how relevant this is, e.g. does lack of visibity propagation correctness affect only canvas rendering, or does it affect other APIs too..

Check out the FIXME + added code in WebPage below. FWIW, we always need the visibility propagation view if we need the GPU process to stay awake and do *any* work.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3940
> +    // For now, it is possible to use the GPUProcess for media playback only. Because media does not
> +    // use the RemoteRenderingBackend, we need to force the creation of a RemoteRenderingBackend in
> +    // the GPUProcess so that the page gets a visibility propagation view.

What about WebGL?

[Kimmo Kinnunen]

Comment on attachment 425560 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendCreationParameters.h:44
> +#endif

IIRC this file almost could replaced by just doing
using RemoteRenderingBackendCreationParameters = WebKit::Messages::GPUConnectionToWebProcess::CreateRenderingBackend;
It does not fix the other shortcomings like holding m_parameters, though.

[Kimmo Kinnunen]

(In reply to Tim Horton from comment #5)
> It seems slightly odd to do *both* when in the 99.999999% case, canShowWhileLocked will be false.

But does creating 2 sound odd really? I mean this alternative is that if you have 70 pages, you create 70?


> > Is there a race for the UI process to parent this vs the first canvas command to be rasterised?
> 
> I don't think it matters. It's OK to do work before you've got this
> parented... if you can (i.e. as long as you don't get suspended).

But does that matter? What if the first canvas is created while suspended without visibility layer?

 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
> >> +    });
> > 
> > Fwiw, previously GPU Process had a visibility propagation layer always.
> > 
> > After this modification, GPU Process has visibility propagation layer only if the page has created an canvas.
> > 
> > Not sure how relevant this is, e.g. does lack of visibity propagation correctness affect only canvas rendering, or does it affect other APIs too..
> 
> Check out the FIXME + added code in WebPage below. FWIW, we always need the
> visibility propagation view if we need the GPU process to stay awake and do
> *any* work.
> 
> 
> What about WebGL?

Right, missed that. So this patch forces RRB instantiatation by default, so in practice it does not matter if some specific API not related to RRB needs the visibility propagation (like WebGL).

[Tim Horton]

(In reply to Kimmo Kinnunen from comment #7)
> (In reply to Tim Horton from comment #5)
> > It seems slightly odd to do *both* when in the 99.999999% case, canShowWhileLocked will be false.
> 
> But does creating 2 sound odd really? I mean this alternative is that if you
> have 70 pages, you create 70?

I don't think there's any version of this that doesn't involve >=1 per page (at least, not without a lot of extra logic, keeping track of view/window/etc visibility ourselves), because you need a visibility propagation view per WKWebView, and I don't believe a CAContext can be hosted in more than one place (so you can't have many visibility propagation views hosting the same CAContext from the GPUP).

> 
> > > Is there a race for the UI process to parent this vs the first canvas command to be rasterised?
> > 
> > I don't think it matters. It's OK to do work before you've got this
> > parented... if you can (i.e. as long as you don't get suspended).
> 
> But does that matter? What if the first canvas is created while suspended
> without visibility layer?

You can't create things while you're suspended :) If you don't have the visibility propagation view installed, your process might go to sleep, but whenever the UI process installs it you'll wake back up.

The visibility propagation context is just about forwarding "should I be runnable now?" state for each WKWebView to the GPU process, nothing more.

[Chris Dumez]

(In reply to Tim Horton from comment #5)
> Comment on attachment 425560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425560&action=review
> 
> >> Source/WebKit/GPUProcess/GPUProcess.cpp:164
> >>  
> > 
> > Would it instead make sense here to create one for secure and one insecure and send both to UI process.
> > Then the UI process could select which one to parent based on which canShowWhileLocked the page in the view has?
> > (This is assuming that the "secure" applies only when the layer is parented)
> 
> It seems slightly odd to do *both* when in the 99.999999% case,
> canShowWhileLocked will be false.
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:84
> >> +    m_contextForVisibilityPropagation = LayerHostingContext::createForExternalHostingProcess({
> > 
> > Is there a race for the UI process to parent this vs the first canvas command to be rasterised?
> 
> I don't think it matters. It's OK to do work before you've got this
> parented... if you can (i.e. as long as you don't get suspended).
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
> >> +    });
> > 
> > Fwiw, previously GPU Process had a visibility propagation layer always.
> > 
> > After this modification, GPU Process has visibility propagation layer only if the page has created an canvas.
> > 
> > Not sure how relevant this is, e.g. does lack of visibity propagation correctness affect only canvas rendering, or does it affect other APIs too..
> 
> Check out the FIXME + added code in WebPage below. FWIW, we always need the
> visibility propagation view if we need the GPU process to stay awake and do
> *any* work.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3940
> > +    // For now, it is possible to use the GPUProcess for media playback only. Because media does not
> > +    // use the RemoteRenderingBackend, we need to force the creation of a RemoteRenderingBackend in
> > +    // the GPUProcess so that the page gets a visibility propagation view.
> 
> What about WebGL?

I assumed WebGL was using RemoteRenderingBackend in some way. If it's not, then I can update my FIXME code to create a RemoteRenderingBackend if WebGL in GPUProcess is enabled (but not DOM rendering in GPUProcess).

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Tim Horton from comment #5)
> > Comment on attachment 425560 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=425560&action=review
> > 
> > >> Source/WebKit/GPUProcess/GPUProcess.cpp:164
> > >>  
> > > 
> > > Would it instead make sense here to create one for secure and one insecure and send both to UI process.
> > > Then the UI process could select which one to parent based on which canShowWhileLocked the page in the view has?
> > > (This is assuming that the "secure" applies only when the layer is parented)
> > 
> > It seems slightly odd to do *both* when in the 99.999999% case,
> > canShowWhileLocked will be false.
> > 
> > >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:84
> > >> +    m_contextForVisibilityPropagation = LayerHostingContext::createForExternalHostingProcess({
> > > 
> > > Is there a race for the UI process to parent this vs the first canvas command to be rasterised?
> > 
> > I don't think it matters. It's OK to do work before you've got this
> > parented... if you can (i.e. as long as you don't get suspended).
> > 
> > >> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
> > >> +    });
> > > 
> > > Fwiw, previously GPU Process had a visibility propagation layer always.
> > > 
> > > After this modification, GPU Process has visibility propagation layer only if the page has created an canvas.
> > > 
> > > Not sure how relevant this is, e.g. does lack of visibity propagation correctness affect only canvas rendering, or does it affect other APIs too..
> > 
> > Check out the FIXME + added code in WebPage below. FWIW, we always need the
> > visibility propagation view if we need the GPU process to stay awake and do
> > *any* work.
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3940
> > > +    // For now, it is possible to use the GPUProcess for media playback only. Because media does not
> > > +    // use the RemoteRenderingBackend, we need to force the creation of a RemoteRenderingBackend in
> > > +    // the GPUProcess so that the page gets a visibility propagation view.
> > 
> > What about WebGL?
> 
> I assumed WebGL was using RemoteRenderingBackend in some way. If it's not,
> then I can update my FIXME code to create a RemoteRenderingBackend if WebGL
> in GPUProcess is enabled (but not DOM rendering in GPUProcess).

I have no idea how WebGL in GPUProcess is implementation so Kimmo can be of help here. From my quick look of the code, RemoteGraphicsContextGL requires a RemoteRenderingBackend. Is there any case where WebGL can be used in the GPUProcess without a RemoteRenderingBackend being created?

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #4)
> Comment on attachment 425560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425560&action=review
> 
> > Source/WebKit/GPUProcess/GPUProcess.cpp:164
> >  
> 
> Would it instead make sense here to create one for secure and one insecure
> and send both to UI process.
> Then the UI process could select which one to parent based on which
> canShowWhileLocked the page in the view has?
> (This is assuming that the "secure" applies only when the layer is parented)
> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:84
> > +    m_contextForVisibilityPropagation = LayerHostingContext::createForExternalHostingProcess({
> 
> Is there a race for the UI process to parent this vs the first canvas
> command to be rasterised?
> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:86
> > +    });
> 
> Fwiw, previously GPU Process had a visibility propagation layer always.
> 
> After this modification, GPU Process has visibility propagation layer only
> if the page has created an canvas.
> 
> Not sure how relevant this is, e.g. does lack of visibity propagation
> correctness affect only canvas rendering, or does it affect other APIs too..
> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendCreationParameters.h:42
> > +#if PLATFORM(IOS_FAMILY)
> 
> For whatever it is worth: 
> if I understand correctly, this ifdef  is the reason you want to add this
> file.
> I think adding this kind of intermediary struct is a bit cumbersome, as it
> creates:
>  - maintenance burden: you have to manually type up repetitive code that we
> have a generator already
>  - cognitive burden: why is this struct needed, why is it being stored as a
> member variable, etc.
> 
> I wonder if the IPC generator could be improved so that this sort of stuff
> was not needed or was generated automatically? It actually, exactly is
> generated already, it's perhaps just problematic to use.
> (Does not help this patch, though)

I think that similarly to regular code, you wouldn't want a function to take too many parameters and at some point, you'd switch to a struct. I also think that the struct help abstract away some of the platform-specificities (here one of the data members is iOS-only). I did not come up with this pattern, it is used throughout WebKit. I don't like that the struct is stored as a data member here either. It is only because of how weird IPC::Semaphore is and it not being copyable. I'd be happy to not store the struct as a data member and store a MachPort for the semaphore in the struct (which I think is that I used to do). I actually did this because you did some work to add an IPC encoder / decoder for IPC::Semaphore.

[EWS]

Committed r275768 (236344@main): <https://commits.webkit.org/236344@main>

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