Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2535173002:
Prevent implicit framebuffer clear from clobbering alpha in emulated RGB (Closed)
|
Created:
4 years ago by Kai Ninomiya Modified:
4 years ago CC:
chromium-reviews, f(malita), krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jam, jbroman, Justin Novosad, Rik, darin-cc_chromium.org, pdr+graphicswatchlist_chromium.org, blink-reviews, piman+watch_chromium.org, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis, ccameron, erikchen Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent implicit framebuffer clear from clobbering alpha in emulated RGB
In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is
backed by an RGBA IOSurface), the implicit framebuffer clear (due to
preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the
RGBA IOSurface (which should always be 1.0).
BUG=666259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Committed: https://crrev.com/4259dbb0600a71bcff06f342294fe1546bec9f08
Cr-Commit-Position: refs/heads/master@{#435850}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix #
Total comments: 16
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : remove unnecessary std::list #
Messages
Total messages: 38 (22 generated)
Description was changed from ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). **** This patch is NOT ready for review yet - just running it through a dry run. **** BUG=666259 ========== to ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). **** This patch is NOT ready for review yet - just running it through a dry run. **** BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). **** This patch is NOT ready for review yet - just running it through a dry run. **** BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). **** This patch is NOT ready for review yet - just running it through a dry run. **** BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
kainino@chromium.org changed reviewers: + kbr@chromium.org
Description was changed from ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). **** This patch is NOT ready for review yet - just running it through a dry run. **** BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kbr, PTAL
kbr@chromium.org changed reviewers: + haraken@chromium.org, sigbjornf@opera.se
Excellent work Kai, especially producing the thorough test. A couple of minor questions and concerns. haraken@, sigbjornf@: could you please respond to the question in WebGLRenderingContextBase.h? https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:45: self.Fail('Pixel_WebGLGreenTriangle_NonChromiumImage_NoAA_NoAlpha', bug=666259) The name in this suppression is wrong -- not clear to me why the mac_chromium_rel_ng tryjob passed. It should have failed. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_test_pages.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_test_pages.py:299: browser_args=non_chromium_image_args), Please add the new page to the set of DefaultPages, above, instead of the MacSpecificPages. (Running with non_chromium_image_args defeats the purpose of this test, btw.) There's a chance this test might catch a regression on another platform before it happens. I suggest a name like WebGLTransparentGreenTriangle_NoAlpha_ImplicitClear. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:212: expectations=expectations)) Not necessary to add this here; this obsolete page list is only used by trace_test.py, paradoxically. Sorry for the confusion. (See http://crbug.com/352807 for the tracking bug for porting all the tests to the new harness.) Please undo this change. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:120: WeakPersistent<WebGLRenderingContextBase> m_context; I'm concerned about the performance impact of allocating a WeakPersistent during every draw call. haraken@, sigbjornf@, could you comment on that? These data structures are only stack-allocated. Is there some cheaper way to be safe with regard to Oilpan but still maintain temporary pointers to the WebGLRenderingContextBase? With Oilpan's clang plugin, we found this necessary to compile. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:595: std::list<ScopedRGBEmulationColorMask*> activeScopedRGBEmulationColorMasks; Please put all of this in the private section and make ScopedRGBEmulationColorMask a friend. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:991: m_gl->ColorMask(true, true, true, false); While I'm pretty sure this was just an omission, this code path isn't tested on any of our waterfall machines. Could you do at least a manual test where you modify your test to use the AA path and run the browser with the command line argument --disable_multisampling_color_mask_usage ? (GPU driver bug workarounds can be specified on the command line.) Let's make sure this code path is taken in that case, and that it still renders correctly. (We should also manually test PlayCanvas's test case with this command line argument.)
https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:45: self.Fail('Pixel_WebGLGreenTriangle_NonChromiumImage_NoAA_NoAlpha', bug=666259) On 2016/12/01 07:20:43, Ken Russell wrote: > The name in this suppression is wrong -- not clear to me why the > mac_chromium_rel_ng tryjob passed. It should have failed. *Facepalm* When I ported the pixel_tests to the new harness a couple of *months* ago I accidentally dropped the Mac-specific pages. I'm so sorry. Fixing in https://codereview.chromium.org/2547463002 . Please do run your new test on all platforms though.
https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:120: WeakPersistent<WebGLRenderingContextBase> m_context; On 2016/12/01 07:20:43, Ken Russell wrote: > I'm concerned about the performance impact of allocating a WeakPersistent during > every draw call. haraken@, sigbjornf@, could you comment on that? These data > structures are only stack-allocated. Is there some cheaper way to be safe with > regard to Oilpan but still maintain temporary pointers to the > WebGLRenderingContextBase? With Oilpan's clang plugin, we found this necessary > to compile. I just learned from looking at another CL ( https://codereview.chromium.org/2476693002/ ) that the way to handle this is to declare this STACK_ALLOCATED(), and use a Member. Kai, could you make that change? Kentaro, Sigbjorn, no longer necessary to look at this.
On 2016/12/01 08:36:47, Ken Russell wrote: > https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > (right): > > https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:120: > WeakPersistent<WebGLRenderingContextBase> m_context; > On 2016/12/01 07:20:43, Ken Russell wrote: > > I'm concerned about the performance impact of allocating a WeakPersistent > during > > every draw call. haraken@, sigbjornf@, could you comment on that? These data > > structures are only stack-allocated. Is there some cheaper way to be safe with > > regard to Oilpan but still maintain temporary pointers to the > > WebGLRenderingContextBase? With Oilpan's clang plugin, we found this necessary > > to compile. > > I just learned from looking at another CL ( > https://codereview.chromium.org/2476693002/ ) that the way to handle this is to > declare this STACK_ALLOCATED(), and use a Member. Kai, could you make that > change? > > Kentaro, Sigbjorn, no longer necessary to look at this. Sorry about the late reply. Yes, using STACK_ALLOCATED & Members is a solution.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:991: m_gl->ColorMask(true, true, true, false); On 2016/12/01 07:20:43, Ken Russell wrote: > While I'm pretty sure this was just an omission, this code path isn't tested on > any of our waterfall machines. Could you do at least a manual test where you > modify your test to use the AA path and run the browser with the command line > argument --disable_multisampling_color_mask_usage ? (GPU driver bug workarounds > can be specified on the command line.) Let's make sure this code path is taken > in that case, and that it still renders correctly. (We should also manually test > PlayCanvas's test case with this command line argument.) This was a relatively recent regression from: https://chromiumcodereview.appspot.com/2402273002/diff/140001/third_party/Web... Can we use a nested instance of m_stateRestorer [not sure if it supports nesting off the top of my head]
ccameron@chromium.org changed reviewers: + ccameron@chromium.org
https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:991: m_gl->ColorMask(true, true, true, false); On 2016/12/01 16:04:11, erikchen wrote: > On 2016/12/01 07:20:43, Ken Russell wrote: > > While I'm pretty sure this was just an omission, this code path isn't tested > on > > any of our waterfall machines. Could you do at least a manual test where you > > modify your test to use the AA path and run the browser with the command line > > argument --disable_multisampling_color_mask_usage ? (GPU driver bug > workarounds > > can be specified on the command line.) Let's make sure this code path is taken > > in that case, and that it still renders correctly. (We should also manually > test > > PlayCanvas's test case with this command line argument.) > > This was a relatively recent regression from: > https://chromiumcodereview.appspot.com/2402273002/diff/140001/third_party/Web... > > Can we use a nested instance of m_stateRestorer [not sure if it supports nesting > off the top of my head] Gah, sorry!!! Shouldn't we be doing m_client->DrawingBufferClientRestoreMaskAndClearValues(); to restore the values that the client expects (that's what it was before my patch chopped out the Clear by accident).
https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:45: self.Fail('Pixel_WebGLGreenTriangle_NonChromiumImage_NoAA_NoAlpha', bug=666259) On 2016/12/01 08:00:54, Ken Russell wrote: > On 2016/12/01 07:20:43, Ken Russell wrote: > > The name in this suppression is wrong -- not clear to me why the > > mac_chromium_rel_ng tryjob passed. It should have failed. > > *Facepalm* > > When I ported the pixel_tests to the new harness a couple of *months* ago I > accidentally dropped the Mac-specific pages. I'm so sorry. Fixing in > https://codereview.chromium.org/2547463002 . Please do run your new test on all > platforms though. Done. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_test_pages.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_test_pages.py:299: browser_args=non_chromium_image_args), On 2016/12/01 07:20:43, Ken Russell wrote: > Please add the new page to the set of DefaultPages, above, instead of the > MacSpecificPages. (Running with non_chromium_image_args defeats the purpose of > this test, btw.) There's a chance this test might catch a regression on another > platform before it happens. I suggest a name like > WebGLTransparentGreenTriangle_NoAlpha_ImplicitClear. Done. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:212: expectations=expectations)) On 2016/12/01 07:20:43, Ken Russell wrote: > Not necessary to add this here; this obsolete page list is only used by > trace_test.py, paradoxically. Sorry for the confusion. (See > http://crbug.com/352807 for the tracking bug for porting all the tests to the > new harness.) Please undo this change. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:120: WeakPersistent<WebGLRenderingContextBase> m_context; On 2016/12/01 08:36:47, Ken Russell wrote: > On 2016/12/01 07:20:43, Ken Russell wrote: > > I'm concerned about the performance impact of allocating a WeakPersistent > during > > every draw call. haraken@, sigbjornf@, could you comment on that? These data > > structures are only stack-allocated. Is there some cheaper way to be safe with > > regard to Oilpan but still maintain temporary pointers to the > > WebGLRenderingContextBase? With Oilpan's clang plugin, we found this necessary > > to compile. > > I just learned from looking at another CL ( > https://codereview.chromium.org/2476693002/ ) that the way to handle this is to > declare this STACK_ALLOCATED(), and use a Member. Kai, could you make that > change? > > Kentaro, Sigbjorn, no longer necessary to look at this. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:595: std::list<ScopedRGBEmulationColorMask*> activeScopedRGBEmulationColorMasks; On 2016/12/01 07:20:43, Ken Russell wrote: > Please put all of this in the private section and make > ScopedRGBEmulationColorMask a friend. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:991: m_gl->ColorMask(true, true, true, false); On 2016/12/01 19:25:59, ccameron wrote: > On 2016/12/01 16:04:11, erikchen wrote: > > On 2016/12/01 07:20:43, Ken Russell wrote: > > > While I'm pretty sure this was just an omission, this code path isn't tested > > on > > > any of our waterfall machines. Could you do at least a manual test where you > > > modify your test to use the AA path and run the browser with the command > line > > > argument --disable_multisampling_color_mask_usage ? (GPU driver bug > > workarounds > > > can be specified on the command line.) Let's make sure this code path is > taken > > > in that case, and that it still renders correctly. (We should also manually > > test > > > PlayCanvas's test case with this command line argument.) > > > > This was a relatively recent regression from: > > > https://chromiumcodereview.appspot.com/2402273002/diff/140001/third_party/Web... > > > > Can we use a nested instance of m_stateRestorer [not sure if it supports > nesting > > off the top of my head] > > Gah, sorry!!! > > Shouldn't we be doing > m_client->DrawingBufferClientRestoreMaskAndClearValues(); > to restore the values that the client expects (that's what it was before my > patch chopped out the Clear by accident). I see, it looks like the `m_stateRestorer->setClearStateDirty();` added in your patch ought to take care of this. In that case, I should only need to add in the Clear, right?
https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_expectations.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_expectations.py:45: self.Fail('Pixel_WebGLGreenTriangle_NonChromiumImage_NoAA_NoAlpha', bug=666259) On 2016/12/01 08:00:54, Ken Russell wrote: > On 2016/12/01 07:20:43, Ken Russell wrote: > > The name in this suppression is wrong -- not clear to me why the > > mac_chromium_rel_ng tryjob passed. It should have failed. > > *Facepalm* > > When I ported the pixel_tests to the new harness a couple of *months* ago I > accidentally dropped the Mac-specific pages. I'm so sorry. Fixing in > https://codereview.chromium.org/2547463002 . Please do run your new test on all > platforms though. Done. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/pixel_test_pages.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/pixel_test_pages.py:299: browser_args=non_chromium_image_args), On 2016/12/01 07:20:43, Ken Russell wrote: > Please add the new page to the set of DefaultPages, above, instead of the > MacSpecificPages. (Running with non_chromium_image_args defeats the purpose of > this test, btw.) There's a chance this test might catch a regression on another > platform before it happens. I suggest a name like > WebGLTransparentGreenTriangle_NoAlpha_ImplicitClear. Done. https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... File content/test/gpu/page_sets/pixel_tests.py (right): https://codereview.chromium.org/2535173002/diff/40001/content/test/gpu/page_s... content/test/gpu/page_sets/pixel_tests.py:212: expectations=expectations)) On 2016/12/01 07:20:43, Ken Russell wrote: > Not necessary to add this here; this obsolete page list is only used by > trace_test.py, paradoxically. Sorry for the confusion. (See > http://crbug.com/352807 for the tracking bug for porting all the tests to the > new harness.) Please undo this change. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:120: WeakPersistent<WebGLRenderingContextBase> m_context; On 2016/12/01 08:36:47, Ken Russell wrote: > On 2016/12/01 07:20:43, Ken Russell wrote: > > I'm concerned about the performance impact of allocating a WeakPersistent > during > > every draw call. haraken@, sigbjornf@, could you comment on that? These data > > structures are only stack-allocated. Is there some cheaper way to be safe with > > regard to Oilpan but still maintain temporary pointers to the > > WebGLRenderingContextBase? With Oilpan's clang plugin, we found this necessary > > to compile. > > I just learned from looking at another CL ( > https://codereview.chromium.org/2476693002/ ) that the way to handle this is to > declare this STACK_ALLOCATED(), and use a Member. Kai, could you make that > change? > > Kentaro, Sigbjorn, no longer necessary to look at this. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:595: std::list<ScopedRGBEmulationColorMask*> activeScopedRGBEmulationColorMasks; On 2016/12/01 07:20:43, Ken Russell wrote: > Please put all of this in the private section and make > ScopedRGBEmulationColorMask a friend. Done. https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2535173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:991: m_gl->ColorMask(true, true, true, false); On 2016/12/01 19:25:59, ccameron wrote: > On 2016/12/01 16:04:11, erikchen wrote: > > On 2016/12/01 07:20:43, Ken Russell wrote: > > > While I'm pretty sure this was just an omission, this code path isn't tested > > on > > > any of our waterfall machines. Could you do at least a manual test where you > > > modify your test to use the AA path and run the browser with the command > line > > > argument --disable_multisampling_color_mask_usage ? (GPU driver bug > > workarounds > > > can be specified on the command line.) Let's make sure this code path is > taken > > > in that case, and that it still renders correctly. (We should also manually > > test > > > PlayCanvas's test case with this command line argument.) > > > > This was a relatively recent regression from: > > > https://chromiumcodereview.appspot.com/2402273002/diff/140001/third_party/Web... > > > > Can we use a nested instance of m_stateRestorer [not sure if it supports > nesting > > off the top of my head] > > Gah, sorry!!! > > Shouldn't we be doing > m_client->DrawingBufferClientRestoreMaskAndClearValues(); > to restore the values that the client expects (that's what it was before my > patch chopped out the Clear by accident). I see, it looks like the `m_stateRestorer->setClearStateDirty();` added in your patch ought to take care of this. In that case, I should only need to add in the Clear, right?
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Excellent work again Kai - thanks for carrying this through and your thorough testing. One more comment. https://codereview.chromium.org/2535173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1543: ScopedRGBEmulationColorMask* popActiveScopedRGBEmulationColorMask() { It occurs to me that since the WebGLRenderingContextBase code only checks activeScopedRGBEmulationColorMasks.empty(), we could just use a simple int to track the active count instead of a std::list. I'd ask that this change be made for simplicity.
The CQ bit was unchecked by kainino@chromium.org
https://codereview.chromium.org/2535173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/2535173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1543: ScopedRGBEmulationColorMask* popActiveScopedRGBEmulationColorMask() { On 2016/12/01 23:45:30, Ken Russell wrote: > It occurs to me that since the WebGLRenderingContextBase code only checks > activeScopedRGBEmulationColorMasks.empty(), we could just use a simple int to > track the active count instead of a std::list. I'd ask that this change be made > for simplicity. Great point, thanks. I was thinking of trying to simplify it from a std::list but I forgot to figure out if I could. Done.
Great. LGTM
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kainino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480650944162760,
"parent_rev": "1b4454ab85eb45f38b558aa65fd158a61b77bf02", "commit_rev":
"e27af6cbc3f66a40f9d8d6e62fd787387370f2f3"}
Message was sent while issue was closed.
Description was changed from ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Prevent implicit framebuffer clear from clobbering alpha in emulated RGB In the case of emulated RGB framebuffers (for alpha:false on Mac, where it is backed by an RGBA IOSurface), the implicit framebuffer clear (due to preserveDrawingBuffer:false) could sometimes clobber the alpha channel of the RGBA IOSurface (which should always be 1.0). BUG=666259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Committed: https://crrev.com/4259dbb0600a71bcff06f342294fe1546bec9f08 Cr-Commit-Position: refs/heads/master@{#435850} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4259dbb0600a71bcff06f342294fe1546bec9f08 Cr-Commit-Position: refs/heads/master@{#435850} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
