Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1314923002:
GLProgramsTest can now randomly create a tree of GrFragmentProcessors instead of a linear pipeline (Closed)
DescriptionThere's a set probability that a linear pipeline of random procs will be created (old behavior), or a pipeline with a single proc tree (added behavior).
Had to move GrComposeEffect class definition from SkComposeShader.cpp to SkComposeShader.h so that GLProgramsTest can call GrComposeEffect::Create()
BUG=skia:4182
Committed: https://skia.googlesource.com/skia/+/059dffae800a81351c93596187099dfe09f2ba56
Patch Set 1 #
Total comments: 12
Patch Set 2 : nits by Josh #
Total comments: 6
Patch Set 3 : minlevels changed to 2 from 1 #Patch Set 4 : rebase #Patch Set 5 : removed warnings #
Depends on Patchset: Messages
Total messages: 26 (7 generated)
wangyix@google.com changed reviewers: + bsalomon@google.com, egdaniel@google.com, joshualitt@google.com, tomhudson@google.com
I'm not sure what percent of the time we want to be creating trees vs regular pipelines. Thoughts Brian? Alternatively, we could use a bool, and run GLProgramsTest or something. https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:147: bool terminate = (1 == maxLevels) || (d->fRandom->nextF() < 0.3f); we probably want to make this number and the 0.5f below named constants. https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:149: GrFragmentProcessor* fp; Can we just use SkAutoTUnref here, and then just reset it on line 151? https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:153: if (0 == fp->numChildProcessors()) { This seems like it could be a bit wasteful, even if this is just a test. Maybe we should just create compose effects directly. In the longer term, we could have a special CreateStage call that only returns effects with children. https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:167: GrFragmentProcessor* minLevelsChild = create_random_proc_tree(d, minLevels, maxLevels - 1); These can just be SkAutoTUnref
https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:153: if (0 == fp->numChildProcessors()) { On 2015/08/25 18:17:13, joshualitt wrote: > This seems like it could be a bit wasteful, even if this is just a test. Maybe > we should just create compose effects directly. In the longer term, we could > have a special CreateStage call that only returns effects with children. What do you mean? Here, I'm trying to return a random fp that's NOT a compose effect.
https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:153: if (0 == fp->numChildProcessors()) { On 2015/08/25 18:36:21, wangyix wrote: > On 2015/08/25 18:17:13, joshualitt wrote: > > This seems like it could be a bit wasteful, even if this is just a test. > Maybe > > we should just create compose effects directly. In the longer term, we could > > have a special CreateStage call that only returns effects with children. > > What do you mean? Here, I'm trying to return a random fp that's NOT a compose > effect. whoops, I probably should have read this more carefully. Could you add a comment?
https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:149: GrFragmentProcessor* fp; On 2015/08/25 18:17:13, joshualitt wrote: > Can we just use SkAutoTUnref here, and then just reset it on line 151? This fp isn't handed off to someone else within this function, so I would have to ref() it right before returning it so it doesn't immediately delete itself when this function returns due to the SkAutoTUnref going out of scope. Would that look confusing? https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:153: if (0 == fp->numChildProcessors()) { On 2015/08/25 18:42:29, joshualitt wrote: > On 2015/08/25 18:36:21, wangyix wrote: > > On 2015/08/25 18:17:13, joshualitt wrote: > > > This seems like it could be a bit wasteful, even if this is just a test. > > Maybe > > > we should just create compose effects directly. In the longer term, we > could > > > have a special CreateStage call that only returns effects with children. > > > > What do you mean? Here, I'm trying to return a random fp that's NOT a compose > > effect. > > whoops, I probably should have read this more carefully. Could you add a > comment? Acknowledged.
https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:149: GrFragmentProcessor* fp; On 2015/08/25 18:55:05, wangyix wrote: > On 2015/08/25 18:17:13, joshualitt wrote: > > Can we just use SkAutoTUnref here, and then just reset it on line 151? > > This fp isn't handed off to someone else within this function, so I would have > to ref() it right before returning it so it doesn't immediately delete itself > when this function returns due to the SkAutoTUnref going out of scope. Would > that look confusing? That makes sense, lets leave it as is
https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:147: bool terminate = (1 == maxLevels) || (d->fRandom->nextF() < 0.3f); On 2015/08/25 18:17:13, joshualitt wrote: > we probably want to make this number and the 0.5f below named constants. Done. https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:149: GrFragmentProcessor* fp; On 2015/08/25 18:56:41, joshualitt wrote: > On 2015/08/25 18:55:05, wangyix wrote: > > On 2015/08/25 18:17:13, joshualitt wrote: > > > Can we just use SkAutoTUnref here, and then just reset it on line 151? > > > > This fp isn't handed off to someone else within this function, so I would have > > to ref() it right before returning it so it doesn't immediately delete itself > > when this function returns due to the SkAutoTUnref going out of scope. Would > > that look confusing? > > That makes sense, lets leave it as is Acknowledged. https://codereview.chromium.org/1314923002/diff/1/tests/GLProgramsTest.cpp#ne... tests/GLProgramsTest.cpp:167: GrFragmentProcessor* minLevelsChild = create_random_proc_tree(d, minLevels, maxLevels - 1); On 2015/08/25 18:17:13, joshualitt wrote: > These can just be SkAutoTUnref Done.
https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... File include/core/SkComposeShader.h (right): https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... include/core/SkComposeShader.h:100: const char* name() const override {return fName.c_str(); } Teensy-tiny nit: space before return. https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cp... tests/GLProgramsTest.cpp:140: static GrFragmentProcessor* create_random_proc_tree(GrProcessorTestData* d, So what's d really doing here? Looks like it's carrying a random number generator around, and otherwise unused? The while() loop with GrProcessorTestFactory::CreateStage() is really obscure and hard to trust. https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cp... tests/GLProgramsTest.cpp:196: // processor key; maxTreeLevels should be a number from 1 to 4 inclusive. A one-level tree will just be a leaf node with no ComposeEffect, meaning we've got an effective 0.625f chance of a linear pipeline and a 0.475f chance of a tree? https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cp... tests/GLProgramsTest.cpp:200: pipelineBuilder->addColorProcessor(fp); No coverage? Or am I missing something?
https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cpp File tests/GLProgramsTest.cpp (right): https://codereview.chromium.org/1314923002/diff/20001/tests/GLProgramsTest.cp... tests/GLProgramsTest.cpp:196: // processor key; maxTreeLevels should be a number from 1 to 4 inclusive. On 2015/08/25 19:29:24, tomhudson wrote: > A one-level tree will just be a leaf node with no ComposeEffect, > meaning we've got an effective 0.625f chance of a linear pipeline and a 0.475f > chance of a tree? Good point. It may be better to have create_random_proc_tree() always return a tree with 2 or more levels. This may also address your comment about the while-loop with GrProcessorTestFactory::CreateStage() above: we can make the base case return a two-level tree by calling GrComposeEffect::TestCreate() instead of having the base case return a random non-compose processor. This lets us reuse the code within GrComposeEffect::TestCreate() that searchs for non-compose procs to use as children.
Patchset #3 (id:40001) has been deleted
patch set 3 has changes described in the previous comment.
I just realized the version of create_random_proc_tree in patchset 3 can't generate certain types of tree. Namely, it can't generate a GrComposeEffect that has a GrComposeEffect child and a non-GrComposeEffect child. This is probably not ideal.
Patchset #3 (id:60001) has been deleted
Patchset 3 has been deleted and subsequently replaced. create_random_proc_tree() remains unchanged for now. https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... File include/core/SkComposeShader.h (right): https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... include/core/SkComposeShader.h:100: const char* name() const override {return fName.c_str(); } On 2015/08/25 19:29:24, tomhudson wrote: > Teensy-tiny nit: space before return. Done.
On 2015/08/25 21:34:10, wangyix wrote: > Patchset 3 has been deleted and subsequently replaced. create_random_proc_tree() > remains unchanged for now. > > https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... > File include/core/SkComposeShader.h (right): > > https://codereview.chromium.org/1314923002/diff/20001/include/core/SkComposeS... > include/core/SkComposeShader.h:100: const char* name() const override {return > fName.c_str(); } > On 2015/08/25 19:29:24, tomhudson wrote: > > Teensy-tiny nit: space before return. > > Done. lgtm
So I added debug warnings that appear if you use CreateFromTwoProcessors() with a trivial xfer mode. In GlProgramsTests, we call CreateFromTwoProcessors() with a random xfer mode many times, which causes a spew of those warnings whenever a trivial xfer mode happens to be chosen. What's a good way to address this?
On 2015/09/09 21:39:17, wangyix wrote: > So I added debug warnings that appear if you use CreateFromTwoProcessors() with > a trivial xfer mode. In GlProgramsTests, we call CreateFromTwoProcessors() with > a random xfer mode many times, which causes a spew of those warnings whenever a > trivial xfer mode happens to be chosen. What's a good way to address this? Maybe the warnings weren't my best idea. I'm ok if you remove them.
The CQ bit was checked by wangyix@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314923002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314923002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangyix@google.com
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/1314923002/#ps120001 (title: "removed warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314923002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314923002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://skia.googlesource.com/skia/+/059dffae800a81351c93596187099dfe09f2ba56 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
