Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14840015:
Lose/restore WebGL contexts if multisampling blackist status changes at runtime. (Closed)
|
Created:
7 years, 7 months ago by bajones Modified:
7 years, 7 months ago CC:
blink-reviews, danakj, jamesr, Stephen Chennney, jeez, Zhenyao Mo Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionLose/restore WebGL contexts if multisampling blackist status changes at runtime.
Also added a mechanism to listen for changes to the blacklist status.
BUG=237931
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150231
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed to a more generic SettingsChangedObserver #
Total comments: 13
Patch Set 3 : Reverted back to MultisampleChangedObserver, addressed feedback #Patch Set 4 : Forgot to remove DrawingBuffer change #
Total comments: 9
Patch Set 5 : Refactored WebGLRenderingContext to inherit from MultisampleChangedObserver #Patch Set 6 : Missed one of kbr's suggestions in previous patch #
Total comments: 1
Patch Set 7 : Changed to using an explicit registration flag and looking up the page on destruct #Patch Set 8 : Replaced erroniously removed default attributes #
Total comments: 1
Patch Set 9 : Changed settings to pass by const reference #
Messages
Total messages: 31 (0 generated)
Not sure who the appropriate reviewers would be for the Page and Settings changes
https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in File Source/core/page/Settings.in (left): https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in#... Source/core/page/Settings.in:79: openGLMultisamplingEnabled initial=true This CL is going in the wrong direction. We want to autogenerate more of the Settings object over time, rather than less. If you need a notification pathway for this data, I'd recommend moving this data to another objects. Settings is just a passive store of data.
On 2013/05/06 15:19:19, abarth wrote: > https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in > File Source/core/page/Settings.in (left): > > https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in#... > Source/core/page/Settings.in:79: openGLMultisamplingEnabled initial=true > This CL is going in the wrong direction. We want to autogenerate more of the > Settings object over time, rather than less. If you need a notification pathway > for this data, I'd recommend moving this data to another objects. Settings is > just a passive store of data. Hey Brandon, Welcome to reviewer hell. Where instead of telling you how to solve the thing you are trying to solve in a way they will approve they just give you a terse "No! Bad!" :-(
https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in File Source/core/page/Settings.in (left): https://codereview.chromium.org/14840015/diff/1/Source/core/page/Settings.in#... Source/core/page/Settings.in:79: openGLMultisamplingEnabled initial=true On 2013/05/06 15:19:20, abarth wrote: > This CL is going in the wrong direction. We want to autogenerate more of the > Settings object over time, rather than less. If you need a notification pathway > for this data, I'd recommend moving this data to another objects. Settings is > just a passive store of data. We don't want to invent a completely new way of passing down this single bit of data. There is already extensive support for updating Settings on the fly via WebPreferences and ViewMsg_UpdateWebPreferences. It is used by the Settings UI among other things. There are already hooks in Settings.cpp which notify the Page of various changes, such as image loading, changes to the font family, etc., which may trigger style recalc and other things in response. Talking with dglazkov, I think a good direction would be to add a more generic observer to watch for updates to the page's Settings. Then this setting would not need to be handwritten. What do you think? If you think this is not a good direction, then please propose an alternative. I don't think cloning the entire Settings/WebPreferences infrastructure is palatable.
On 2013/05/06 17:29:28, greggman wrote: > Hey Brandon, Welcome to reviewer hell. Where instead of telling you how to solve > the thing you are trying to solve in a way they will approve they just give you > a terse "No! Bad!" :-( Sorry, I didn't mean to cause reviewer hell. I was just asked to comment on whether this specific aspect of the change. I don't have enough context to understand the problem you're trying to solve in enough depth to recommend a better solution.
On 2013/05/06 17:43:16, kbr wrote: > We don't want to invent a completely new way of passing down this single bit of > data. There is already extensive support for updating Settings on the fly via > WebPreferences and ViewMsg_UpdateWebPreferences. It is used by the Settings UI > among other things. There are already hooks in Settings.cpp which notify the > Page of various changes, such as image loading, changes to the font family, > etc., which may trigger style recalc and other things in response. I understand that there is already code that has a notification pathway. We're in the process of removing that code. > Talking with dglazkov, I think a good direction would be to add a more generic > observer to watch for updates to the page's Settings. Then this setting would > not need to be handwritten. What do you think? That's certainly better than the existing hand-written notifications. If we go that route, what separates Settings from all the other Page-scoped state? > If you think this is not a good > direction, then please propose an alternative. I don't think cloning the entire > Settings/WebPreferences infrastructure is palatable. I don't have enough information about the problem you're trying to solve to suggest a better approach. For example, the description of this CL doesn't explain anything about the problem you're trying to solve. It's just two sentences long. In another perspective, why should Page have a dozen or two lines about OpenGL multi-sampling? That's not really related to the core concept of Page. If we continue down that route, Page will be many thousands of lines long. I'm sorry that I don't have a better solution for you, but it's hard to come up with solutions without understanding the problem you're trying to solve.
@abarth: I've refactored the Settings notification code to be a more generic "SettingsChanged" notifier rather than anything GL specific. Do you have further concerns about this method of communicating the state? (The Chrome side code for this CL is in https://codereview.chromium.org/14947002/ if that's needed for reference)
On 2013/05/06 19:42:47, abarth wrote: > On 2013/05/06 17:43:16, kbr wrote: > > Talking with dglazkov, I think a good direction would be to add a more generic > > observer to watch for updates to the page's Settings. Then this setting would > > not need to be handwritten. What do you think? > > That's certainly better than the existing hand-written notifications. If we go > that route, what separates Settings from all the other Page-scoped state? The split isn't clear, but in my mind, Settings contains a bunch of switches affecting web page rendering which aren't exposed at the API level. Other Page-scoped state which is handled separately, like page visibility (setVisibilityState), is exposed to JavaScript. > > If you think this is not a good > > direction, then please propose an alternative. I don't think cloning the > entire > > Settings/WebPreferences infrastructure is palatable. > > I don't have enough information about the problem you're trying to solve to > suggest a better approach. For example, the description of this CL doesn't > explain anything about the problem you're trying to solve. It's just two > sentences long. Sorry about that -- but the bug report contains the full description of the problem. If it isn't clear please let us know. We've started putting complete documentation in the bug report rather than the CL because CLs are transient, and difficult to find after the fact. > In another perspective, why should Page have a dozen or two lines about OpenGL > multi-sampling? That's not really related to the core concept of Page. If we > continue down that route, Page will be many thousands of lines long. Yes, Brandon and I both agree with you. Hopefully Brandon's new patch looks more appealing.
The linked bug description makes me think that we should be losing the WebGL context when the multisampling value changes, which is something I'd expect to happen entirely in the gpu process and not involve Blink at all. (aside: Having information about the bug in the bug is great, but information about what the code in a particular patch does is specific to that commit and should be in the patch description.)
On 2013/05/06 20:41:56, jamesr wrote: > The linked bug description makes me think that we should be losing the WebGL > context when the multisampling value changes, which is something I'd expect to > happen entirely in the gpu process and not involve Blink at all. You're right and we're working toward this, but the infrastructure isn't present yet to do this, and this issue (which is a top reliability problem reported by the MapsGL team) needs to be worked around now. The workaround will need to be merged back to M27, too. > (aside: Having information about the bug in the bug is great, but information > about what the code in a particular patch does is specific to that commit and > should be in the patch description.) Noted. (I think the existing description sums it up, but it could be expanded.) We'll add more detail to CLs going forward.
This approach seems like it won't scale very well. If we have N settings, each of which has one observer interested in it, then we'll end up with N^2 calls to notify each of them about all the N-1 settings changes that they don't care about. https://codereview.chromium.org/14840015/diff/8001/Source/WebKit/chromium/pub... File Source/WebKit/chromium/public/WebSettings.h (right): https://codereview.chromium.org/14840015/diff/8001/Source/WebKit/chromium/pub... Source/WebKit/chromium/public/WebSettings.h:55: virtual void settingsChanged() const = 0; I'm surprised this is a public API. Is the embedder required to call this function any time it calls any setters on this object? If so, why not skip exposing it as an API and make the setters themselves call it? https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... Source/core/html/canvas/WebGLRenderingContext.cpp:5502: if(m_multisamplingAllowed != enabled) { nit: missing a space after "if" https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... File Source/core/html/canvas/WebGLRenderingContext.h (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... Source/core/html/canvas/WebGLRenderingContext.h:785: Page* m_page; This object is RefCounted but it holds raw pointers to these objects... How are the lifetimes of this object and these pointers related? https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.cpp#... Source/core/page/Page.cpp:843: (*it)->settingsChanged(m_settings.get()); This is a weak iteration pattern. What if calling settingsChanged mutates m_settingsChangedObservers? https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.h File Source/core/page/Page.h (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.h#ne... Source/core/page/Page.h:265: virtual ~SettingsChangedObserver() {} Shouldn't this destructor call removeSettingsChangedObserver? It seems like every object that subclasses this object will need to make that call in its destructor to be correct. Similarly, perhaps the constructor should call addSettingsChangedObserver. https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.h#ne... Source/core/page/Page.h:353: HashSet<SettingsChangedObserver*> m_settingsChangedObservers; More raw pointers... Can this be HashSet<OwnPtr<...> > instead?
Ok, here's what I think we should do: 1) Return to the approach of patchset #1 where this CL is implemented as a one-off for this setting in Settings.cpp. 2) File a bug about moving this logic from Blink to the GPU process and add a comment to the code in Settings.cpp referencing that bug. I still think this is the wrong direction for the Settings code, but I don't really want to argue about it more.
> This approach seems like it won't scale very well. If we have N settings, each > of which has one observer interested in it, then we'll end up with N^2 calls to > notify each of them about all the N-1 settings changes that they don't care > about. The intent is to broadcast one "settings changed notification" after the embedder changes all of the settings, which is how it's currently done (in src/webkit/glue/webpreferences.cc.) https://codereview.chromium.org/14840015/diff/8001/Source/WebKit/chromium/pub... File Source/WebKit/chromium/public/WebSettings.h (right): https://codereview.chromium.org/14840015/diff/8001/Source/WebKit/chromium/pub... Source/WebKit/chromium/public/WebSettings.h:55: virtual void settingsChanged() const = 0; On 2013/05/06 21:58:30, abarth wrote: > I'm surprised this is a public API. Is the embedder required to call this > function any time it calls any setters on this object? If so, why not skip > exposing it as an API and make the setters themselves call it? It's not possible to detect internally when would be a good point to broadcast the settings changed notifications. Fortunately, there's only one point in the embedder that needs to call this method. https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... Source/core/html/canvas/WebGLRenderingContext.cpp:5502: if(m_multisamplingAllowed != enabled) { The logic here doesn't seem right. If the app didn't request antialiasing in the first place then changes to openGLMultisamplingEnabled shouldn't force a lost context. Context loss should be forced in two situations: 1) The context is currently using antialiasing, but the settings change now forbids it. 2) The context would have been using antialiasing, settings prohibited it, but the settings change enables it again. Please code these up to be as readable as possible. https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... Source/core/platform/graphics/gpu/DrawingBuffer.cpp:86: bool multisampleSupported = extensions->maySupportMultisampling() Per my comment on the Chromium-side CL, I think this should probably be handled differently, and no change to this code should be made.
https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/html/canvas/We... Source/core/html/canvas/WebGLRenderingContext.cpp:5502: if(m_multisamplingAllowed != enabled) { On 2013/05/06 22:12:22, kbr wrote: > The logic here doesn't seem right. If the app didn't request antialiasing in the > first place then changes to openGLMultisamplingEnabled shouldn't force a lost > context. Good point. I should be able to do that without too many gymnastics. https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.h File Source/core/page/Page.h (right): https://codereview.chromium.org/14840015/diff/8001/Source/core/page/Page.h#ne... Source/core/page/Page.h:265: virtual ~SettingsChangedObserver() {} On 2013/05/06 21:58:30, abarth wrote: > Shouldn't this destructor call removeSettingsChangedObserver? It seems like > every object that subclasses this object will need to make that call in its > destructor to be correct. > > Similarly, perhaps the constructor should call addSettingsChangedObserver. Not a bad idea. I'll make sure that's in the next revision. https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... Source/core/platform/graphics/gpu/DrawingBuffer.cpp:86: bool multisampleSupported = extensions->maySupportMultisampling() On 2013/05/06 22:12:22, kbr wrote: > Per my comment on the Chromium-side CL, I think this should probably be handled > differently, and no change to this code should be made. I can put this line back for clarity, but it's not doing what you think it's doing. Following the call chain down to WebGraphics3D it is hard coded to return true. The actual query of the multisampling blacklist entry is done in the multisample() call via m_attributes.antialias
https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... File Source/core/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/14840015/diff/8001/Source/core/platform/graph... Source/core/platform/graphics/gpu/DrawingBuffer.cpp:86: bool multisampleSupported = extensions->maySupportMultisampling() On 2013/05/06 22:44:14, bajones wrote: > On 2013/05/06 22:12:22, kbr wrote: > > Per my comment on the Chromium-side CL, I think this should probably be > handled > > differently, and no change to this code should be made. > I can put this line back for clarity, but it's not doing what you think it's > doing. Following the call chain down to WebGraphics3D it is hard coded to return > true. > > The actual query of the multisampling blacklist entry is done in the > multisample() call via m_attributes.antialias Let's please leave this line alone then. It sounds like a separate CL should just delete Extensions3D::maySupportMultisampling().
Alright, back to a multisampling-specific observer with some feedback addressed.
https://codereview.chromium.org/14840015/diff/27001/Source/core/html/canvas/W... File Source/core/html/canvas/WebGLRenderingContext.h (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/html/canvas/W... Source/core/html/canvas/WebGLRenderingContext.h:777: class WebGLMultisamplingChangedObserver : public Page::MultisamplingChangedObserver, public RefCounted<WebGLMultisamplingChangedObserver> { Why is this object RefCounted? Given that it's RefCounted, how do we know that m_page and m_context remain alive for the lifetime of this object? Page::MultisamplingChangedObserver already has a Page* member. Do we really need to duplicate it here? https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Page.cpp... Source/core/page/Page.cpp:844: (*it)->multisamplingChanged(m_settings->openGLMultisamplingEnabled()); Can this call end up executing JavaScript or destroying the Page? https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Settings... File Source/core/page/Settings.cpp (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Settings... Source/core/page/Settings.cpp:390: if(m_openGLMultisamplingEnabled == flag) Missing a space between "if" and "(". https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Settings... Source/core/page/Settings.cpp:394: m_page->multisamplingChanged(); We discussed filing a bug about removing this code and having a comment here that referenced that bug.
This looks pretty good to me overall, but a few issues. https://codereview.chromium.org/14840015/diff/27001/Source/core/html/canvas/W... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/html/canvas/W... Source/core/html/canvas/WebGLRenderingContext.cpp:454: GraphicsContext3D::Attributes adjustAttributes(GraphicsContext3D::Attributes attributes, Settings* settings) { Passing the GraphicsContext3D::Attributes by value, mutating it and then returning a copy is a little gross and confusing to read. Prefer to pass a const reference, allocate a new GraphicsContext3D::Attributes on the stack in this function copying the input, and adjust the outgoing copy. https://codereview.chromium.org/14840015/diff/27001/Source/core/html/canvas/W... Source/core/html/canvas/WebGLRenderingContext.cpp:659: if (m_multisamplingChangedObserver == 0 && m_requestedAttributes.antialias) Nit: style guide would say "if (!m_multisamplingChangedObserver && ...". https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Page.cpp... Source/core/page/Page.cpp:838: } It's confusing to me that the base class MultisamplingChangedObserver adds and removes itself from the Page's hash set. I would recommend instead making MultisamplingChangedObserver a pure virtual class with a single method, add add/removeMultisamplingChangedObserver methods to Page, and just make WebGLRenderingContext itself implement MultisamplingChangedObserver, and remove itself from the Page's set in its destructor (since Page outlives WebGLRenderingContext). https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Page.cpp... Source/core/page/Page.cpp:844: (*it)->multisamplingChanged(m_settings->openGLMultisamplingEnabled()); On 2013/05/07 22:10:59, abarth wrote: > Can this call end up executing JavaScript or destroying the Page? Actually, it's guaranteed that it doesn't. It starts a timer which dispatches an event to JavaScript. https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Settings.h File Source/core/page/Settings.h (right): https://codereview.chromium.org/14840015/diff/27001/Source/core/page/Settings... Source/core/page/Settings.h:201: bool m_openGLMultisamplingEnabled : 1; This new bit is uninitialized in the constructor in the .cpp file.
Addressed kbr's feedback and refactored the WebGLRenderingContext to be the MultisampleChangedObserver as he suggested offline.
I much prefer the structure in this patch to the previous one. LGTM https://codereview.chromium.org/14840015/diff/33008/Source/core/html/canvas/W... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/14840015/diff/33008/Source/core/html/canvas/W... Source/core/html/canvas/WebGLRenderingContext.cpp:657: m_multisamplingObserverHost = p; Is it necessary to cache the Page like this? Would it work to make the same chain of calls in the destructor, or is it too late (is the Page no longer referenceable)?
On 2013/05/09 00:17:13, kbr wrote: > I much prefer the structure in this patch to the previous one. LGTM > > https://codereview.chromium.org/14840015/diff/33008/Source/core/html/canvas/W... > File Source/core/html/canvas/WebGLRenderingContext.cpp (right): > > https://codereview.chromium.org/14840015/diff/33008/Source/core/html/canvas/W... > Source/core/html/canvas/WebGLRenderingContext.cpp:657: > m_multisamplingObserverHost = p; > Is it necessary to cache the Page like this? Would it work to make the same > chain of calls in the destructor, or is it too late (is the Page no longer > referenceable)? We need some flag to indicate the the observer has been registered, this is simply killing two birds with one stone. I think the call chain should still be valid in the destructor if that's preferable, though.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/14840015/39001
Retried try job too often on mac_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/14840015/39001
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
bajones, have you run the layout tests locally with your change? It's suspicious that the glsl-conformance.html test is reliably failing on try bots on multiple OSs.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/14840015/51001
Retried try job too often on linux_layout for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
LGTM, one minor comment https://chromiumcodereview.appspot.com/14840015/diff/51001/Source/core/html/c... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://chromiumcodereview.appspot.com/14840015/diff/51001/Source/core/html/c... Source/core/html/canvas/WebGLRenderingContext.cpp:454: GraphicsContext3D::Attributes adjustAttributes(const GraphicsContext3D::Attributes attributes, Settings* settings) const GraphicsContext3D::Attributes& attributes
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bajones@chromium.org/14840015/66001
Message was sent while issue was closed.
Change committed as 150231 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
