Improve tracking of bound FBOs in GrGLGpu.

Improve tracking of bound FBOs in GrGLGpu.

Committed: https://skia.googlesource.com/skia/+/d2ad8eb5801e2c8c0fa544a6a776bb46eedde2a0

Committed: https://skia.googlesource.com/skia/+/b2af2d8b83ca4774c3b3bb1e49bc72605faa9589

Committed: https://skia.googlesource.com/skia/+/0b70b86a7e9fda52ee7ebc1b9897eeaa09b9abef

Committed: https://skia.googlesource.com/skia/+/6ba6fa15261be591f33cf0e5df7134e4fc6432ac

Committed: https://skia.googlesource.com/skia/+/dc963b9264908f53650c40a97cff414101dd3e88

Committed: https://skia.googlesource.com/skia/+/160f24ce0e8d6dd7ca80b78871e063d4f4609cfb

[bsalomon, 2015-02-24 17:35:28]

bsalomon@google.com changed reviewers:
+ egdaniel@google.com

[bsalomon, 2015-02-24 17:35:29]

I haven't even started to test this, but wanted to keep you in the loop and get
any early feedback on the direction of this. Needs some comments and cleanup.

The two main goals are:

Robust track bindings to GL_FRAMEBUFFER, GL_READ_FRAMEBUFFER, and
GL_DRAW_FRAMEBUFFER, including tracking of grglgpu's fbo ids which we currently
don't track.

Centralize binding of FBOs (remove code duplication and have one place to get
the determination of which FBO enum to use).

[egdaniel, 2015-02-24 18:47:02]

So the one main thought I have (and this is since I need something similar for
my change), I feel like it would be useful to have functions on the FBO class
for binding textures, stencils, etc. This would at least hide all the explicit
GL_CALLs that are made. We are already hiding the bind, so I feel like it would
be nice to say like fbo->attachTexture(...)

https://codereview.chromium.org/949263002/diff/1/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/949263002/diff/1/src/gpu/gl/GrGLGpu.h#newcode485
src/gpu/gl/GrGLGpu.h:485: } fHWFBOBinding[2];
Do we want to distinguish these as 0 is draw and 1 is read? Or is safe to just
use them internally as such without really forcing anything?

[bsalomon, 2015-02-24 21:00:53]

I agree with the binding helpers but would rather save that for future work, as
this change is already not small.

I put a little polish on it. PTAL.

https://codereview.chromium.org/949263002/diff/1/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/949263002/diff/1/src/gpu/gl/GrGLGpu.h#newcode485
src/gpu/gl/GrGLGpu.h:485: } fHWFBOBinding[2];
On 2015/02/24 18:47:01, egdaniel wrote:
> Do we want to distinguish these as 0 is draw and 1 is read? Or is safe to just
> use them internally as such without really forcing anything?

I added a cnt value and used it here. In the impl I used the enum values to
index the array.

[egdaniel, 2015-02-25 16:21:34]

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLGpu.h#new...
src/gpu/gl/GrGLGpu.h:53: void willDeleteOrAbandonFramebuffer(GrGLFBO* fbo);
comment on when and why this needs to be called.

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLGpu.h#new...
src/gpu/gl/GrGLGpu.h:264: static const int kFBOBindingCnt = kLast_FBOBinding +
1;
comment on why there is a +1 here? I'm assuming it is for the invalidBinding you
make later?

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLGpu.h#new...
src/gpu/gl/GrGLGpu.h:487: } fHWFBOBinding[kFBOBindingCnt];
Do you want 3 of these? Is one for an invalidBinding?

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLRenderTar...
File src/gpu/gl/GrGLRenderTarget.h (right):

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLRenderTar...
src/gpu/gl/GrGLRenderTarget.h:21: must be deleted or abandoned before this
object is freed. FBO IDs should never be owned by
"FBO IDs should never be owned by more that one instance."

This is contradicted by the id constructor.

https://codereview.chromium.org/949263002/diff/60001/src/gpu/gl/GrGLRenderTar...
src/gpu/gl/GrGLRenderTarget.h:46: void generateIfInvalid(const GrGLInterface*
gl) {
I wonder if this should be public. I feel like it would be nice to hide this
inside bindFBO and ctor, but that does add extra conditional check each bind.

[bsalomon, 2015-02-25 19:46:22]

ptal

[egdaniel, 2015-02-26 15:44:27]

lgtm

[bsalomon, 2015-02-26 15:47:26]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-02-26 15:47:39]

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

[commit-bot: I haz the power, 2015-02-26 15:56:27]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/d2ad8eb5801e2c8c0fa544a6a776bb46eedde2a0

[bsalomon, 2015-02-26 19:23:55]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/964543002/ by bsalomon@google.com.

The reason for reverting is: breaking hd2000 windows bots.

[bsalomon, 2015-02-27 23:04:40]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-02-27 23:04:41]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/949263002/#ps120001
(title: "ensure that we use the correct fbo target enum after binding")

[commit-bot: I haz the power, 2015-02-27 23:05:35]

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

[commit-bot: I haz the power, 2015-02-27 23:06:35]

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

[commit-bot: I haz the power, 2015-02-27 23:06:37]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[bsalomon, 2015-02-27 23:09:51]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-02-27 23:09:52]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/949263002/#ps140001
(title: "fix errors")

