Add WebRender error handling to RendererOGL

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Assignee: nobody → sotaro.ikeda.g
Blocks: 1390138
Depends on: 1394337
Summary: Add WebRender error handling to RendererOGL::WebRenderError() → Add WebRender error handling to RendererOGL
This bug is actually a preparation of Bug 1390138.
attachment 8901760 [details] [diff] [review] delivers WebRender error happening until GPUProcessManager, then GPUProcessManager disables WebRender.
Attachment #8901760 - Flags: review?(nical.bugzilla)
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
> 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.
(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.
(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())".
Attachment #8901760 - Attachment is obsolete: true
Attachment #8901760 - Flags: review?(nical.bugzilla)
Attachment #8902106 - Flags: review?(nical.bugzilla)
Comment on attachment 8902106 [details] [diff] [review]
patch - Add WebRender error handling to RendererOGL

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

Thanks!
Attachment #8902106 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919487ecd16f
Add WebRender error handling to RendererOGL r=nical
https://hg.mozilla.org/mozilla-central/rev/919487ecd16f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.