Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1060583003:
WebGL backbuffer creation on Mali-400 GPU
|
Created:
5 years, 8 months ago by rahulg Modified:
5 years, 6 months ago Reviewers:
Ken Russell (switch to Gerrit) CC:
blink-reviews, krit, dshwang, Dominik Röttsches, blink-reviews-html_chromium.org, pdr+graphicswatchlist_chromium.org, Justin Novosad, jbroman, danakj, dglazkov+blink, Rik, f(malita), Stephen Chennney, aandrey+blink_chromium.org, rwlbuis, bajones Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionWebGL back-buffer creation on Mali-400 GPU
DrawingBuffer tries to allocate back-buffer as GL_RGB texture in case of requested
alpha attribute is false and fails because GL_RGB not supported on specific
GPU's (e.g Mali- 2/3/400 family).
This CL manages workaround for back-buffer and choose texture format correctly.
BUG=449150
Patch Set 1 #Patch Set 2 : Added comments #
Total comments: 8
Messages
Total messages: 13 (1 generated)
rahul.g@samsung.com changed reviewers: + kbr@chromium.org
PTAL
I'm sorry, but this does not lgtm. There are bugs in the code and I'm extremely concerned about affecting the behavior of other platforms and not just this GPU. Please see below about how to narrow down this fix to only the affected platforms. https://codereview.chromium.org/1060583003/diff/20001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1060583003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:1455: alpha = '\0'; That's a strange way to write that constant. Please use alpha = false or GL_FALSE. https://codereview.chromium.org/1060583003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2349: return (!m_requestedAttributes.alpha() && !drawingBuffer()->isRGBTextureSupported()) > 0 This is a strange way to write a boolean test. Please drop the "> 0". https://codereview.chromium.org/1060583003/diff/20001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:2351: : getIntParameter(scriptState, pname); Please also fix up the query of COLOR_WRITEMASK. https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:776: m_context->colorMask(true, true, true, false); This won't work -- the color mask applies to clear operations too, so this code will disable the needed clear of the alpha channel to 0xff. How much testing was done on this patch? There should be WebGL conformance tests which would have caught this bug. If not, we need to add them. https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is required for GL_RGB texture I'm sorry, but I'm not comfortable doing this code unconditionally. First, running this code will slow down WebGL context creation on all platforms. Second, it's clear there are bugs in this code, and it's essential that this workaround be done exactly on the platforms where it's intended and no others. Please: - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with a getter in WebKit/public/platform/Platform.h. For the time being, this can have just one virtual function named similarly to your new GPU driver bug workaround in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" - Implement the getter and this type in src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in src/gpu/blink/ (not sure where the best place for this code is -- look for similarly structured classes). Base the result on the actual GPU driver bug workarounds in use. - Perform the RGB -> RGBA back buffer emulation only when the driver bug workaround is enabled. This will require more CLs to be landed. First introduce the new type (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium side. Finally, use the new type on the Blink side to implement the workaround. https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:1016: if (fbo) Also m_context_>bindFramebuffer(GL_FRAMEBUFFER, 0); . https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... File Source/platform/graphics/gpu/DrawingBuffer.h (right): https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.h:225: void validateBackbuffer(); Please name this something more obvious like "determineRGBTextureSupport".
https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is required for GL_RGB texture On 2015/04/06 21:43:18, Ken Russell wrote: > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > running this code will slow down WebGL context creation on all platforms. > Second, it's clear there are bugs in this code, and it's essential that this > workaround be done exactly on the platforms where it's intended and no others. > > Please: > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with a > getter in WebKit/public/platform/Platform.h. For the time being, this can have > just one virtual function named similarly to your new GPU driver bug workaround > in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" > - Implement the getter and this type in > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > src/gpu/blink/ (not sure where the best place for this code is -- look for > similarly structured classes). Base the result on the actual GPU driver bug > workarounds in use. > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > workaround is enabled. > > This will require more CLs to be landed. First introduce the new type > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium side. > Finally, use the new type on the Blink side to implement the workaround. Also -- on request from another engineer on Blink, please add a UseCounter (see core/frame/UseCounter.h ) around the workaround code. We'll want to be able to remove this workaround at some point in the future based on usage data.
On 2015/04/06 21:44:55, Ken Russell wrote: > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is > required for GL_RGB texture > On 2015/04/06 21:43:18, Ken Russell wrote: > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > running this code will slow down WebGL context creation on all platforms. > > Second, it's clear there are bugs in this code, and it's essential that this > > workaround be done exactly on the platforms where it's intended and no others. > > > > Please: > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with a > > getter in WebKit/public/platform/Platform.h. For the time being, this can have > > just one virtual function named similarly to your new GPU driver bug > workaround > > in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" > > - Implement the getter and this type in > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > similarly structured classes). Base the result on the actual GPU driver bug > > workarounds in use. > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > workaround is enabled. > > > > This will require more CLs to be landed. First introduce the new type > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > side. > > Finally, use the new type on the Blink side to implement the workaround. > > Also -- on request from another engineer on Blink, please add a UseCounter (see > core/frame/UseCounter.h ) around the workaround code. We'll want to be able to > remove this workaround at some point in the future based on usage data. Thank you for your time to review. I agree with your suggestion, will update accordingly.
On 2015/04/07 12:07:29, rahulg wrote: > On 2015/04/06 21:44:55, Ken Russell wrote: > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is > > required for GL_RGB texture > > On 2015/04/06 21:43:18, Ken Russell wrote: > > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > > running this code will slow down WebGL context creation on all platforms. > > > Second, it's clear there are bugs in this code, and it's essential that this > > > workaround be done exactly on the platforms where it's intended and no > others. > > > > > > Please: > > > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with > a > > > getter in WebKit/public/platform/Platform.h. For the time being, this can > have > > > just one virtual function named similarly to your new GPU driver bug > > workaround > > > in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" > > > - Implement the getter and this type in > > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > > similarly structured classes). Base the result on the actual GPU driver bug > > > workarounds in use. > > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > > workaround is enabled. > > > > > > This will require more CLs to be landed. First introduce the new type > > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > > side. > > > Finally, use the new type on the Blink side to implement the workaround. > > > > Also -- on request from another engineer on Blink, please add a UseCounter > (see > > core/frame/UseCounter.h ) around the workaround code. We'll want to be able to > > remove this workaround at some point in the future based on usage data. > > Thank you for your time to review. > I agree with your suggestion, will update accordingly. Is there an update coming soon on this issue?
On 2015/05/07 23:59:13, halliwell wrote: > On 2015/04/07 12:07:29, rahulg wrote: > > On 2015/04/06 21:44:55, Ken Russell wrote: > > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation > is > > > required for GL_RGB texture > > > On 2015/04/06 21:43:18, Ken Russell wrote: > > > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > > > running this code will slow down WebGL context creation on all platforms. > > > > Second, it's clear there are bugs in this code, and it's essential that > this > > > > workaround be done exactly on the platforms where it's intended and no > > others. > > > > > > > > Please: > > > > > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform > with > > a > > > > getter in WebKit/public/platform/Platform.h. For the time being, this can > > have > > > > just one virtual function named similarly to your new GPU driver bug > > > workaround > > > > in https://codereview.chromium.org/996163003/ , such as > "disableGLRGBFormat" > > > > - Implement the getter and this type in > > > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > > > similarly structured classes). Base the result on the actual GPU driver > bug > > > > workarounds in use. > > > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > > > workaround is enabled. > > > > > > > > This will require more CLs to be landed. First introduce the new type > > > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > > > side. > > > > Finally, use the new type on the Blink side to implement the workaround. > > > > > > Also -- on request from another engineer on Blink, please add a UseCounter > > (see > > > core/frame/UseCounter.h ) around the workaround code. We'll want to be able > to > > > remove this workaround at some point in the future based on usage data. > > > > Thank you for your time to review. > > I agree with your suggestion, will update accordingly. > > Is there an update coming soon on this issue? Yes will update soon.
On 2015/04/06 21:44:55, Ken Russell wrote: > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is > required for GL_RGB texture > On 2015/04/06 21:43:18, Ken Russell wrote: > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > running this code will slow down WebGL context creation on all platforms. > > Second, it's clear there are bugs in this code, and it's essential that this > > workaround be done exactly on the platforms where it's intended and no others. > > > > Please: > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with a > > getter in WebKit/public/platform/Platform.h. For the time being, this can have > > just one virtual function named similarly to your new GPU driver bug > workaround > > in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" > > - Implement the getter and this type in > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > similarly structured classes). Base the result on the actual GPU driver bug > > workarounds in use. > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > workaround is enabled. > > > > This will require more CLs to be landed. First introduce the new type > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > side. @kbr: rahulg was asking me about how to get at the workaround flag from renderer_blink_platform_impl.cc. I think this info isn't automatically there in a renderer ... closest equivalent I can see is canAccelerate2dCanvas, which sets up a GpuChannelHost and looks at its GpuInfo. Which implies he'd have to add his workaround flag to GPUInfo, add it to the IPC struct traits for GPUInfo, and access in similar fashion ... does that sound right or did you have something else in mind? > > Finally, use the new type on the Blink side to implement the workaround. > > Also -- on request from another engineer on Blink, please add a UseCounter (see > core/frame/UseCounter.h ) around the workaround code. We'll want to be able to > remove this workaround at some point in the future based on usage data.
On 2015/05/12 23:52:01, halliwell wrote: > On 2015/04/06 21:44:55, Ken Russell wrote: > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation is > > required for GL_RGB texture > > On 2015/04/06 21:43:18, Ken Russell wrote: > > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > > running this code will slow down WebGL context creation on all platforms. > > > Second, it's clear there are bugs in this code, and it's essential that this > > > workaround be done exactly on the platforms where it's intended and no > others. > > > > > > Please: > > > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform with > a > > > getter in WebKit/public/platform/Platform.h. For the time being, this can > have > > > just one virtual function named similarly to your new GPU driver bug > > workaround > > > in https://codereview.chromium.org/996163003/ , such as "disableGLRGBFormat" > > > - Implement the getter and this type in > > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > > similarly structured classes). Base the result on the actual GPU driver bug > > > workarounds in use. > > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > > workaround is enabled. > > > > > > This will require more CLs to be landed. First introduce the new type > > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > > side. > > @kbr: > rahulg was asking me about how to get at the workaround flag from > renderer_blink_platform_impl.cc. I think this info isn't automatically there in > a renderer ... closest equivalent I can see is canAccelerate2dCanvas, which sets > up a GpuChannelHost and looks at its GpuInfo. Which implies he'd have to add > his workaround flag to GPUInfo, add it to the IPC struct traits for GPUInfo, and > access in similar fashion ... does that sound right or did you have something > else in mind? It should be defined as one of the WebPreferences and propagate through GpuDataManagerImplPrivate::UpdateRendererWebPrefs(). > > > > Finally, use the new type on the Blink side to implement the workaround. > > > > Also -- on request from another engineer on Blink, please add a UseCounter > (see > > core/frame/UseCounter.h ) around the workaround code. We'll want to be able to > > remove this workaround at some point in the future based on usage data.
On 2015/05/13 00:00:20, Zhenyao Mo wrote: > On 2015/05/12 23:52:01, halliwell wrote: > > On 2015/04/06 21:44:55, Ken Russell wrote: > > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > > > > > https://codereview.chromium.org/1060583003/diff/20001/Source/platform/graphic... > > > Source/platform/graphics/gpu/DrawingBuffer.cpp:1005: // One time validation > is > > > required for GL_RGB texture > > > On 2015/04/06 21:43:18, Ken Russell wrote: > > > > I'm sorry, but I'm not comfortable doing this code unconditionally. First, > > > > running this code will slow down WebGL context creation on all platforms. > > > > Second, it's clear there are bugs in this code, and it's essential that > this > > > > workaround be done exactly on the platforms where it's intended and no > > others. > > > > > > > > Please: > > > > > > > > - Introduce a new type like WebGpuBlacklist in WebKit/public/platform > with > > a > > > > getter in WebKit/public/platform/Platform.h. For the time being, this can > > have > > > > just one virtual function named similarly to your new GPU driver bug > > > workaround > > > > in https://codereview.chromium.org/996163003/ , such as > "disableGLRGBFormat" > > > > - Implement the getter and this type in > > > > src/content/renderer/renderer_blink_platform_impl.{cc,h} and possibly in > > > > src/gpu/blink/ (not sure where the best place for this code is -- look for > > > > similarly structured classes). Base the result on the actual GPU driver > bug > > > > workarounds in use. > > > > - Perform the RGB -> RGBA back buffer emulation only when the driver bug > > > > workaround is enabled. > > > > > > > > This will require more CLs to be landed. First introduce the new type > > > > (WebGpuBlacklist) on the Blink side. Then plumb it through to the Chromium > > > side. > > > > @kbr: > > rahulg was asking me about how to get at the workaround flag from > > renderer_blink_platform_impl.cc. I think this info isn't automatically there > in > > a renderer ... closest equivalent I can see is canAccelerate2dCanvas, which > sets > > up a GpuChannelHost and looks at its GpuInfo. Which implies he'd have to add > > his workaround flag to GPUInfo, add it to the IPC struct traits for GPUInfo, > and > > access in similar fashion ... does that sound right or did you have something > > else in mind? > > It should be defined as one of the WebPreferences and propagate through > GpuDataManagerImplPrivate::UpdateRendererWebPrefs(). > Thank you for suggestion. > > > > > > Finally, use the new type on the Blink side to implement the workaround. > > > > > > Also -- on request from another engineer on Blink, please add a UseCounter > > (see > > > core/frame/UseCounter.h ) around the workaround code. We'll want to be able > to > > > remove this workaround at some point in the future based on usage data.
> > It should be defined as one of the WebPreferences and propagate through > > GpuDataManagerImplPrivate::UpdateRendererWebPrefs(). > > > Thank you for suggestion. > @zmo Need your suggestion for (Accessing WebPreference in render_blink_platform_impl.cc) As per your suggestion I have defined gl_rgb_support FLAG as WebPreferences and updated on this method [GpuDataManagerImplPrivate::UpdateRendererWebPrefs()] please check => https://codereview.chromium.org/1154463005/ could you also suggest how to access "gl_rgb_support WebPreference" in render_blink_platform_impl.cc I need to expose WebPreference (gl_rgb_support) to the Blink side Please check https://codereview.chromium.org/1153713003/
On 2015/05/28 07:27:58, rahulg wrote: > > > It should be defined as one of the WebPreferences and propagate through > > > GpuDataManagerImplPrivate::UpdateRendererWebPrefs(). > > > > > Thank you for suggestion. > > > > @zmo > Need your suggestion for (Accessing WebPreference in > render_blink_platform_impl.cc) > > As per your suggestion > I have defined gl_rgb_support FLAG as WebPreferences and updated on this method > [GpuDataManagerImplPrivate::UpdateRendererWebPrefs()] > please check => https://codereview.chromium.org/1154463005/ > > could you also suggest how to access "gl_rgb_support WebPreference" in > render_blink_platform_impl.cc > I need to expose WebPreference (gl_rgb_support) to the Blink side > Please check https://codereview.chromium.org/1153713003/ Sorry this thread dropped out of my radar. You need to wire the WebPreferences all the way to the blink side, then you can access it there. Exposing blacklist is definitely not the way to do this. Please take a look at WebSettings. You can follow how the accelerated_2d_canvas_enabled is wired: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
On 2015/06/05 16:29:12, Zhenyao Mo wrote: > On 2015/05/28 07:27:58, rahulg wrote: > > > > It should be defined as one of the WebPreferences and propagate through > > > > GpuDataManagerImplPrivate::UpdateRendererWebPrefs(). > > > > > > > Thank you for suggestion. > > > > > > > @zmo > > Need your suggestion for (Accessing WebPreference in > > render_blink_platform_impl.cc) > > > > As per your suggestion > > I have defined gl_rgb_support FLAG as WebPreferences and updated on this > method > > [GpuDataManagerImplPrivate::UpdateRendererWebPrefs()] > > please check => https://codereview.chromium.org/1154463005/ > > > > could you also suggest how to access "gl_rgb_support WebPreference" in > > render_blink_platform_impl.cc > > I need to expose WebPreference (gl_rgb_support) to the Blink side > > Please check https://codereview.chromium.org/1153713003/ > > Sorry this thread dropped out of my radar. > > You need to wire the WebPreferences all the way to the blink side, then you can > access it there. > > Exposing blacklist is definitely not the way to do this. > > Please take a look at WebSettings. You can follow how the > accelerated_2d_canvas_enabled is wired: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... Thank you |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
