All child GrFragmentProcs' transforms and textures will be stored in the root GrFragmentProc in pre…

All child GrFragmentProcs' transforms and textures will be stored in the root GrFragmentProc in preorder

Changed childProcessor(i) to return const referencd


Fixed rootProc/parentProc offset issues; renamed a few things.


added nonempty check to gatherTransforms to avoid segfault


removed recursive append_gr_coord_transforms() from GrGLProgramBuilder


BUILDS! Changed num*includeProc() calls to num() calls


added gatherCoordTransforms(). added coordTransforms() for root proc only


Modified GrFragmentProcessor to append child proc transforms and textures to root proc's arrays.

BUG=skia:4182
Committed: https://skia.googlesource.com/skia/+/58d890bd45a201477ec6c4ba0ae61c574b4e5aef

[wangyix, 2015-08-07 16:07:04]

Patchset #1 (id:1) has been deleted

[wangyix, 2015-08-07 16:13:56]

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

[wangyix, 2015-08-07 16:13:57]

I figured this was a better way to do things since it avoids the need to make
specialized versions of existing functions to use in ProgramBuilder and other
classes just to handle GrFragmentProcessors' children.  A GrFragmentProcessor
with children should be handled as if it had all its children's textures and
transforms when building the program, setting data, etc.

[tomhudson, 2015-08-10 13:15:06]

If we don't already have a skia: bug tracking this, we should have created one.
If we do have a skia bug tracking this, it should be filled in on BUG= above.

[joshualitt, 2015-08-10 15:21:48]

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

[joshualitt, 2015-08-10 15:22:13]

On 2015/08/10 13:15:06, tomhudson wrote:
> If we don't already have a skia: bug tracking this, we should have created
one.
> If we do have a skia bug tracking this, it should be filled in on BUG= above.

adding Greg and Brian, this CL looks fine to me, but other people should
definitely take a look

[wangyix, 2015-08-12 13:48:38]

Patchset #2 (id:40001) has been deleted

[wangyix, 2015-08-12 13:52:54]

Patchset #2 (id:60001) has been deleted

[wangyix, 2015-08-12 14:37:42]

Patchset #2 (id:80001) has been deleted

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

GrFragmentProcessor's children procs are now stored in a GrFragmentStage array.
The textures and transforms getters cannot be called on child procs since they
no longer have a parent to go to to retrieve their textures and transforms.

[joshualitt, 2015-08-12 14:49:52]

Feedback for Brian, most of this looks fine to me.

https://codereview.chromium.org/1275853005/diff/100001/include/gpu/GrFragment...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1275853005/diff/100001/include/gpu/GrFragment...
include/gpu/GrFragmentProcessor.h:183: SkDEBUGCODE(bool fIsChild;)
I think this has the same problem as the fIsParent

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cpp
File src/gpu/GrProcessor.cpp (right):

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cp...
src/gpu/GrProcessor.cpp:144: child->fCoordTransforms.reset();
I think we don't want to reset the child's data because the child may be used
elsewhere.  I think registerChildProcessor might have to take a const
GrFragmentProcessor*.

However, double storing the pointers in both the child and the parent doesn't
seem like a big deal to me because we shouldn't have very many coord transforms
or textureaccesses.  Thoughts Brian?

[wangyix, 2015-08-12 15:04:47]

Patchset #3 (id:120001) has been deleted

[wangyix, 2015-08-12 15:09:24]

https://codereview.chromium.org/1275853005/diff/100001/include/gpu/GrFragment...
File include/gpu/GrFragmentProcessor.h (right):

https://codereview.chromium.org/1275853005/diff/100001/include/gpu/GrFragment...
include/gpu/GrFragmentProcessor.h:183: SkDEBUGCODE(bool fIsChild;)
On 2015/08/12 14:49:52, joshualitt wrote:
> I think this has the same problem as the fIsParent

Ok. This variable is no longer necessary if the children keep a copy of their
textures and transforms since that'll allow us to call numTextures(),
numTransforms(), etc on child procs, so no SkASSERT(!fIsChild) will be necessary
anywhere.

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cpp
File src/gpu/GrProcessor.cpp (right):

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cp...
src/gpu/GrProcessor.cpp:144: child->fCoordTransforms.reset();
On 2015/08/12 14:49:52, joshualitt wrote:
> I think we don't want to reset the child's data because the child may be used
> elsewhere.  I think registerChildProcessor might have to take a const
> GrFragmentProcessor*.
> 
> However, double storing the pointers in both the child and the parent doesn't
> seem like a big deal to me because we shouldn't have very many coord
transforms
> or textureaccesses.  Thoughts Brian?

Done.

[bsalomon, 2015-08-12 15:14:22]

Let's make sure when we get to the point of being able to test this that we have
a test case that has a double-parented child.

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cpp
File src/gpu/GrProcessor.cpp (right):

https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cp...
src/gpu/GrProcessor.cpp:144: child->fCoordTransforms.reset();
On 2015/08/12 15:09:24, wangyix wrote:
> On 2015/08/12 14:49:52, joshualitt wrote:
> > I think we don't want to reset the child's data because the child may be
used
> > elsewhere.  I think registerChildProcessor might have to take a const
> > GrFragmentProcessor*.
> > 
> > However, double storing the pointers in both the child and the parent
doesn't
> > seem like a big deal to me because we shouldn't have very many coord
> transforms
> > or textureaccesses.  Thoughts Brian?
> 
> Done.

sgtm

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

On 2015/08/12 15:14:22, bsalomon wrote:
> Let's make sure when we get to the point of being able to test this that we
have
> a test case that has a double-parented child.
> 
> https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cpp
> File src/gpu/GrProcessor.cpp (right):
> 
>
https://codereview.chromium.org/1275853005/diff/100001/src/gpu/GrProcessor.cp...
> src/gpu/GrProcessor.cpp:144: child->fCoordTransforms.reset();
> On 2015/08/12 15:09:24, wangyix wrote:
> > On 2015/08/12 14:49:52, joshualitt wrote:
> > > I think we don't want to reset the child's data because the child may be
> used
> > > elsewhere.  I think registerChildProcessor might have to take a const
> > > GrFragmentProcessor*.
> > > 
> > > However, double storing the pointers in both the child and the parent
> doesn't
> > > seem like a big deal to me because we shouldn't have very many coord
> > transforms
> > > or textureaccesses.  Thoughts Brian?
> > 
> > Done.
> 
> sgtm

lgtm

[wangyix, 2015-08-12 16:19:15]

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2015-08-12 16:19:21]

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

[commit-bot: I haz the power, 2015-08-12 16:20:22]

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

[commit-bot: I haz the power, 2015-08-12 16:20:22]

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-12 16:30:14]

On 2015/08/12 16:20:22, commit-bot: I haz the power wrote:
> 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...)

Does Brian need to LGTM this?

[bsalomon, 2015-08-12 16:39:54]

On 2015/08/12 16:30:14, wangyix wrote:
> On 2015/08/12 16:20:22, commit-bot: I haz the power wrote:
> > 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...)
> 
> Does Brian need to LGTM this?

lgtm

[wangyix, 2015-08-12 16:40:17]

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2015-08-12 16:40:24]

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

[commit-bot: I haz the power, 2015-08-12 16:40:50]

Committed patchset #3 (id:140001) as
https://skia.googlesource.com/skia/+/58d890bd45a201477ec6c4ba0ae61c574b4e5aef