gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

Add new unittests for glApplyScreenSpaceAntialiasingCHROMIUM, which check the
conformance with GL_INTEL_framebuffer_CMAA spec [1].

Many tests are very similar to tests of GLCopyTextureCHROMIUMTest.

[1] https://www.khronos.org/registry/gles/extensions/INTEL/framebuffer_CMAA.txt

BUG=535198
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

Committed: https://crrev.com/a5d6052cdc38c8d12bb7d4686177d67809b1f66e
Cr-Commit-Position: refs/heads/master@{#427326}

[dshwang, 2016-10-19 15:10:53]

Description was changed from

==========
gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

Add new unittests for glApplyScreenSpaceAntialiasingCHROMIUM, which check the
conformance with GL_INTEL_framebuffer_CMAA spec [1].

Many tests are very similar to tests of GLCopyTextureCHROMIUMTest.

[1] https://www.khronos.org/registry/gles/extensions/INTEL/framebuffer_CMAA.txt

BUG=535198
==========

to

==========
gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

Add new unittests for glApplyScreenSpaceAntialiasingCHROMIUM, which check the
conformance with GL_INTEL_framebuffer_CMAA spec [1].

Many tests are very similar to tests of GLCopyTextureCHROMIUMTest.

[1] https://www.khronos.org/registry/gles/extensions/INTEL/framebuffer_CMAA.txt

BUG=535198
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
==========

[inactive_dshwang_plz_cc_intel, 2016-10-19 15:11:00]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 15:11:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-19 15:12:06]

dongseong.hwang@intel.com changed reviewers:
+ adrian.belgun@intel.com, piman@chromium.org

[dshwang, 2016-10-19 15:12:07]

piman@: could you review?

[commit-bot: I haz the power, 2016-10-19 16:09:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 16:09:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2016-10-19 19:10:43]

Thanks for the tests, they look great. Couple of things...

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/build_gl...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/build_gl...
gpu/command_buffer/build_gles2_cmd_buffer.py:2393: 'extension_flag':
'chromium_screen_space_antialiasing',
Why this change?

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/tests/gl...
File
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc
(right):

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:100:
GLuint textures_;
nit: texture_  (singular)

Please initialize this pod field (as well as framebuffer_id_).

[inactive_dshwang_plz_cc_intel, 2016-10-20 16:20:38]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 16:20:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-20 16:21:20]

thx for reviewing. I resolved all nits. could you review again?

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/build_gl...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/build_gl...
gpu/command_buffer/build_gles2_cmd_buffer.py:2393: 'extension_flag':
'chromium_screen_space_antialiasing',
On 2016/10/19 19:10:43, piman wrote:
> Why this change?

oh, mistake. reverted.

In the detail, I tried to check if it's supported by calling
glApplyScreenSpaceAntialiasingCHROMIUM, but later I found
GLTestHelper::HasExtension("GL_CHROMIUM_screen_space_antialiasing")

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/tests/gl...
File
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc
(right):

https://codereview.chromium.org/2432413003/diff/1/gpu/command_buffer/tests/gl...
gpu/command_buffer/tests/gl_apply_screen_space_antialiasing_CHROMIUM_unittest.cc:100:
GLuint textures_;
On 2016/10/19 19:10:43, piman wrote:
> nit: texture_  (singular)
> 
> Please initialize this pod field (as well as framebuffer_id_).

Done.

[commit-bot: I haz the power, 2016-10-20 17:56:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 17:56:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2016-10-20 18:36:17]

lgtm

https://codereview.chromium.org/2432413003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2432413003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16728: }
Can you update
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt to document
this new restriction?

What would be required to support it on the main framebuffer? In the future we
may want to do direct-to-main-framebuffer rendering for skia.

[dshwang, 2016-10-20 19:12:19]

Thank you for reviewing. Let me land it with replying your comment.

https://codereview.chromium.org/2432413003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2432413003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16728: }
On 2016/10/20 18:36:17, piman wrote:
> Can you update
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt to
document
> this new restriction?
> 
> What would be required to support it on the main framebuffer? In the future we
> may want to do direct-to-main-framebuffer rendering for skia.

good question.

https://cs.chromium.org/chromium/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_s...