[commit-bot: I haz the power, 2015-02-27 23:10:46]

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

[commit-bot: I haz the power, 2015-02-27 23:17:36]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/b2af2d8b83ca4774c3b3bb1e49bc72605faa9589

[bsalomon, 2015-02-28 00:27:27]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/963973002/ by bsalomon@google.com.

The reason for reverting is: breaking things.

[bsalomon, 2015-02-28 00:52:47]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-02-28 00:52:48]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/949263002/#ps180001
(title: "more assert fixes in debug context")

[bsalomon, 2015-02-28 00:57:41]

The CQ bit was unchecked by bsalomon@google.com

[bsalomon, 2015-02-28 00:57:48]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-02-28 01:04:06]

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

[commit-bot: I haz the power, 2015-02-28 01:10:08]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/0b70b86a7e9fda52ee7ebc1b9897eeaa09b9abef

[robertphillips, 2015-03-01 15:34:40]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/970613003/ by robertphillips@google.com.

The reason for reverting is: Experimentally reverting to see if this is the
cause of the mac_chromium_rel_ng roll-blocking failures..

[egdaniel, 2015-03-04 19:49:15]

lgtm

[bsalomon, 2015-03-04 19:49:35]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-03-04 19:51:30]

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

[commit-bot: I haz the power, 2015-03-04 19:57:39]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/6ba6fa15261be591f33cf0e5df7134e4fc6432ac

[bsalomon, 2015-03-14 19:08:31]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1011493002/ by bsalomon@google.com.

The reason for reverting is: That one needs to be reverted to fix black boxes
with GPU
raster, and GPU bot failures.

BUG=466690.

[bsalomon, 2015-03-17 15:15:57]

Greg, can you look at the diff since patchset 12 (the last version that landed)?

[egdaniel, 2015-03-17 17:44:58]

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1812: fHWFBOBinding[0].fFBO.reset(SkRef(fbo));
maybe add some local enum for the draw and read instead of using 0 and 1

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1815: } else {
it is not needed, but do you want to explicitly use the else here for clarity?

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1940: this->setViewport(rt->getViewport());
Does stencilPath change the RT or texture at all? Aka do we need to set anything
to dirty?

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:268: 
extra \n

[bsalomon, 2015-03-17 18:00:01]

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1812: fHWFBOBinding[0].fFBO.reset(SkRef(fbo));
On 2015/03/17 17:44:58, egdaniel wrote:
> maybe add some local enum for the draw and read instead of using 0 and 1

Done.

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1815: } else {
On 2015/03/17 17:44:58, egdaniel wrote:
> it is not needed, but do you want to explicitly use the else here for clarity?

Done.

https://codereview.chromium.org/949263002/diff/260001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1940: this->setViewport(rt->getViewport());
On 2015/03/17 17:44:58, egdaniel wrote:
> Does stencilPath change the RT or texture at all? Aka do we need to set
anything
> to dirty?

No, we only track dirtiness of the colorbuffer since it is the only thing used
to gen mips or resolve.

[egdaniel, 2015-03-17 19:18:28]

lgtm with the one question

https://codereview.chromium.org/949263002/diff/280001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/949263002/diff/280001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:344: for (size_t i = 0; i <
SK_ARRAY_COUNT(fHWFBOBinding); ++i) {
since this is no just two, draw and read...

is it clearer to have:
if(...) {
     fHWFBO[draw].invalidate()
     fHWFBO[read].invalidate()
}

[bsalomon, 2015-03-17 19:21:14]

https://codereview.chromium.org/949263002/diff/280001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/949263002/diff/280001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:344: for (size_t i = 0; i <
SK_ARRAY_COUNT(fHWFBOBinding); ++i) {
On 2015/03/17 19:18:28, egdaniel wrote:
> since this is no just two, draw and read...
> 
> is it clearer to have:
> if(...) {
>      fHWFBO[draw].invalidate()
>      fHWFBO[read].invalidate()
> }


The enum would is currently just in the bind function, so it'd be [0], [1]. Not
obviously better or worse to me.

[bsalomon, 2015-03-17 19:27:12]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-03-17 19:27:13]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/949263002/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-17 19:27:48]

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

[commit-bot: I haz the power, 2015-03-17 19:33:37]

Committed patchset #16 (id:300001) as
https://skia.googlesource.com/skia/+/dc963b9264908f53650c40a97cff414101dd3e88

[bsalomon, 2015-03-17 19:46:32]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/1017803002/ by bsalomon@google.com.

The reason for reverting is: locally hit assert..

[bsalomon, 2015-03-17 22:49:20]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-03-17 22:49:20]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/949263002/#ps320001
(title: "add enum value for copy tex src")

[commit-bot: I haz the power, 2015-03-17 22:49:38]

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

[commit-bot: I haz the power, 2015-03-17 22:55:47]

Committed patchset #17 (id:320001) as
https://skia.googlesource.com/skia/+/160f24ce0e8d6dd7ca80b78871e063d4f4609cfb

[egdaniel, 2015-03-18 19:59:43]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1009563003/ by egdaniel@google.com.

The reason for reverting is: Perf regression on win8 hd7700 gpu skps.