1394338 - Add WebRender error handling to RendererOGL

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Comment 1

[Sotaro Ikeda [:sotaro]]

This bug is actually a preparation of Bug 1390138.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add WebRender error handling to RendererOGL

Comment 3

[Sotaro Ikeda [:sotaro]]

attachment 8901760 [details] [diff] [review] delivers WebRender error happening until GPUProcessManager, then GPUProcessManager disables WebRender.

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8901760 [details] [diff] [review]
patch - Add WebRender error handling to RendererOGL

Review of attachment 8901760 [details] [diff] [review]:
-----------------------------------------------------------------

I am not super found of how the information has to go through the child process to take effect. The choice of whether to allow webrender should be entirely controlled by the parent side and the child process should only try to ask for webrender and get whatever the parent side decides is best. This patch is built on top of the logic of the initialization of webrender that is overly complicated and started this awkward mix of the decision being split between the child and the parent sides.

It should be possible to just tell the parent process that webrender is having trouble without talking to the child process and let the child process fail to initialize a layer manager next time it tries to (since it will have to ask the parent process). That way when we get to fix the initialization code we won't have to fix the error notification as well.

::: gfx/ipc/GPUProcessManager.cpp
@@ +452,5 @@
>  
>  void
> +GPUProcessManager::NotifyWebRenderError()
> +{
> +  if (!gfx::gfxVars::UseWebRender()) {

Nit: is there a reason we'd receive WebRenderError notifications without webrender being enabled? If not I'd rather keep it simple because reading this implies that the situation is more complicated than it should be.

@@ +455,5 @@
> +{
> +  if (!gfx::gfxVars::UseWebRender()) {
> +    return;
> +  }
> +  DisableWebRender();

This function (DisableWebrender) appears to have been (confusingly) written specifically for the initialization of webrender. If you want to re-purpose it for other reasons webrender should be disabled, please add some parameters to report the correct information to about:support (right now it will claim that we failed to initialize webrender).

::: gfx/webrender_bindings/RendererOGL.h
@@ +90,5 @@
>  
>    wr::Renderer* GetRenderer() { return mRenderer; }
>  
>  protected:
> +  void NoitfyWebRenderError();

Typo: Noit -> Noti

Comment 5

[Nicolas Silva [:nical]]

> I am not super found of how the information has to go through the child process to take effect.

Sorry, I totally misunderstood the code and, as Kats pointed out to me, the information transits through the main thread of the parent process and not the child process.
There are still nits to address in the patch but I'm ok with it overall.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #4)
> Comment on attachment 8901760 [details] [diff] [review]
> patch - Add WebRender error handling to RendererOGL
> 
> Review of attachment 8901760 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: gfx/ipc/GPUProcessManager.cpp
> @@ +452,5 @@
> >  
> >  void
> > +GPUProcessManager::NotifyWebRenderError()
> > +{
> > +  if (!gfx::gfxVars::UseWebRender()) {
> 
> Nit: is there a reason we'd receive WebRenderError notifications without
> webrender being enabled? If not I'd rather keep it simple because reading
> this implies that the situation is more complicated than it should be.

Multiple NotifyWebRenderError() could be delivered for WebRender. They are async, therefore NotifyWebRenderError() could be received after disabling WebRender. I am going to add a comment there.

> 
> @@ +455,5 @@
> > +{
> > +  if (!gfx::gfxVars::UseWebRender()) {
> > +    return;
> > +  }
> > +  DisableWebRender();
> 
> This function (DisableWebrender) appears to have been (confusingly) written
> specifically for the initialization of webrender. If you want to re-purpose
> it for other reasons webrender should be disabled, please add some
> parameters to report the correct information to about:support (right now it
> will claim that we failed to initialize webrender).

I am going to update in a next patch.

> 
> ::: gfx/webrender_bindings/RendererOGL.h
> @@ +90,5 @@
> >  
> >    wr::Renderer* GetRenderer() { return mRenderer; }
> >  
> >  protected:
> > +  void NoitfyWebRenderError();
> 
> Typo: Noit -> Noti

I am going to update in a next patch.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro  PTO 1/Sep - 13/Sep] from comment #6)
> 
> Multiple NotifyWebRenderError() could be delivered for WebRender. They are
> async, therefore NotifyWebRenderError() could be received after disabling
> WebRender. I am going to add a comment there.

I changed my mind, I will update DisableWebRender() as to remove an assert "MOZ_ASSERT(gfx::gfxVars::UseWebRender())".

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add WebRender error handling to RendererOGL

Comment 9

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c559fabdc0d67fd4b6137522c3a07196879945

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8902106 [details] [diff] [review]
patch - Add WebRender error handling to RendererOGL

Review of attachment 8902106 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919487ecd16f
Add WebRender error handling to RendererOGL r=nical

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/919487ecd16f