Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1301923002:
Add GL_CHROMIUM_screen_space_antialiasing to support alternative AA (Closed)
|
Created:
5 years, 4 months ago by Jin Yang Modified:
5 years, 3 months ago Reviewers:
Ken Russell (switch to Gerrit), chrishtr, bajones, dglazkov, piman, Zhenyao Mo, no sievers, pdr. CC:
blink-reviews, krit, drott+blinkwatch_chromium.org, Rik, dshwang, jbroman, Justin Novosad, danakj, dglazkov+blink, pdr+graphicswatchlist_chromium.org, f(malita), Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd GL_CHROMIUM_screen_space_antialiasing to support alternative AA
MSAA is very expensive on some platforms. We introduce extention
GL_CHROMIUM_screen_space_antialiasing to support other antialiasing
approach. It will perform antialiasing to the color attachments of
the currently bound draw framebuffer. As an example, we implement it
using GL_INTEL_framebuffer_CMAA which results in similar quality of
MSAA at better performance.
The chromium side change is in below link:
https://codereview.chromium.org/1298523003/
BUG=524285
Committed: https://crrev.com/00dfe9ca54edc8f8ce8e7bb0bb3295870669bb33
git-svn-id: svn://svn.chromium.org/blink/trunk@202174 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Patch Set 1 #
Total comments: 5
Patch Set 2 : modify according to comments #Patch Set 3 : add extension #Patch Set 4 : fix naming #
Total comments: 1
Patch Set 5 : add empty implementation #
Messages
Total messages: 34 (10 generated)
jin.a.yang@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, piman@chromium.org, sievers@chromium.org, zmo@chromium.org
A couple of minor nits, but otherwise looks good. I can't give official approval as I'm not an owner of these files. https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... Source/platform/graphics/gpu/DrawingBuffer.cpp:440: if (m_extensionsUtil->supportsExtension("GL_INTEL_framebuffer_CMAA")) { Nit: This should be an `else if` https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... File Source/platform/graphics/gpu/DrawingBuffer.h (right): https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... Source/platform/graphics/gpu/DrawingBuffer.h:288: INTELCMAAResolve, Nit: I know that you're following the capitalization pattern from the extension here, but the Blink style guide indicates this should be "IntelCMAAResolve"
https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = INTELCMAAResolve; drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into MSAAImplicitResolve/MSAAExplicitResolve ?
Thanks, this looks good overall. Couple of questions based on bajones' and piman's reviews. Will give approval when they're answered/addressed. Please also see comment on https://codereview.chromium.org/1298523003/ . https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... Source/platform/graphics/gpu/DrawingBuffer.cpp:440: if (m_extensionsUtil->supportsExtension("GL_INTEL_framebuffer_CMAA")) { On 2015/08/19 16:26:44, bajones wrote: > Nit: This should be an `else if` Or does the Intel hardware supporting INTEL_framebuffer_CMAA support both extensions? Is CMAA preferred by Intel? https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = INTELCMAAResolve; On 2015/08/19 16:36:51, piman (slow to review) wrote: > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > MSAAImplicitResolve/MSAAExplicitResolve ? I like Antoine's suggestion. Could you please make that change?
On 2015/08/19 16:26:44, bajones wrote: > A couple of minor nits, but otherwise looks good. I can't give official approval > as I'm not an owner of these files. > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > Source/platform/graphics/gpu/DrawingBuffer.cpp:440: if > (m_extensionsUtil->supportsExtension("GL_INTEL_framebuffer_CMAA")) { > Nit: This should be an `else if` > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > File Source/platform/graphics/gpu/DrawingBuffer.h (right): > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > Source/platform/graphics/gpu/DrawingBuffer.h:288: INTELCMAAResolve, > Nit: I know that you're following the capitalization pattern from the extension > here, but the Blink style guide indicates this should be "IntelCMAAResolve" Done.
On 2015/08/19 17:56:18, Ken Russell wrote: > Thanks, this looks good overall. Couple of questions based on bajones' and > piman's reviews. Will give approval when they're answered/addressed. Please also > see comment on https://codereview.chromium.org/1298523003/ . > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > Source/platform/graphics/gpu/DrawingBuffer.cpp:440: if > (m_extensionsUtil->supportsExtension("GL_INTEL_framebuffer_CMAA")) { > On 2015/08/19 16:26:44, bajones wrote: > > Nit: This should be an `else if` > > Or does the Intel hardware supporting INTEL_framebuffer_CMAA support both > extensions? Is CMAA preferred by Intel? > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = > INTELCMAAResolve; > On 2015/08/19 16:36:51, piman (slow to review) wrote: > > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into > > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > > MSAAImplicitResolve/MSAAExplicitResolve ? > > I like Antoine's suggestion. Could you please make that change? Nice suggestion! Updated the patch according to the comments
On 2015/08/19 16:36:51, piman (slow to review) wrote: > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = > INTELCMAAResolve; > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > MSAAImplicitResolve/MSAAExplicitResolve ? Patch updated accordingly
On 2015/08/21 06:05:49, Jin Yang wrote: > On 2015/08/19 16:36:51, piman (slow to review) wrote: > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = > > INTELCMAAResolve; > > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into > > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > > MSAAImplicitResolve/MSAAExplicitResolve ? > > Patch updated accordingly @kbr, PTAL
On 2015/08/24 16:08:48, Jin Yang wrote: > On 2015/08/21 06:05:49, Jin Yang wrote: > > On 2015/08/19 16:36:51, piman (slow to review) wrote: > > > > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > > > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > > Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = > > > INTELCMAAResolve; > > > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode into > > > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > > > MSAAImplicitResolve/MSAAExplicitResolve ? > > > > Patch updated accordingly > > @kbr, PTAL Let's resolve the discussion in https://codereview.chromium.org/1298523003/ before moving forward with this patch.
On 2015/08/25 01:05:15, Ken Russell wrote: > On 2015/08/24 16:08:48, Jin Yang wrote: > > On 2015/08/21 06:05:49, Jin Yang wrote: > > > On 2015/08/19 16:36:51, piman (slow to review) wrote: > > > > > > > > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > > > File Source/platform/graphics/gpu/DrawingBuffer.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1301923002/diff/1/Source/platform/graphics/gp... > > > > Source/platform/graphics/gpu/DrawingBuffer.cpp:441: m_multisampleMode = > > > > INTELCMAAResolve; > > > > drive-by nit: CMAA is not multisampling, maybe rename m_multisampleMode > into > > > > m_antiAliasingMode, and then ImplicitResolve/ExplicitResolve into > > > > MSAAImplicitResolve/MSAAExplicitResolve ? > > > > > > Patch updated accordingly > > > > @kbr, PTAL > > Let's resolve the discussion in https://codereview.chromium.org/1298523003/ > before moving forward with this patch. kbr, PTAL for this part. thanks.
Thanks for the update. This side LGTM but please don't land it until the other side is resolved. (Probably wouldn't be possible, anyway.)
The CQ bit was checked by jin.a.yang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301923002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301923002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kbr@chromium.org changed reviewers: + chrishtr@chromium.org, dglazkov@chromium.org
dglazkov, chrishtr: OWNERS review of WebGraphicsContext3D.h please.
On 2015/09/11 at 18:14:20, kbr wrote: > dglazkov, chrishtr: OWNERS review of WebGraphicsContext3D.h please. LGTM
https://codereview.chromium.org/1301923002/diff/60001/public/platform/WebGrap... File public/platform/WebGraphicsContext3D.h (right): https://codereview.chromium.org/1301923002/diff/60001/public/platform/WebGrap... public/platform/WebGraphicsContext3D.h:380: virtual void applyScreenSpaceAntialiasingCHROMIUM() = 0; This will either need to be given an empty implementation or the tests will need to be updated. Probably adding an empty implementation is the easiest way to go.
On 2015/09/11 18:39:16, Ken Russell wrote: > https://codereview.chromium.org/1301923002/diff/60001/public/platform/WebGrap... > File public/platform/WebGraphicsContext3D.h (right): > > https://codereview.chromium.org/1301923002/diff/60001/public/platform/WebGrap... > public/platform/WebGraphicsContext3D.h:380: virtual void > applyScreenSpaceAntialiasingCHROMIUM() = 0; > This will either need to be given an empty implementation or the tests will need > to be updated. Probably adding an empty implementation is the easiest way to go. Thanks, Ken! Done
The CQ bit was checked by jin.a.yang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1301923002/#ps80001 (title: "add empty implementation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301923002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301923002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/09/12 02:25:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) Filed http://crbug.com/530847 about android_webview_unittests flakiness.
The CQ bit was checked by kbr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301923002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301923002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by jin.a.yang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301923002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301923002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202174
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/00dfe9ca54edc8f8ce8e7bb0bb3295870669bb33 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
