cc: Use the correct internal format for glCopyTexImage2D calls.

cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[danakj, 2016-06-21 21:53:37]

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

[danakj, 2016-06-21 21:53:42]

Description was changed from

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
==========

to

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-21 21:54:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/1

[danakj, 2016-06-21 22:01:34]

Patchset #2 (id:20001) has been deleted

[danakj, 2016-06-21 22:52:12]

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

[commit-bot: I haz the power, 2016-06-21 22:53:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/40001

[danakj, 2016-06-21 23:01:06]

Patchset #2 (id:40001) has been deleted

[danakj, 2016-06-21 23:10:00]

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

[danakj, 2016-06-21 23:10:21]

danakj@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[danakj, 2016-06-21 23:10:22]

+bo for webview
+sky for mus

[commit-bot: I haz the power, 2016-06-21 23:10:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/60001

[commit-bot: I haz the power, 2016-06-21 23:21:03]

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

[commit-bot: I haz the power, 2016-06-21 23:21:05]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[danakj, 2016-06-21 23:35:18]

Description was changed from

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-06-21 23:35:27]

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

[danakj, 2016-06-21 23:35:59]

I changed cc pixel tests to fail without the change in GLRenderer, which
required some rebaselining then.

[commit-bot: I haz the power, 2016-06-21 23:36:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/90001

[danakj, 2016-06-21 23:36:43]

Patchset #1 (id:1) has been deleted

[sky, 2016-06-21 23:40:35]

LGTM

https://codereview.chromium.org/2089753003/diff/90001/components/mus/public/c...
File components/mus/public/cpp/lib/output_surface.cc (right):

https://codereview.chromium.org/2089753003/diff/90001/components/mus/public/c...
components/mus/public/cpp/lib/output_surface.cc:38: // This is a delegating
output surface, no framebuffer/direct drawing support.
Thanks for the comments!

[Fady Samuel, 2016-06-21 23:42:17]

lgtm

[commit-bot: I haz the power, 2016-06-21 23:42:35]

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

[commit-bot: I haz the power, 2016-06-21 23:42:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[boliu, 2016-06-21 23:57:24]

a_w lgtm

[danakj, 2016-06-22 00:14:43]

danakj@chromium.org changed reviewers:
+ nyquist@chromium.org

[danakj, 2016-06-22 00:14:44]

+nyquist for blimp/

[danakj, 2016-06-22 00:16:09]

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

[commit-bot: I haz the power, 2016-06-22 00:16:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/150001

[commit-bot: I haz the power, 2016-06-22 00:53:20]

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

[commit-bot: I haz the power, 2016-06-22 00:53:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danakj, 2016-06-22 00:59:39]

Patchset #5 (id:130001) has been deleted

[danakj, 2016-06-22 01:00:11]

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

[commit-bot: I haz the power, 2016-06-22 01:01:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/170001

[piman, 2016-06-22 01:15:03]

lgtm

https://codereview.chromium.org/2089753003/diff/170001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2089753003/diff/170001/cc/output/gl_renderer....
cc/output/gl_renderer.h:513: // This is valud when current_framebuffer_lock_ is
not null.
nit: valud->valid

[commit-bot: I haz the power, 2016-06-22 02:32:44]

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

[commit-bot: I haz the power, 2016-06-22 02:32:46]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danakj, 2016-06-22 20:54:01]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-06-22 20:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, sky@chromium.org, boliu@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2089753003/#ps190001
(title: "copytextureformat: windowsbaseline")

[danakj, 2016-06-22 20:54:33]

The CQ bit was unchecked by danakj@chromium.org

[commit-bot: I haz the power, 2016-06-22 20:54:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/190001

[danakj, 2016-06-22 20:55:11]

https://codereview.chromium.org/2089753003/diff/170001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2089753003/diff/170001/cc/output/gl_renderer....
cc/output/gl_renderer.h:513: // This is valud when current_framebuffer_lock_ is
not null.
On 2016/06/22 01:15:03, piman wrote:
> nit: valud->valid

Done.

[danakj, 2016-06-22 20:55:45]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-06-22 20:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
boliu@chromium.org, piman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2089753003/#ps210001
(title: "copytextureformat: typo")

[commit-bot: I haz the power, 2016-06-22 20:57:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/210001

[danakj, 2016-06-22 20:59:51]

The CQ bit was unchecked by danakj@chromium.org

[Wez, 2016-06-22 21:02:20]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2016-06-22 21:02:21]

blimp/ LGTM w/ comment addressed.

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.h (right):

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.h:46: uint32_t
GetCopyTextureInternalFormat();
nit: Please add a comment e.g. "Returns the GL texture format, for use by the
owning OutputSurface implementation." so it's clear why we need this here, in
addition to the ContextProvider interface.

nit: Should ContextProvider interface have this method too, so this becomes an
override? Are there ContextProviders for which the texture format is not
relevant, or is well-known to the caller..?

[danakj, 2016-06-22 21:06:50]

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.h (right):

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.h:46: uint32_t
GetCopyTextureInternalFormat();
On 2016/06/22 21:02:21, Wez wrote:
> nit: Please add a comment e.g. "Returns the GL texture format, for use by the
> owning OutputSurface implementation." so it's clear why we need this here, in
> addition to the ContextProvider interface.

Done, here and on the other context providers too.

> nit: Should ContextProvider interface have this method too, so this becomes an
> override? Are there ContextProviders for which the texture format is not
> relevant, or is well-known to the caller..?

Since every place that wants this data knows the concrete type of the
ContextProvider, I'd rather not make it part of the general API.

[danakj, 2016-06-22 21:07:25]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-06-22 21:07:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org, boliu@chromium.org, piman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2089753003/#ps230001
(title: "copytextureformat: comments")

[Wez, 2016-06-22 21:08:14]

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.h (right):

https://codereview.chromium.org/2089753003/diff/210001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.h:46: uint32_t
GetCopyTextureInternalFormat();
On 2016/06/22 21:06:50, danakj wrote:
> On 2016/06/22 21:02:21, Wez wrote:
> > nit: Please add a comment e.g. "Returns the GL texture format, for use by
the
> > owning OutputSurface implementation." so it's clear why we need this here,
in
> > addition to the ContextProvider interface.
> 
> Done, here and on the other context providers too.
> 
> > nit: Should ContextProvider interface have this method too, so this becomes
an
> > override? Are there ContextProviders for which the texture format is not
> > relevant, or is well-known to the caller..?
> 
> Since every place that wants this data knows the concrete type of the
> ContextProvider, I'd rather not make it part of the general API.

Acknowledged.

[commit-bot: I haz the power, 2016-06-22 21:08:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089753003/230001

[commit-bot: I haz the power, 2016-06-22 22:26:02]

Description was changed from

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-22 22:26:05]

Committed patchset #9 (id:230001)

[commit-bot: I haz the power, 2016-06-22 22:28:38]

Description was changed from

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Use the correct internal format for glCopyTexImage2D calls.

This uses the correct format for glCopyTexImage2D calls from the
default framebuffer (for the root renderpass). This function is used
for the implementation of backdrop filters.

Currently it uses GL_RGBA, but it is not valid to add a channel, so
if the framebuffer is only RGB, then we must use GL_RGB or it will fail.

This is demonstrated by the backdrop filter layout tests once they
use a cc::Display for doing GL drawing, instead of the current method
with a MailboxOutputSurface, which happens to provide an RGBA
framebuffer.

The layout tests are:
css3/filters/backdrop-filter-rendering.html
css3/filters/backdrop-filter-rendering-no-background.html

This also changes the ContextProvider used by cc pixel tests
to drop the alpha. With that, a number of pixel tests require
new baselines, as the background becomes black instead of
transparent, and some tests use background filters or leave some
pixels undrawn.

With the change to the pixel tests, but without the change in
GLRenderer, many cc pixel tests fail showing black for any
background filter, as exhibited by bug 612238, along with the
error "GL ERROR :GL_INVALID_OPERATION : glCopyTexImage2D:
incompatible format".

R=fsamuel@chromium.org, piman@chromium.org, sievers
BUG=311404, 612238
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-06-22 22:28:41]

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