CHROMIUM_screen_space_antialiasing.txt already implies the extensions doesn't
work after glBindFramebuffer(0).

In fact, chromium implementation can work even after calling
glBindFramebuffer(0), because offscreen GLES2 decoder has FBO as default
framebuffer, rather than EGLSurface or something.

In summary, I add this restriction so that
CHROMIUM_screen_space_antialiasing.txt has simple explanation.

As far as I know, skia ganesh creates its own default FBO, rather than using
glBindFramebuffer(0), because it makes it efficient to do
canvas.draw(another_canvas).
When we support cmaa in skia and skia really need glBindFramebuffer(0), I will
change hear to support default FBO of offscreen GLES2 decoder, while it doesn't
support default FBO of onscreen GLES2 decoder tho.

[dshwang, 2016-10-20 19:13:08]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2016-10-20 19:13:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-10-20 19:45:26]

The CQ bit was unchecked by piman@chromium.org

[piman, 2016-10-20 19:47:14]

On Thu, Oct 20, 2016 at 12:12 PM, <dongseong.hwang@intel.com> wrote:

> Thank you for reviewing. Let me land it with replying your comment.
>
>
> https://codereview.chromium.org/2432413003/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
>
> https://codereview.chromium.org/2432413003/diff/20001/gpu/
> command_buffer/service/gles2_cmd_decoder.cc#newcode16728
> gpu/command_buffer/service/gles2_cmd_decoder.cc:16728: }
> On 2016/10/20 18:36:17, piman wrote:
> > Can you update
> > gpu/GLES2/extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt
> to document
> > this new restriction?
> >
> > What would be required to support it on the main framebuffer? In the
> future we
> > may want to do direct-to-main-framebuffer rendering for skia.
>
> good question.
>
> https://cs.chromium.org/chromium/src/gpu/GLES2/
> extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt?
> q=CHROMIUM_screen_space_antialiasing&sq=package:chromium&l=31
>
> CHROMIUM_screen_space_antialiasing.txt already implies the extensions
> doesn't work after glBindFramebuffer(0).
>

It implies, but does not document - in particular it doesn't mention the
error that is raised if the default framebuffer is bound. Please add this
before landing (I unchecked the cq)

In fact, chromium implementation can work even after calling
> glBindFramebuffer(0), because offscreen GLES2 decoder has FBO as default
> framebuffer, rather than EGLSurface or something.
>

This is only true for offscreen contexts. The onscreen contexts (the ones
we use to render to the window) do use the default framebuffer.

In summary, I add this restriction so that
> CHROMIUM_screen_space_antialiasing.txt has simple explanation.
>
> As far as I know, skia ganesh creates its own default FBO, rather than
> using glBindFramebuffer(0), because it makes it efficient to do
> canvas.draw(another_canvas).
>

This is true for canvas, but we're increasingly interested in using skia to
do at least some rendering to the window.


> When we support cmaa in skia and skia really need glBindFramebuffer(0),
> I will change hear to support default FBO of offscreen GLES2 decoder,
> while it doesn't support default FBO of onscreen GLES2 decoder tho.
>
> https://codereview.chromium.org/2432413003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[inactive_dshwang_plz_cc_intel, 2016-10-21 09:48:42]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 09:48:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-21 09:58:42]

Hi, I add the explicit description in txt file.
Could you approve to land?

On 2016/10/20 19:47:14, piman wrote:
> > good question.
> >
> > https://cs.chromium.org/chromium/src/gpu/GLES2/
> > extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt?
> > q=CHROMIUM_screen_space_antialiasing&sq=package:chromium&l=31
> >
> > CHROMIUM_screen_space_antialiasing.txt already implies the extensions
> > doesn't work after glBindFramebuffer(0).
> >
> 
> It implies, but does not document - in particular it doesn't mention the
> error that is raised if the default framebuffer is bound. Please add this
> before landing (I unchecked the cq)

Done. Add new description about INVALID_OPERATION. Sorry for premature CQ.

> In fact, chromium implementation can work even after calling
> > glBindFramebuffer(0), because offscreen GLES2 decoder has FBO as default
> > framebuffer, rather than EGLSurface or something.
> >
> 
> This is only true for offscreen contexts. The onscreen contexts (the ones
> we use to render to the window) do use the default framebuffer.

