Closed
Bug 1394338
Opened 7 years ago
Closed 7 years ago
Add WebRender error handling to RendererOGL
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(1 file, 1 obsolete file)
15.25 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•7 years ago
|
Summary: Add WebRender error handling to RendererOGL::WebRenderError() → Add WebRender error handling to RendererOGL
Assignee | ||
Comment 1•7 years ago
|
||
This bug is actually a preparation of Bug 1390138.
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
attachment 8901760 [details] [diff] [review] delivers WebRender error happening until GPUProcessManager, then GPUProcessManager disables WebRender.
Assignee | ||
Updated•7 years ago
|
Attachment #8901760 -
Flags: review?(nical.bugzilla)
Comment 4•7 years ago
|
||
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•7 years ago
|
||
> 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.
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
(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())".
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8901760 -
Attachment is obsolete: true
Attachment #8901760 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c559fabdc0d67fd4b6137522c3a07196879945
Assignee | ||
Updated•7 years ago
|
Attachment #8902106 -
Flags: review?(nical.bugzilla)
Comment 10•7 years ago
|
||
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+
Comment 11•7 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/919487ecd16f Add WebRender error handling to RendererOGL r=nical
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/919487ecd16f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•