Added class AutoFragmentChildProcAdvance to be constructed before a child emitCode and destructed a…

Added class AutoFragmentChildProcAdvance to be constructed before a child emitCode and destructed after

Fixed wrong indent


Changed auto child advance back to backwards linear search for getting subset of coords and samplers array of a child


Used offset from parent instead of backwards linear search to find a child proc's coords and transforms in Auto...Advance


append mangleString to variable name in nameVariable()


BUILDS! Added AutoFragmentChildProcAdvance class; fixed a few errors from previous commits

BUG=skia:4182
Committed: https://skia.googlesource.com/skia/+/69ed114d388c56e83a2385d8b8182e6d05a0b073

[wangyix, 2015-08-12 22:02:33]

wangyix@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, joshualitt@google.com

[wangyix, 2015-08-12 22:02:34]

The AutoFragmentChildProcAdvance class was added. In its constructor, given the
index of the child you want to emit code for, it will update the manglestring,
figure out the EmitArgs to pass to that child for emitCode(), and codeappend the
opening brace and a comment containing the name of the child proc.  In its
destructor, it will update the manglestring and codeappend the closing brace.

[joshualitt, 2015-08-13 18:33:53]

nits

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:350: if (!coords.empty())
braces

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:352: if (!samplers.empty())
braces

[wangyix, 2015-08-14 13:43:19]

Patchset #3 (id:40001) has been deleted

[wangyix, 2015-08-14 13:46:16]

The EmitArgs no longer uses SkTArray for coords and samplers; instead, it takes
a pointer plus a count. This is so that when we figure out the EmitArgs to pass
to a child proc, we don't have to copy its subset of coords and samplers since
we just give it a pointer to where its coords and samplers start.

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:350: if (!coords.empty())
On 2015/08/13 18:33:53, joshualitt wrote:
> braces

Done.

https://codereview.chromium.org/1286293002/diff/1/src/gpu/gl/builders/GrGLFra...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:352: if (!samplers.empty())
On 2015/08/13 18:33:53, joshualitt wrote:
> braces

Done.

[wangyix, 2015-08-14 14:24:08]

Patchset #3 (id:60001) has been deleted

[wangyix, 2015-08-14 15:43:06]

Removed the patchset changing SkTArray to a ptr and a count.

[joshualitt, 2015-08-14 15:51:59]

looks good to me, Brian will you take a quick look to see if you agree with this
direction?

[tomhudson, 2015-08-14 15:58:47]

tomhudson@google.com changed reviewers:
+ tomhudson@google.com

[tomhudson, 2015-08-14 15:58:48]

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:372: */
This sounds really complicated - why not store the number of non-child
transforms in a builder, and then be able to do a forward search? Or is that
likely to be just as bad?

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.h:80: // This class is like
AutoStageAdvance but used for the child procs of a fragment proc.
This comment requires the reader to understand AutoStageAdvance, which isn't
even in the same source file. Try to make it self-contained? The long list of
arguments sure makes it look like something complex is going on.

[wangyix, 2015-08-14 16:07:28]

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:372: */
On 2015/08/14 15:58:48, tomhudson wrote:
> This sounds really complicated - why not store the number of non-child
> transforms in a builder, and then be able to do a forward search? Or is that
> likely to be just as bad?

I suppose we could store the number of non-child transforms as a member in
GrFragmentProcessor?

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLFragmentShaderBuilder.h (right):

https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLFragmentShaderBuilder.h:80: // This class is like
AutoStageAdvance but used for the child procs of a fragment proc.
On 2015/08/14 15:58:48, tomhudson wrote:
> This comment requires the reader to understand AutoStageAdvance, which isn't
> even in the same source file. Try to make it self-contained? The long list of
> arguments sure makes it look like something complex is going on.

Acknowledged.

[joshualitt, 2015-08-14 18:55:38]

On 2015/08/14 16:07:28, wangyix wrote:
>
https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp (right):
> 
>
https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:372: */
> On 2015/08/14 15:58:48, tomhudson wrote:
> > This sounds really complicated - why not store the number of non-child
> > transforms in a builder, and then be able to do a forward search? Or is that
> > likely to be just as bad?
> 
> I suppose we could store the number of non-child transforms as a member in
> GrFragmentProcessor?
> 
>
https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLFragmentShaderBuilder.h (right):
> 
>
https://codereview.chromium.org/1286293002/diff/20001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLFragmentShaderBuilder.h:80: // This class is like
> AutoStageAdvance but used for the child procs of a fragment proc.
> On 2015/08/14 15:58:48, tomhudson wrote:
> > This comment requires the reader to understand AutoStageAdvance, which isn't
> > even in the same source file. Try to make it self-contained? The long list
of
> > arguments sure makes it look like something complex is going on.
> 
> Acknowledged.

Lets leave the reverse linear search and the comment for now, we can always
figure out a better way to do this later.

LGTM

[wangyix, 2015-08-14 20:39:11]

The CQ bit was checked by wangyix@google.com

[wangyix, 2015-08-14 20:39:12]

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/1286293002/#ps80001
(title: "added comment explaining AutoFragmentChildProcAdvance")

[commit-bot: I haz the power, 2015-08-14 20:39:20]

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

[commit-bot: I haz the power, 2015-08-14 20:40:19]

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

[commit-bot: I haz the power, 2015-08-14 20:40:20]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[wangyix, 2015-08-14 20:46:23]

Need LGTM from Brian.

[bsalomon, 2015-08-18 14:12:46]

On 2015/08/14 20:46:23, wangyix wrote:
> Need LGTM from Brian.

lgtm

[wangyix, 2015-08-18 14:13:26]

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2015-08-18 14:13:39]

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

[commit-bot: I haz the power, 2015-08-18 14:24:32]

Committed patchset #3 (id:80001) as
https://skia.googlesource.com/skia/+/69ed114d388c56e83a2385d8b8182e6d05a0b073