Add comment
  // TODO(dshwang): support it even after glBindFrameBuffer(GL_FRAMEBUFFER, 0).
  // skia will need to render to the window. 

I'm also very intereted in using cmaa on skia. crbug.com/656618

> In summary, I add this restriction so that
> > CHROMIUM_screen_space_antialiasing.txt has simple explanation.
> >
> > As far as I know, skia ganesh creates its own default FBO, rather than
> > using glBindFramebuffer(0), because it makes it efficient to do
> > canvas.draw(another_canvas).
> >
> 
> This is true for canvas, but we're increasingly interested in using skia to
> do at least some rendering to the window.

I guess you talk about gpu rasterization by skia ganesh.

gpu rasterization allocates texture by resource_pool_ and binds it to SkSurface.
So glBindFrameBuffer(GL_FRAMEBUFFER, 0) is not needed.
https://cs.chromium.org/chromium/src/cc/tiles/tile_manager.cc?q=AcquireBuffer...

If you talk about other projects, I'm very curious. Could you share me more?
In addition, you or googler might have some plan about skia and cmaa. Can I
listen to more? 
Welcome to feedback in crbug.com/656618

[commit-bot: I haz the power, 2016-10-21 11:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 11:46:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piman, 2016-10-21 20:16:45]

lgtm

[piman, 2016-10-21 20:17:02]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:17:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-10-21 20:20:07]

On Fri, Oct 21, 2016 at 2:58 AM, <dongseong.hwang@intel.com> wrote:

> Hi, I add the explicit description in txt file.
> Could you approve to land?
>
> On 2016/10/20 19:47:14, piman wrote:
> > > good question.
> > >
> > > https://cs.chromium.org/chromium/src/gpu/GLES2/
> > > extensions/CHROMIUM/CHROMIUM_screen_space_antialiasing.txt?
> > > q=CHROMIUM_screen_space_antialiasing&sq=package:chromium&l=31
> > >
> > > CHROMIUM_screen_space_antialiasing.txt already implies the extensions
> > > doesn't work after glBindFramebuffer(0).
> > >
> >
> > It implies, but does not document - in particular it doesn't mention the
> > error that is raised if the default framebuffer is bound. Please add this
> > before landing (I unchecked the cq)
>
> Done. Add new description about INVALID_OPERATION. Sorry for premature CQ.
>
> > In fact, chromium implementation can work even after calling
> > > glBindFramebuffer(0), because offscreen GLES2 decoder has FBO as
> default
> > > framebuffer, rather than EGLSurface or something.
> > >
> >
> > This is only true for offscreen contexts. The onscreen contexts (the ones
> > we use to render to the window) do use the default framebuffer.
>
> Add comment
> // TODO(dshwang): support it even after glBindFrameBuffer(GL_FRAMEBUFFER,
> 0).
> // skia will need to render to the window.
>
> I'm also very intereted in using cmaa on skia. crbug.com/656618
>
> > In summary, I add this restriction so that
> > > CHROMIUM_screen_space_antialiasing.txt has simple explanation.
> > >
> > > As far as I know, skia ganesh creates its own default FBO, rather than
> > > using glBindFramebuffer(0), because it makes it efficient to do
> > > canvas.draw(another_canvas).
> > >
> >
> > This is true for canvas, but we're increasingly interested in using skia
> to
> > do at least some rendering to the window.
>
> I guess you talk about gpu rasterization by skia ganesh.
>
> gpu rasterization allocates texture by resource_pool_ and binds it to
> SkSurface.
> So glBindFrameBuffer(GL_FRAMEBUFFER, 0) is not needed.
> https://cs.chromium.org/chromium/src/cc/tiles/tile_manager.cc?q=
> AcquireBufferForRaster&sq=package:chromium&dr=C&l=987
>
> If you talk about other projects, I'm very curious. Could you share me
> more?
> In addition, you or googler might have some plan about skia and cmaa. Can I
> listen to more?
>

In a nutshell, we're looking into a generic SkiaRenderer, similar to the
SoftwareRenderer, but would work with GL and Vulkan. Initially just the
compositing, but I suspect we will investigate moving some rasterization
onto it (fewer copies for very dynamic content). This is further away.


