The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf7c3813f7d59a971b590ddb2e94d4a6afc84e11

commit bf7c3813f7d59a971b590ddb2e94d4a6afc84e11
Author: Daniele Castagna <dcastagna@chromium.org>
Date: Tue Oct 30 23:51:40 2018

cc: Don't use BGRA_EXT with glCopyTexImage2d

When GLRenderer reads back from the framebuffer with glCopyTexImage2d,
it tries to be smart about what internal format to use for the new texture.

Unfortunately on CrOS it ends up using GL_BGRA_EXT, that seems to hit
a particularly slow path in gles2_cmd_decoder.

This CL changes it to RGB/RGBA, decreasing the CPU cost of CopyTexImage2d
of two orders of magnitude when applying fullscreen backdrop effects.

Bug:  900046 

Change-Id: I19bd8a9d008109e336f0fd346df130c2d7878154
Reviewed-on: https://chromium-review.googlesource.com/c/1306653
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604062}
[modify] https://crrev.com/bf7c3813f7d59a971b590ddb2e94d4a6afc84e11/components/viz/service/display/gl_renderer.cc

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Before we approve merge to M71, please answer followings:
* Is this M71 regression? Is it critical?
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M71?
* Any other important details to justify the merge.
Please note M71 is already in Beta, so merge bar is very high. Thank you.


Label"RegressedIn-67" is applied in bug , if that is a case this is not a merge candidate for M71. 

@#4:
* This fixes a significant performance regression for backdrop filters, that we have not used until recently. Pixel Slate ChromeOS UI has significant performance issues without this fix.
* The change itself is pretty contained. We can wait a few days until it reaches a canary version on CrOS if you prefer. While the fix is for ChromeOS ui, the change should be well exercised on any platform.
* This is critical for Pixel Slate launch.

I'm going to cc omri that might be able to better justify this merge.

Regarding the last sentence, while this likely regressed in 67, we were not using backdrop filters on the UI there, so it was not affecting anyone.
Additionally, Pixel Slate is going to launch with 71, and that's what we're trying to target here.

+kbleicher@ (Chrome OS M71 Release TPM) for review as per comment #5, change is needed for Pixel Slate ChromeOS. 

Removing non Chrome OSs from list per comment #5. Pls add back if needed. Thank you.

re "miu's mentioned" in Comment 0: What I meant by it was that the problem in this bug may be analogous to a similar performance concern around the glReadPixels() use cases. In order to avoid GPU drivers, or other OpenGL-related platform libraries, doing extra R and B channel swizzling on the CPU (with a cost essentially equivalent to a full memcpy() of a bitmap), we query for the platform's preferred internal representation (GL_RGBA versus GL_BGRA_EXT), and use that.

So, if the issue here is that we're having to swizzle on the CPU in gles2_cmd_decoder.cc, then maybe the CL committed in Comment 1 will improve performance on CrOS but would be making it worse on other platforms? Have we tested performance on Win/Mac as well, to confirm the change is good for all?

The path we hit when using GL_BGRA_EXT was slow in gles2_cmd_decoder.cc not because we were swizzling, just because we did a texImage2d(..., malloc(pixel_size)) service side to initialize the texture, and that was not necessary.

The CL we landed affects backdrop filters only, that are disabled by default on all platforms, and we currently use only for CrOS UI. 

re Comment 9: Ah, now I understand. SGTM.

Approving merge to M71 Chrome OS.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18eafc251421488e434121a965af6b8a160e2491

commit 18eafc251421488e434121a965af6b8a160e2491
Author: Daniele Castagna <dcastagna@chromium.org>
Date: Fri Nov 02 21:36:32 2018

cc: Don't use BGRA_EXT with glCopyTexImage2d

Merge to 71.

When GLRenderer reads back from the framebuffer with glCopyTexImage2d,
it tries to be smart about what internal format to use for the new texture.

Unfortunately on CrOS it ends up using GL_BGRA_EXT, that seems to hit
a particularly slow path in gles2_cmd_decoder.

This CL changes it to RGB/RGBA, decreasing the CPU cost of CopyTexImage2d
of two orders of magnitude when applying fullscreen backdrop effects.

Bug:  900046 

TBR=dcastagna@chromium.org

(cherry picked from commit bf7c3813f7d59a971b590ddb2e94d4a6afc84e11)

Change-Id: I19bd8a9d008109e336f0fd346df130c2d7878154
Reviewed-on: https://chromium-review.googlesource.com/c/1306653
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604062}
Reviewed-on: https://chromium-review.googlesource.com/c/1316115
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#479}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/18eafc251421488e434121a965af6b8a160e2491/components/viz/service/display/gl_renderer.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/18eafc251421488e434121a965af6b8a160e2491

Commit: 18eafc251421488e434121a965af6b8a160e2491
Author: dcastagna@chromium.org
Commiter: dcastagna@chromium.org
Date: 2018-11-02 21:36:32 +0000 UTC

cc: Don't use BGRA_EXT with glCopyTexImage2d

Merge to 71.

When GLRenderer reads back from the framebuffer with glCopyTexImage2d,
it tries to be smart about what internal format to use for the new texture.

Unfortunately on CrOS it ends up using GL_BGRA_EXT, that seems to hit
a particularly slow path in gles2_cmd_decoder.

This CL changes it to RGB/RGBA, decreasing the CPU cost of CopyTexImage2d
of two orders of magnitude when applying fullscreen backdrop effects.

Bug:  900046 

TBR=dcastagna@chromium.org

(cherry picked from commit bf7c3813f7d59a971b590ddb2e94d4a6afc84e11)

Change-Id: I19bd8a9d008109e336f0fd346df130c2d7878154
Reviewed-on: https://chromium-review.googlesource.com/c/1306653
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604062}
Reviewed-on: https://chromium-review.googlesource.com/c/1316115
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#479}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}