Add option to draw wireframe batch bounds

Add option to draw wireframe batch bounds

Committed: https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960

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

[bsalomon, 2015-12-02 23:51:47]

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

[joshualitt, 2015-12-03 14:43:52]

requests for comments

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (left):

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3083: 
I kind of like this space here, but ymmv

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

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3108: void GrGLGpu::createWireRectProgram() {
I wonder if we want to create a 'createBackendProgram' abstraction which can be
used by this and the copy shader stuff.  Alternatively, wrap this logic in a
class of some kind.  Maybe its not really possible, and its outside the scope of
this CL, but if we end up doing this a third time we might want to think about
it.

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3120: vshaderTxt.append(
A comment on this algorithm would be nice.

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3195: void GrGLGpu::drawDebugWireRect(GrRenderTarget* rt,
const SkIRect& rect, GrColor color) {
Similar to the above, a 'drawBackend' abstraction

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3204: GrGLfloat verts[4];
Could you comment exactly what you are doing here?  I think I get the general
idea, but a sentence or two would help.

[bsalomon, 2015-12-03 16:25:32]

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (left):

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3083: 
On 2015/12/03 14:43:52, joshualitt wrote:
> I kind of like this space here, but ymmv

Done.

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

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3108: void GrGLGpu::createWireRectProgram() {
On 2015/12/03 14:43:52, joshualitt wrote:
> I wonder if we want to create a 'createBackendProgram' abstraction which can
be
> used by this and the copy shader stuff.  Alternatively, wrap this logic in a
> class of some kind.  Maybe its not really possible, and its outside the scope
of
> this CL, but if we end up doing this a third time we might want to think about
> it.

I'd prefer to wait on that. I think the !/$ is not particularly high. AFAICT
most of what would get eliminated from any particular program setup is appending
declarations and default precision to the code, combining shaders into a
program, and binding unis/attrs. With only two users I suspect a net increase in
code and another abstraction to understand. If we wind up creating a bunch of
these I can see the value.

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3120: vshaderTxt.append(
On 2015/12/03 14:43:52, joshualitt wrote:
> A comment on this algorithm would be nice.

Done.

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3195: void GrGLGpu::drawDebugWireRect(GrRenderTarget* rt,
const SkIRect& rect, GrColor color) {
On 2015/12/03 14:43:52, joshualitt wrote:
> Similar to the above, a 'drawBackend' abstraction

I'm not sure what there is to abstract... this calculates and sets uniforms that
are particular to this program, sets the vertex buffer that is particular to
this program, sets fixed function state that is particular to this program, and
issues a draw call that is particular to this program.

[joshualitt, 2015-12-03 16:30:15]

lgtm

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

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3108: void GrGLGpu::createWireRectProgram() {
On 2015/12/03 16:25:31, bsalomon wrote:
> On 2015/12/03 14:43:52, joshualitt wrote:
> > I wonder if we want to create a 'createBackendProgram' abstraction which can
> be
> > used by this and the copy shader stuff.  Alternatively, wrap this logic in a
> > class of some kind.  Maybe its not really possible, and its outside the
scope
> of
> > this CL, but if we end up doing this a third time we might want to think
about
> > it.
> 
> I'd prefer to wait on that. I think the !/$ is not particularly high. AFAICT
> most of what would get eliminated from any particular program setup is
appending
> declarations and default precision to the code, combining shaders into a
> program, and binding unis/attrs. With only two users I suspect a net increase
in
> code and another abstraction to understand. If we wind up creating a bunch of
> these I can see the value.

agreed.

https://codereview.chromium.org/1494473005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:3195: void GrGLGpu::drawDebugWireRect(GrRenderTarget* rt,
const SkIRect& rect, GrColor color) {
On 2015/12/03 16:25:31, bsalomon wrote:
> On 2015/12/03 14:43:52, joshualitt wrote:
> > Similar to the above, a 'drawBackend' abstraction
> 
> I'm not sure what there is to abstract... this calculates and sets uniforms
that
> are particular to this program, sets the vertex buffer that is particular to
> this program, sets fixed function state that is particular to this program,
and
> issues a draw call that is particular to this program.

Yea, I mean, kind of a backend GrBatch / GrGeometryProcessor type system.  Ie,
you provide some code for the shaders, some code for tesselation / filling out
vertex attributes, and then the system handles all of the nitty gritty.  It
would obviously be stripped down from what we use at the drawtarget level. 
Maybe this isn't possible.  I definitely think we should see if we do this
again, and if so we should think a little more about it.

[bsalomon, 2015-12-03 16:54:57]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-12-03 16:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1494473005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1494473005/80001

[commit-bot: I haz the power, 2015-12-03 16:55:47]

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

[commit-bot: I haz the power, 2015-12-03 16:55:47]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[bsalomon, 2015-12-03 17:14:39]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-12-03 17:14:40]

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

[commit-bot: I haz the power, 2015-12-03 17:14:48]

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

[commit-bot: I haz the power, 2015-12-03 17:36:51]

Description was changed from

==========
Add option to draw wireframe batch bounds
==========

to

==========
Add option to draw wireframe batch bounds

Committed:
https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960
==========

[commit-bot: I haz the power, 2015-12-03 17:36:51]

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

[scroggo, 2015-12-03 18:30:19]

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

The reason for reverting is: Looks to be responsible for breaking builds e.g.
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Mac10.9-Clan....

[joshualitt, 2015-12-03 19:00:04]

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

https://codereview.chromium.org/1494473005/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:236: fWireRectProgram.fProgram = 0;
I think we need to init fWireRectArrayBuffer here to 0

[bsalomon, 2015-12-03 19:50:00]

Description was changed from

==========
Add option to draw wireframe batch bounds

Committed:
https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960
==========

to

==========
Add option to draw wireframe batch bounds

Committed:
https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960
==========

[bsalomon, 2015-12-03 19:50:04]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-12-03 19:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com
Link to the patchset: https://codereview.chromium.org/1494473005/#ps120001
(title: "init vbo id to 0")

[commit-bot: I haz the power, 2015-12-03 19:50:07]

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

[commit-bot: I haz the power, 2015-12-03 20:58:08]

Description was changed from

==========
Add option to draw wireframe batch bounds

Committed:
https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960
==========

to

==========
Add option to draw wireframe batch bounds

Committed:
https://skia.googlesource.com/skia/+/26489ef21ff5df33b8cb5943fddfd4604e203960

Committed:
https://skia.googlesource.com/skia/+/6dea83f244cfdea52901eef6b31cee60b07a8ea0
==========

[commit-bot: I haz the power, 2015-12-03 20:58:09]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/6dea83f244cfdea52901eef6b31cee60b07a8ea0