Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952913002:
Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. (Closed)
|
Created:
4 years, 7 months ago by erikchen Modified:
4 years, 6 months ago Reviewers:
Ken Russell (switch to Gerrit) CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, danakj+watch_chromium.org, Rik, f(malita), blink-reviews, piman+watch_chromium.org, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@temp42_drawingbuffer_cleanup2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers.
Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers.
This CL turns on GL_RGB emulation for those GPUs.
BUG=607130
Committed: https://crrev.com/c34f049e6f49920680eb5634502b22511d7e03f9
Cr-Commit-Position: refs/heads/master@{#397490}
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : Rebase. #Messages
Total messages: 34 (13 generated)
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: Please review.
On 2016/05/05 00:47:19, erikchen wrote: > kbr: Please review. This doesn't quite work, for slightly subtle reasons. I've got to do one more round of cleanup first.
Description was changed from ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 ========== to ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 ==========
erikchen@chromium.org changed reviewers: - kbr@chromium.org
On 2016/05/05 18:36:44, erikchen wrote: > On 2016/05/05 00:47:19, erikchen wrote: > > kbr: Please review. > > This doesn't quite work, for slightly subtle reasons. I've got to do one more > round of cleanup first. OK. Sorry I didn't get to this review yet, but please ping when it's ready. Thanks. P.S. Did any of the existing tests catch the bug? If not let's add one...
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: The subtlety I was referring to is that by default, we use CHROMIUM_image for WebGL on Mac, and with my last CL, the problem is already gone. So the point of this CL would be to improve WebGL performance when CHROMIUM_image is turned off. The problem is that we don't test that case anymore, since CHROMIUM_image is default. We probably should.
On 2016/05/06 01:37:33, erikchen wrote: > kbr: The subtlety I was referring to is that by default, we use CHROMIUM_image > for WebGL on Mac, and with my last CL, the problem is already gone. Ah, I see. > So the point of this CL would be to improve WebGL performance when > CHROMIUM_image is turned off. The problem is that we don't test that case > anymore, since CHROMIUM_image is default. We probably should. Yes, agreed. Do you have a suggestion for what to test? Should we perhaps expand src/content/test/gpu/page_sets/pixel_tests.py so that it does one run of some of the tests with --disable-webgl-image-chromium? We could make sure there are alpha:false and alpha:true WebGL tests. I would be happy to expand the test suite with your guidance.
On 2016/05/06 01:58:34, Ken Russell wrote: > On 2016/05/06 01:37:33, erikchen wrote: > > kbr: The subtlety I was referring to is that by default, we use CHROMIUM_image > > for WebGL on Mac, and with my last CL, the problem is already gone. > > Ah, I see. > > > So the point of this CL would be to improve WebGL performance when > > CHROMIUM_image is turned off. The problem is that we don't test that case > > anymore, since CHROMIUM_image is default. We probably should. > > Yes, agreed. Do you have a suggestion for what to test? Should we perhaps expand > src/content/test/gpu/page_sets/pixel_tests.py so that it does one run of some of > the tests with --disable-webgl-image-chromium? We could make sure there are > alpha:false and alpha:true WebGL tests. I would be happy to expand the test > suite with your guidance. We should test all combinations of: CHROMIUM_Image: on/off alpha: true/false antialias: true/false I'm not worried about actual WebGL drawing - that gets tested pretty thoroughly by the conformance tests.This just tests the basic paths for getting pixels to the screen.
kbr: Please review. I tested this change manually, but you mentioned you were going to bring up some tests for disable-webgl-image-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/1952913002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952913002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/18 17:30:24, erikchen wrote: > kbr: Please review. > > I tested this change manually, but you mentioned you were going to bring up some > tests for disable-webgl-image-chromium? Yes, this is still on my to-do list, but I'm swamped. If I discussed with you how to add the required pixel tests would you be interested in adding them? We really should since this code is being modified.
https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1056: if (contextProvider()->getCapabilities().disable_webgl_rgb_multisampling_usage) It seems a little strange that this checks a different condition than the other code (i.e., defaultBufferRequiresAlphaChannelToBePreserved). Are you sure all of the possible legal combinations work?
Pinging about this. Are we going to move forward with this emulation? I can write the new pixel tests this week, maybe today.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
kbr: Please review. https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1056: if (contextProvider()->getCapabilities().disable_webgl_rgb_multisampling_usage) On 2016/05/19 00:06:41, Ken Russell wrote: > It seems a little strange that this checks a different condition than the other > code (i.e., defaultBufferRequiresAlphaChannelToBePreserved). Are you sure all of > the possible legal combinations work? defaultBufferRequiresAlphaChannelToBePreserved calls this method, which is expected and correct, now that I removed the fallback code.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952913002/40001
LGTM. Thanks for pushing this through. https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1952913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1056: if (contextProvider()->getCapabilities().disable_webgl_rgb_multisampling_usage) On 2016/06/02 00:14:06, erikchen wrote: > On 2016/05/19 00:06:41, Ken Russell wrote: > > It seems a little strange that this checks a different condition than the > other > > code (i.e., defaultBufferRequiresAlphaChannelToBePreserved). Are you sure all > of > > the possible legal combinations work? > > defaultBufferRequiresAlphaChannelToBePreserved calls this method, which is > expected and correct, now that I removed the fallback code. Ah, I see. Thanks for the explanation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng 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/1952913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952913002/40001
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952913002/40001
Message was sent while issue was closed.
Description was changed from ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 ========== to ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 ========== to ========== Use GL_RGBA renderbuffer for antialiased, no-alpha DrawingBuffers. Intel GPUs suffer performance penalties with GL_RGB multisampled renderbuffers. This CL turns on GL_RGB emulation for those GPUs. BUG=607130 Committed: https://crrev.com/c34f049e6f49920680eb5634502b22511d7e03f9 Cr-Commit-Position: refs/heads/master@{#397490} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c34f049e6f49920680eb5634502b22511d7e03f9 Cr-Commit-Position: refs/heads/master@{#397490} |