> Welcome to feedback in crbug.com/656618
>
>
> https://codereview.chromium.org/2432413003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-10-21 23:15:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 23:15:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[inactive_dshwang_plz_cc_intel, 2016-10-24 18:20:19]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 18:20:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 20:50:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 20:50:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dshwang, 2016-10-25 11:50:40]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2016-10-25 11:50:41]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2432413003/#ps60001
(title: "fix linux_chromium_rel_ng")

[commit-bot: I haz the power, 2016-10-25 11:50:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 12:30:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 12:32:12]

Description was changed from

==========
gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

Add new unittests for glApplyScreenSpaceAntialiasingCHROMIUM, which check the
conformance with GL_INTEL_framebuffer_CMAA spec [1].

Many tests are very similar to tests of GLCopyTextureCHROMIUMTest.

[1] https://www.khronos.org/registry/gles/extensions/INTEL/framebuffer_CMAA.txt

BUG=535198
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

==========
gpu, cmaa: add glApplyScreenSpaceAntialiasingCHROMIUM unittests

Add new unittests for glApplyScreenSpaceAntialiasingCHROMIUM, which check the
conformance with GL_INTEL_framebuffer_CMAA spec [1].

Many tests are very similar to tests of GLCopyTextureCHROMIUMTest.

[1] https://www.khronos.org/registry/gles/extensions/INTEL/framebuffer_CMAA.txt

BUG=535198
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

Committed: https://crrev.com/a5d6052cdc38c8d12bb7d4686177d67809b1f66e
Cr-Commit-Position: refs/heads/master@{#427326}
==========

[commit-bot: I haz the power, 2016-10-25 12:32:13]

Patchset 4 (id:??) landed as
https://crrev.com/a5d6052cdc38c8d12bb7d4686177d67809b1f66e
Cr-Commit-Position: refs/heads/master@{#427326}

[ajuma, 2016-10-25 14:00:02]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2451803002/ by ajuma@chromium.org.

The reason for reverting is: The tests added here are crashing on an assertion
on multiple GPU Debug bots. For example:
https://build.chromium.org/p/chromium.gpu/builders/Mac%2010.10%20Retina%20Deb...
https://build.chromium.org/p/chromium.gpu/builders/Win7%20Debug%20%28NVIDIA%2...

Snippet from the log:
[548:3764:1025/061243:2681579:FATAL:gles2_implementation_impl_autogen.h(606)]
Check failed: textures[i] != 0. 
Backtrace:
	base::debug::StackTrace::StackTrace [0x00A7B3E7+23]
	logging::LogMessage::~LogMessage [0x00ACD4EB+59]
	gpu::gles2::GLES2Implementation::DeleteTextures [0x03DB1732+626]
	GLES2DeleteTextures [0x03EB5954+36]
	gpu::GLApplyScreenSpaceAntialiasingCHROMIUMTest::TearDown [0x00442AC8+24]
.

[dshwang, 2016-10-25 14:24:13]

On 2016/10/25 14:00:02, ajuma wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2451803002/ by mailto:ajuma@chromium.org.
> 
> The reason for reverting is: The tests added here are crashing on an assertion
> on multiple GPU Debug bots. For example:
>
https://build.chromium.org/p/chromium.gpu/builders/Mac%2010.10%20Retina%20Deb...
>
https://build.chromium.org/p/chromium.gpu/builders/Win7%20Debug%20%28NVIDIA%2...
> 
> Snippet from the log:
> [548:3764:1025/061243:2681579:FATAL:gles2_implementation_impl_autogen.h(606)]
> Check failed: textures[i] != 0. 
> Backtrace:
> 	base::debug::StackTrace::StackTrace [0x00A7B3E7+23]
> 	logging::LogMessage::~LogMessage [0x00ACD4EB+59]
> 	gpu::gles2::GLES2Implementation::DeleteTextures [0x03DB1732+626]
> 	GLES2DeleteTextures [0x03EB5954+36]
> 	gpu::GLApplyScreenSpaceAntialiasingCHROMIUMTest::TearDown [0x00442AC8+24]
> .

Thx for reverting as well as sharing stack trace.