Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1856933002:
WebGL GL_RGB emulation to support IOSurfaces on Mac. (Closed)
|
Created:
4 years, 8 months ago by erikchen Modified:
4 years, 7 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, danakj+watch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, haraken, Rik, f(malita), blink-reviews, piman+watch_chromium.org, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebGL GL_RGB emulation to support IOSurface back buffers on Mac.
This CL creates a client side implementation of GL_RGB emulation of the WebGL
back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work
correctly. The emulation is composed of several simple steps:
1. The service side is told that the back buffer has format GL_RGB, but it
will create a GL_RGBA texture as per the chromium_image_rgb_emulation
capability.
2. When the back buffer is first created, its alpha channel is cleared ot 1.
3. Any time the back buffer would be written to, the alpha channel of the
color mask is set to 0.
4. Queries about the alpha channel of the back buffer are modified to return
the correct result.
Explicit multisample renderbuffer resolve takes a different path.
1. The back buffer is still created with emulated format GL_RGB.
2. The renderbuffer is given format GL_RGB.
3. During resolution, the renderbuffer is blitted to an intermediate
non-multisample renderbuffer with format GL_RGBA.
4. The intermediate renderbuffer is then blitted to the destination texture.
The multisample path uses an extra blit. Theoretically, it should be possible to
only use a single blit by using glColorMask() just like the non-multisample
case. Driver bugs in ATI gpus make this solution not universally applicable.
BUG=595948
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/7e3811e8e71885af851e4b5a444a2715756de96e
Cr-Commit-Position: refs/heads/master@{#391428}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Add error checking. #Patch Set 5 : Full client side implementation of GL_RGB emulation. #
Total comments: 1
Patch Set 6 : Copy of patchset 1. Mostly client side implementation of RGB emulation. #Patch Set 7 : Remove more format checking that should be service side. #Patch Set 8 : Remove whitespace. #Patch Set 9 : Rebase. #Patch Set 10 : Rebase and use RGB render buffer. #Patch Set 11 : Fix GN. #Patch Set 12 : Intermediate Renderbuffer. #Patch Set 13 : Fix test. #Patch Set 14 : Clean up. #
Total comments: 18
Patch Set 15 : Comments from zmo and kbr. #Patch Set 16 : Removing logging. #Patch Set 17 : Initialize to 0. #
Total comments: 2
Patch Set 18 : Comments from kbr. #Messages
Total messages: 60 (29 generated)
Description was changed from ========== WebGL GL_RGB emulation to support IOSurfaces on Mac. BUG= ========== to ========== WebGL GL_RGB emulation to support IOSurfaces on Mac. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/20001
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 erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/40001
Description was changed from ========== WebGL GL_RGB emulation to support IOSurfaces on Mac. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGBA. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. 5. When copying to a texture from the back buffer, an additional format check is performed to ensure that the destination texture does not require an alpha channel. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/60001
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/80001
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: Please review. The code that's not specific to the CHROMIUMImage extension gets tested heavily by the WebGL conformance tests. Locally, the conformance tests also look fine when I pass in "--enable-webgl-image-chromium". I'd like to turn that on [by default] in a follow up CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for putting this together. The earlier patch sets looked OK but the current direction doesn't look good. Let's fix it as you suggested by teaching the service side about the emulation of the RGB back buffer, and rejecting CopyTexSubImage2D calls appropriately. OK? https://codereview.chromium.org/1856933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1856933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4009: GLenum error = contextGL()->GetError(); Sorry, but this synchronous GetError call will kill texture upload performance. A different solution is needed, as mentioned in https://bugs.chromium.org/p/chromium/issues/detail?id=595948#c18 .
Description was changed from ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGBA. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. 5. When copying to a texture from the back buffer, an additional format check is performed to ensure that the destination texture does not require an alpha channel. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGBA. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/240001
Description was changed from ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGBA. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGB, but it will create a GL_RGBA texture as per the chromium_image_rgb_emulation capability. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. Explicit multisample renderbuffer resolve takes a different path. 1. The back buffer is still created with emulated format GL_RGB. 2. The renderbuffer is given format GL_RGB. 3. During resolution, the renderbuffer is blitted to an intermediate non-multisample renderbuffer with format GL_RGBA. 4. The intermediate renderbuffer is then blitted to the destination texture. The multisample path uses an extra blit. Theoretically, it should be possible to only use a single blit by using glColorMask() just like the non-multisample case. Driver bugs in ATI gpus make this solution not universally applicable. If the extra blit ends up becoming a performance problem, we can implement the efficient solution on most gpus. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/260001
Description was changed from ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGB, but it will create a GL_RGBA texture as per the chromium_image_rgb_emulation capability. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. Explicit multisample renderbuffer resolve takes a different path. 1. The back buffer is still created with emulated format GL_RGB. 2. The renderbuffer is given format GL_RGB. 3. During resolution, the renderbuffer is blitted to an intermediate non-multisample renderbuffer with format GL_RGBA. 4. The intermediate renderbuffer is then blitted to the destination texture. The multisample path uses an extra blit. Theoretically, it should be possible to only use a single blit by using glColorMask() just like the non-multisample case. Driver bugs in ATI gpus make this solution not universally applicable. If the extra blit ends up becoming a performance problem, we can implement the efficient solution on most gpus. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGB, but it will create a GL_RGBA texture as per the chromium_image_rgb_emulation capability. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. Explicit multisample renderbuffer resolve takes a different path. 1. The back buffer is still created with emulated format GL_RGB. 2. The renderbuffer is given format GL_RGB. 3. During resolution, the renderbuffer is blitted to an intermediate non-multisample renderbuffer with format GL_RGBA. 4. The intermediate renderbuffer is then blitted to the destination texture. The multisample path uses an extra blit. Theoretically, it should be possible to only use a single blit by using glColorMask() just like the non-multisample case. Driver bugs in ATI gpus make this solution not universally applicable. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kbr: Please review.
kbr@chromium.org changed reviewers: + zmo@chromium.org
Thanks for revising this. It looks pretty good overall. Apologies, at a conference this week and can't do an in-depth review until next week. CC'ing zmo@ in case he can help review. A few questions. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1674: std::unique_ptr<ScopedRGBEmulationColorMask> scopedColorMask; I'd suggest changing ScopedRGBEmulationColorMask's constructor to take contextGL, the color mask, and the drawing buffer, and internalize whether it does its work. That way this block can just be ScopedRGBEmulationColorMask scopedColorMask(contextGL(), m_colorMask, m_drawingBuffer); throughout the CL. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:374: // Unused. Unused when CHROMIUM_image is being used, right? Does it need to be set if falling back to use a regular texture? https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:802: m_gl->ColorMask(true, true, true, true); Were callers relying on this to set the color mask to (true, true, true, true)? If so then maybe that should be made more explicit in the callers. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:831: m_gl->BindRenderbuffer(GL_RENDERBUFFER, m_multisampleIntermediateRenderbuffer); Is resizing of m_multisampleIntermediateRenderbuffer handled properly during resizes of the DrawingBuffer? It looks like this might have been missed in DrawingBuffer::reset(). If it's missed, let's add a WebGL conformance test to catch the error. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1011: m_gl->ColorMask(false, false, false, true); Is it deliberate that only the alpha channel is being cleared (to 1.0) here? If so I'd suggest making this more explicit in either the name of this method or its documentation in the header. (I'm guessing this is the only place in the code which is allowed to touch the alpha channel for the emulated RGB back buffers, and the clearIfComposited code path takes care of clearing the RGB channels.)
Besides kbr's comments, which I agree, I have a couple more. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:127: class ScopedRGBEmulationColorMask { Does this bug also exist in core profile? If yes, then you need to expose it to .h file, and use it in WebGL2RenderingContextBase also (a few draw calls there). https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:829: DCHECK(!m_multisampleIntermediateRenderbuffer); These two names are misleading. The intermediate render buffer isn't multi-sampled, so maybe just name it m_intermediateRenderbuffer, m_intermediateFBO? https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1003: if (m_wantAlphaChannel) You can also return early if we don't need to emulate, then the texture channels will just be cleared lazily. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1015: restoreFramebufferBindings(); Should we also restore clear color and clear mask at the end?
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
kbr, zmo: PTAL https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:127: class ScopedRGBEmulationColorMask { On 2016/04/29 22:54:15, Zhenyao Mo wrote: > Does this bug also exist in core profile? If yes, then you need to expose it to > .h file, and use it in WebGL2RenderingContextBase also (a few draw calls there). It does. I've done as you suggested. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1674: std::unique_ptr<ScopedRGBEmulationColorMask> scopedColorMask; On 2016/04/29 09:33:09, Ken Russell wrote: > I'd suggest changing ScopedRGBEmulationColorMask's constructor to take > contextGL, the color mask, and the drawing buffer, and internalize whether it > does its work. That way this block can just be > > ScopedRGBEmulationColorMask scopedColorMask(contextGL(), m_colorMask, > m_drawingBuffer); > > throughout the CL. Done. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:374: // Unused. On 2016/04/29 09:33:09, Ken Russell wrote: > Unused when CHROMIUM_image is being used, right? Does it need to be set if > falling back to use a regular texture? Correct. I updated the comment. Fallback textures use DrawingBuffer::defaultTextureParameters. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:802: m_gl->ColorMask(true, true, true, true); On 2016/04/29 09:33:09, Ken Russell wrote: > Were callers relying on this to set the color mask to (true, true, true, true)? > If so then maybe that should be made more explicit in the callers. No. I removed this call. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:829: DCHECK(!m_multisampleIntermediateRenderbuffer); On 2016/04/29 22:54:16, Zhenyao Mo wrote: > These two names are misleading. The intermediate render buffer isn't > multi-sampled, so maybe just name it m_intermediateRenderbuffer, > m_intermediateFBO? Done. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:831: m_gl->BindRenderbuffer(GL_RENDERBUFFER, m_multisampleIntermediateRenderbuffer); On 2016/04/29 09:33:09, Ken Russell wrote: > Is resizing of m_multisampleIntermediateRenderbuffer handled properly during > resizes of the DrawingBuffer? It looks like this might have been missed in > DrawingBuffer::reset(). If it's missed, let's add a WebGL conformance test to > catch the error. No, it wasn't. I added a test: https://codereview.appspot.com/294490043/ https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1003: if (m_wantAlphaChannel) On 2016/04/29 22:54:15, Zhenyao Mo wrote: > You can also return early if we don't need to emulate, then the texture channels > will just be cleared lazily. Done. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1011: m_gl->ColorMask(false, false, false, true); On 2016/04/29 09:33:09, Ken Russell wrote: > Is it deliberate that only the alpha channel is being cleared (to 1.0) here? If > so I'd suggest making this more explicit in either the name of this method or > its documentation in the header. (I'm guessing this is the only place in the > code which is allowed to touch the alpha channel for the emulated RGB back > buffers, and the clearIfComposited code path takes care of clearing the RGB > channels.) Yes. I updated the name of this method to be clearChromiumImageAlpha, and also updated the documentation. https://codereview.chromium.org/1856933002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1015: restoreFramebufferBindings(); On 2016/04/29 22:54:16, Zhenyao Mo wrote: > Should we also restore clear color and clear mask at the end? Yes. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/320001
Very nice. Thanks for pushing this forward. Does your new test catch the bug that was in the earlier patch set? If so, then LGTM. We can land the test into the WebGL conformance suite and roll that dependency separately. https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:141: // This class uses the color mask to prevents drawing to the alpha channel, if typo: prevents -> prevent
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/03 23:32:27, Ken Russell wrote: > Very nice. Thanks for pushing this forward. > > Does your new test catch the bug that was in the earlier patch set? If so, then > LGTM. We can land the test into the WebGL conformance suite and roll that > dependency separately. Yes it does. > > https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h > (right): > > https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:141: // This > class uses the color mask to prevents drawing to the alpha channel, if > typo: prevents -> prevent
https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1856933002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:141: // This class uses the color mask to prevents drawing to the alpha channel, if On 2016/05/03 23:32:27, Ken Russell wrote: > typo: prevents -> prevent Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1856933002/#ps340001 (title: "Comments from kbr.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856933002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856933002/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGB, but it will create a GL_RGBA texture as per the chromium_image_rgb_emulation capability. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. Explicit multisample renderbuffer resolve takes a different path. 1. The back buffer is still created with emulated format GL_RGB. 2. The renderbuffer is given format GL_RGB. 3. During resolution, the renderbuffer is blitted to an intermediate non-multisample renderbuffer with format GL_RGBA. 4. The intermediate renderbuffer is then blitted to the destination texture. The multisample path uses an extra blit. Theoretically, it should be possible to only use a single blit by using glColorMask() just like the non-multisample case. Driver bugs in ATI gpus make this solution not universally applicable. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== WebGL GL_RGB emulation to support IOSurface back buffers on Mac. This CL creates a client side implementation of GL_RGB emulation of the WebGL back buffer. The emulation is necessary because GL_RGB IOSurfaces don't work correctly. The emulation is composed of several simple steps: 1. The service side is told that the back buffer has format GL_RGB, but it will create a GL_RGBA texture as per the chromium_image_rgb_emulation capability. 2. When the back buffer is first created, its alpha channel is cleared ot 1. 3. Any time the back buffer would be written to, the alpha channel of the color mask is set to 0. 4. Queries about the alpha channel of the back buffer are modified to return the correct result. Explicit multisample renderbuffer resolve takes a different path. 1. The back buffer is still created with emulated format GL_RGB. 2. The renderbuffer is given format GL_RGB. 3. During resolution, the renderbuffer is blitted to an intermediate non-multisample renderbuffer with format GL_RGBA. 4. The intermediate renderbuffer is then blitted to the destination texture. The multisample path uses an extra blit. Theoretically, it should be possible to only use a single blit by using glColorMask() just like the non-multisample case. Driver bugs in ATI gpus make this solution not universally applicable. BUG=595948 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/7e3811e8e71885af851e4b5a444a2715756de96e Cr-Commit-Position: refs/heads/master@{#391428} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/7e3811e8e71885af851e4b5a444a2715756de96e Cr-Commit-Position: refs/heads/master@{#391428} |
