Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1292353005:
Added SkComposeShader GPU implementation (Closed)
DescriptionAdded SkComposeShader GPU implementation
moved onCreateGLInstance() to private in GrComposeEffect
Added SkComposeShader gpu implementation; composeshader gm is unchanged
BUG=skia:4182
TBR=bsalomon@google.com
Committed: https://skia.googlesource.com/skia/+/73fa61670d95c52250a660a2cec618ab77716934
Patch Set 1 #
Total comments: 6
Patch Set 2 : updated SkComposeShader to use emitChild() #Patch Set 3 : nits by Tom #Patch Set 4 : asFragmentProcessor only accepts coefficient modes #Patch Set 5 : handle null mode in asFragmentProcessor #Patch Set 6 : asFragmentProcessor returns false if xfermode is kClear_Mode #
Total comments: 4
Patch Set 7 : added paint alpha un-premul/remul to emitted code #Patch Set 8 : removed children's names from name of GrComposeEffect #
Total comments: 10
Patch Set 9 : rebase, mode optimizations, nits #
Total comments: 4
Patch Set 10 : Brian's nits #
Total comments: 6
Patch Set 11 : Josh's nits #Messages
Total messages: 44 (13 generated)
wangyix@google.com changed reviewers: + bsalomon@google.com, egdaniel@google.com, joshualitt@google.com
This is what I have currently for SkComposeShader; this version actually assumes that the GL instances have a tree structure, so it assumes the existence of GrGLFragmentProcessor::childProcessor(int index).
wangyix@google.com changed reviewers: + tomhudson@google.com
I've updated SkComposeShader to use the emitChild() function added in https://codereview.chromium.org/1301523003
lgtm https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cpp File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:260: //GR_DEFINE_FRAGMENT_PROCESSOR_TEST(GrComposeShader); Remove commented-out code before committing. https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:278: void GrComposeEffect::onGetGLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const{ nit: space https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:338: GrGLBlend::AppendPorterDuffBlend(fsBuilder, mangledOutputColorB.c_str(), mangledOutputColorA.c_str(), nit: line length?
https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cpp File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:260: //GR_DEFINE_FRAGMENT_PROCESSOR_TEST(GrComposeShader); On 2015/08/18 14:58:38, tomhudson wrote: > Remove commented-out code before committing. Done. https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:278: void GrComposeEffect::onGetGLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const{ On 2015/08/18 14:58:38, tomhudson wrote: > nit: space Done. https://codereview.chromium.org/1292353005/diff/1/src/core/SkComposeShader.cp... src/core/SkComposeShader.cpp:338: GrGLBlend::AppendPorterDuffBlend(fsBuilder, mangledOutputColorB.c_str(), mangledOutputColorA.c_str(), On 2015/08/18 14:58:38, tomhudson wrote: > nit: line length? Done.
asFragmentProcessor() now only returns true for SkXfermodes that are coefficient modes, since the function GrGLBlend::AppendPorterDuffBlend(), which is used to generate the blend code in the shader, only accepts coefficient modes
asFragmentProcessor() now returns false for xfermode kClear_Mode, since GrGLBlend::AppendPorterDuffBlend() fails an assert if you give it kClear_Mode.
On 2015/08/21 14:39:51, wangyix wrote:
> asFragmentProcessor() now returns false for xfermode kClear_Mode, since
> GrGLBlend::AppendPorterDuffBlend() fails an assert if you give it kClear_Mode.
I think AppendPorterDufferBlend() should be fixed to have something like.
if (clear == mode) {
output = "0";
}
I think most other places that call AppendPorterDuff fall out of the blending
ahead of time with clear so we never hit this assert. So I'm not against you
shortcutting clear as well in your functions (assuming its what you actually
want to do), but since AppendPDB is a general utility function we want to make
sure it works for all cases.
Hey so I think you should hold of on landing this at least for today. After some discussion, we are thinking that the asFragmentProcessor virtual signature may need to be changed/updated. A concrete example of an issue in this patch is that asFP can return true, not return a pointer, but can set the passed in paintColor. It would then be assumed that the draw continues without adding an FP but uses the new color. I'm not sure if your compose asFP handles all these corner cases. Anyways we think recent changes can alleviate all these complexities in asFP. So lets hold off on actually landing this patch to see what we can do. https://codereview.chromium.org/1292353005/diff/100001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/100001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:317: || mode == SkXfermode::Mode::kClear_Mode) { nit: we put the variable on the right side of == https://codereview.chromium.org/1292353005/diff/100001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:323: procDataManager, &fpA) || !fpA) { Can you line this up so that procDataMangeger, &fpA is aligned with the other params in the function call. https://codereview.chromium.org/1292353005/diff/100001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:327: if (!fShaderB->asFragmentProcessor(context, paint, viewM, localMatrix, paintColor, same as above https://codereview.chromium.org/1292353005/diff/100001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:333: *fp = GrComposeEffect::Create(fpA, fpB, mode); Is there any chance that this Create can fail and return null? If we don't plan on this ever failing in the future maybe we should just go with a ctor. Otherwise we need to check here that it is not null
On 2015/08/21 16:21:39, egdaniel wrote: > Hey so I think you should hold of on landing this at least for today. After some > discussion, we are thinking that the asFragmentProcessor virtual signature may > need to be changed/updated. A concrete example of an issue in this patch is that > asFP can return true, not return a pointer, but can set the passed in > paintColor. It would then be assumed that the draw continues without adding an > FP but uses the new color. I'm not sure if your compose asFP handles all these > corner cases. > > Anyways we think recent changes can alleviate all these complexities in asFP. So > lets hold off on actually landing this patch to see what we can do. > What he said.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #8 (id:200001) has been deleted
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fName.printf("ComposeShader (Mode: %s)", SkXfermode::ModeName(fMode)); Maybe we should init this in name()? I believe it is only called when building the shader.
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:353: #else Let's just put the function decl in the header behind a #if SK_SUPPORT_GPU. (I got rid of all these bogus impls in the the refactor of asFragmentProcessor)
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fName.printf("ComposeShader (Mode: %s)", SkXfermode::ModeName(fMode)); On 2015/08/27 20:12:24, bsalomon wrote: > Maybe we should init this in name()? I believe it is only called when building > the shader. I'm not sure what you mean by this. On another note, it seems all other shaders' name() function just returns the name of the shader and nothing more, so maybe I could change name() to just 'return "ComposeShader";' for consistency? https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:328: || mode == SkXfermode::Mode::kClear_Mode) { What should the behavior be for kClear_Mode under the new asFragmentProcessor() signature? Should NULL be returned for asFragmentProcessor()? Also, if the mode is kSrc_Mode or kDst_Mode, should I just return the src or dst child's fragment processor directly instead of a GrComposeEffect?
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:328: || mode == SkXfermode::Mode::kClear_Mode) { On 2015/08/31 14:10:05, wangyix wrote: > What should the behavior be for kClear_Mode under the new asFragmentProcessor() > signature? Should NULL be returned for asFragmentProcessor()? > > Also, if the mode is kSrc_Mode or kDst_Mode, should I just return the src or dst > child's fragment processor directly instead of a GrComposeEffect? Did AppendPorterDuffBlend() ever get fixed to support clear? That definitely should be done first. As for optimizations that can be done here, I don't think I would return NULL for the FP here on clear. If you have a clear Xfermode then what you will end up with is all trans black for each pixel. I don't think returning NULL would give you the same output? Maybe a const color processor that does trans black? The src and dst could also be optimized as you said. However personally I say lets make sure it works without any of these specific mode optimizations first then add those in.
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fName.printf("ComposeShader (Mode: %s)", SkXfermode::ModeName(fMode)); On 2015/08/31 14:10:05, wangyix wrote: > On 2015/08/27 20:12:24, bsalomon wrote: > > Maybe we should init this in name()? I believe it is only called when building > > the shader. > > I'm not sure what you mean by this. On another note, it seems all other > shaders' name() function just returns the name of the shader and nothing more, > so maybe I could change name() to just 'return "ComposeShader";' for > consistency? Let's go with that. I just don't want the constructor to pay for printf() when the name string is only really used for shader debugging. https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:328: || mode == SkXfermode::Mode::kClear_Mode) { On 2015/08/31 14:43:05, egdaniel wrote: > On 2015/08/31 14:10:05, wangyix wrote: > > What should the behavior be for kClear_Mode under the new > asFragmentProcessor() > > signature? Should NULL be returned for asFragmentProcessor()? > > > > Also, if the mode is kSrc_Mode or kDst_Mode, should I just return the src or > dst > > child's fragment processor directly instead of a GrComposeEffect? > > Did AppendPorterDuffBlend() ever get fixed to support clear? That definitely > should be done first. > > As for optimizations that can be done here, I don't think I would return NULL > for the FP here on clear. If you have a clear Xfermode then what you will end up > with is all trans black for each pixel. I don't think returning NULL would give > you the same output? Maybe a const color processor that does trans black? > > The src and dst could also be optimized as you said. However personally I say > lets make sure it works without any of these specific mode optimizations first > then add those in. Hm... I think making kSrc, kDst, kClear FPs is maybe a bad idea that should always fail. If we're doing that we can probably use a simpler shader that will result in less duplication. (e.g. use a const color fp for clear). It might be better to make it assert in the append so that the author sees the issue and writes the correct optimization. The clear optimization should probably just happen in the ComposeEffect's factory function, since every caller (when this is moved out of this cpp file) can benefit from it. The kSrc and kDst optimization probably should happen in this asFP function since we can skip making either the dst or src FP entirely.
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:328: || mode == SkXfermode::Mode::kClear_Mode) { On 2015/08/31 14:48:35, bsalomon wrote: > On 2015/08/31 14:43:05, egdaniel wrote: > > On 2015/08/31 14:10:05, wangyix wrote: > > > What should the behavior be for kClear_Mode under the new > > asFragmentProcessor() > > > signature? Should NULL be returned for asFragmentProcessor()? > > > > > > Also, if the mode is kSrc_Mode or kDst_Mode, should I just return the src or > > dst > > > child's fragment processor directly instead of a GrComposeEffect? > > > > Did AppendPorterDuffBlend() ever get fixed to support clear? That definitely > > should be done first. > > > > As for optimizations that can be done here, I don't think I would return NULL > > for the FP here on clear. If you have a clear Xfermode then what you will end > up > > with is all trans black for each pixel. I don't think returning NULL would > give > > you the same output? Maybe a const color processor that does trans black? > > > > The src and dst could also be optimized as you said. However personally I say > > lets make sure it works without any of these specific mode optimizations first > > then add those in. > > > Hm... I think making kSrc, kDst, kClear FPs is maybe a bad idea that should > always fail. If we're doing that we can probably use a simpler shader that will > result in less duplication. (e.g. use a const color fp for clear). It might be > better to make it assert in the append so that the author sees the issue and > writes the correct optimization. > > The clear optimization should probably just happen in the ComposeEffect's > factory function, since every caller (when this is moved out of this cpp file) > can benefit from it. The kSrc and kDst optimization probably should happen in > this asFP function since we can skip making either the dst or src FP entirely. If the clear optimization happens in GrComposeEffect::Create(), then we can't skip making the src and dst FPs. I also feel that SkComposeShader shouldn't support kClear, kSrc, kDst in the first place. Maybe add warnings or asserts for those 3 cases, but still have code that optimizes for them in SkComposeShader::asFragmentProcessor() so those modes still work in release mode?
On 2015/08/31 15:59:24, wangyix wrote: > https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... > File src/core/SkComposeShader.cpp (right): > > https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... > src/core/SkComposeShader.cpp:328: || mode == SkXfermode::Mode::kClear_Mode) { > On 2015/08/31 14:48:35, bsalomon wrote: > > On 2015/08/31 14:43:05, egdaniel wrote: > > > On 2015/08/31 14:10:05, wangyix wrote: > > > > What should the behavior be for kClear_Mode under the new > > > asFragmentProcessor() > > > > signature? Should NULL be returned for asFragmentProcessor()? > > > > > > > > Also, if the mode is kSrc_Mode or kDst_Mode, should I just return the src > or > > > dst > > > > child's fragment processor directly instead of a GrComposeEffect? > > > > > > Did AppendPorterDuffBlend() ever get fixed to support clear? That definitely > > > should be done first. > > > > > > As for optimizations that can be done here, I don't think I would return > NULL > > > for the FP here on clear. If you have a clear Xfermode then what you will > end > > up > > > with is all trans black for each pixel. I don't think returning NULL would > > give > > > you the same output? Maybe a const color processor that does trans black? > > > > > > The src and dst could also be optimized as you said. However personally I > say > > > lets make sure it works without any of these specific mode optimizations > first > > > then add those in. > > > > > > Hm... I think making kSrc, kDst, kClear FPs is maybe a bad idea that should > > always fail. If we're doing that we can probably use a simpler shader that > will > > result in less duplication. (e.g. use a const color fp for clear). It might be > > better to make it assert in the append so that the author sees the issue and > > writes the correct optimization. > > > > The clear optimization should probably just happen in the ComposeEffect's > > factory function, since every caller (when this is moved out of this cpp file) > > can benefit from it. The kSrc and kDst optimization probably should happen in > > this asFP function since we can skip making either the dst or src FP entirely. > > If the clear optimization happens in GrComposeEffect::Create(), then we can't > skip making the src and dst FPs. > > I also feel that SkComposeShader shouldn't support kClear, kSrc, kDst in the > first place. Maybe add warnings or asserts for those 3 cases, but still have > code that optimizes for them in SkComposeShader::asFragmentProcessor() so those > modes still work in release mode? I agree... why don't we make SkComposeShader's factory return shadera, shaderb, or SkColorShader when passed kDst, kSrc, kClear? Maybe it already does...
On 2015/08/31 16:04:40, bsalomon wrote: > I agree... why don't we make SkComposeShader's factory return shadera, shaderb, > or SkColorShader when passed kDst, kSrc, kClear? Maybe it already does... SkComposeShader currently does not have a factory. In SkDraw.cpp, an SkComposeShader is created, and SkComposeShader::getShaderA() is called on it. If we did have a factory for SkComposeShader, then we can't guarantee that it returns an SkComposeShader instance, which means we can't call getShaderA() on it in SkDraw.cpp. I think I'll keep the optimizations in SkComposeShader::asFragmentProcessor() for now and add asserts() for those three cases.
https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fName.printf("ComposeShader (Mode: %s)", SkXfermode::ModeName(fMode)); On 2015/08/31 14:48:35, bsalomon wrote: > On 2015/08/31 14:10:05, wangyix wrote: > > On 2015/08/27 20:12:24, bsalomon wrote: > > > Maybe we should init this in name()? I believe it is only called when > building > > > the shader. > > > > I'm not sure what you mean by this. On another note, it seems all other > > shaders' name() function just returns the name of the shader and nothing more, > > so maybe I could change name() to just 'return "ComposeShader";' for > > consistency? > > Let's go with that. I just don't want the constructor to pay for printf() when > the name string is only really used for shader debugging. Done. https://codereview.chromium.org/1292353005/diff/220001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:353: #else On 2015/08/28 16:15:30, bsalomon wrote: > Let's just put the function decl in the header behind a #if SK_SUPPORT_GPU. (I > got rid of all these bogus impls in the the refactor of asFragmentProcessor) Done.
https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fShaderAChildIndex = this->registerChildProcessor(fpA); Maybe just assert that indices are 0 and 1 rather than store them? https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:257: int fShaderAChildIndex; Does this guy really need any members? emitCode has access to the GrComposeEffect that generated the GrGLComposeEffect and can read these values off it.
https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:230: fShaderAChildIndex = this->registerChildProcessor(fpA); On 2015/08/31 18:51:37, bsalomon wrote: > Maybe just assert that indices are 0 and 1 rather than store them? Done. https://codereview.chromium.org/1292353005/diff/240001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:257: int fShaderAChildIndex; On 2015/08/31 18:51:37, bsalomon wrote: > Does this guy really need any members? emitCode has access to the > GrComposeEffect that generated the GrGLComposeEffect and can read these values > off it. Done.
lgtm with some nits. https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:283: fsBuilder->codeAppendf("float %s = %s.a;\n", inputAlpha.c_str(), args.fInputColor); no reason for newlines in these strings. https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:333: viewM, localMatrix, fq, procDataManager)); alignment on this and the next one
https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:283: fsBuilder->codeAppendf("float %s = %s.a;\n", inputAlpha.c_str(), args.fInputColor); On 2015/09/01 15:27:18, joshualitt wrote: > no reason for newlines in these strings. Removeing \n from all the codeAppends causes shader program to fail compilation, specifically at GrGLShaderStringBuilder.cpp line 77.
https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:295: fsBuilder->codeAppendf("// Compose Xfer Mode: %s\n", SkXfermode::ModeName(mode)); This should be the only newline you actually need. Explicit newlines at the end of comments are required by GLSL and our pretty printer has no way to generate this automatically.
Patchset #11 (id:280001) has been deleted
https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... File src/core/SkComposeShader.cpp (right): https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:295: fsBuilder->codeAppendf("// Compose Xfer Mode: %s\n", SkXfermode::ModeName(mode)); On 2015/09/01 15:52:57, joshualitt wrote: > This should be the only newline you actually need. Explicit newlines at the end > of comments are required by GLSL and our pretty printer has no way to generate > this automatically. Done. https://codereview.chromium.org/1292353005/diff/260001/src/core/SkComposeShad... src/core/SkComposeShader.cpp:333: viewM, localMatrix, fq, procDataManager)); On 2015/09/01 15:27:18, joshualitt wrote: > alignment on this and the next one Done.
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/1292353005/220002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292353005/220002
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 tomhudson@google.com, joshualitt@google.com Link to the patchset: https://codereview.chromium.org/1292353005/#ps220002 (title: "Josh's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292353005/220002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292353005/220002
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by wangyix@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292353005/220002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292353005/220002
Message was sent while issue was closed.
Committed patchset #11 (id:220002) as https://skia.googlesource.com/skia/+/73fa61670d95c52250a660a2cec618ab77716934 |
