Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
204543006:
New option for arithmetic mode (Closed)
|
Created:
6 years, 9 months ago by sugoi1 Modified:
6 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionArithmetic mode now has the option of not validating the output color, which will allow multiple arithmetic operations to be done sequentially, without intermediate clamping. This is required for mimicking blink's current behavior.
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=14031
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed SkValidateImageFilter #
Total comments: 3
Patch Set 3 : Added conditional check instead of suppressing it #
Total comments: 1
Patch Set 4 : Added GenKey() #
Total comments: 5
Patch Set 5 : Added background texture check #Messages
Total messages: 19 (0 generated)
As discussed, we may not need the separate validating image filter after all. Let's try it out in Blink just using the new bool at DAG build time. https://codereview.chromium.org/204543006/diff/1/include/effects/SkValidateIm... File include/effects/SkValidateImageFilter.h (right): https://codereview.chromium.org/204543006/diff/1/include/effects/SkValidateIm... include/effects/SkValidateImageFilter.h:8: #ifndef SkValidateImageFilter_DEFINED Bikeshed: the name doesn't really explain what it's validating. Some ideas: SkValidatePremulImageFilter SkValidatePMColorImageFilter SkForceValidPMColorImageFilter SkForceValidPMImageFilter SkForceValidPremulImageFilter I think I like the last one the best, but I'd like to hear Mike's opinion too. https://codereview.chromium.org/204543006/diff/1/src/effects/SkArithmeticMode... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/1/src/effects/SkArithmeticMode... src/effects/SkArithmeticMode.cpp:54: fValidatePMColor = buffer.readBool(); As discussed, we might have to support older versions of the picture format. I don't *think* so, since the only place Blink serializes an SkArithmeticMode is when sending it to the browser process in SVG-on-HTML accelerated filters. I don't think any of the SKP's we have currently could have an SkArithmeticMode in them. https://codereview.chromium.org/204543006/diff/1/src/effects/SkValidateImageF... File src/effects/SkValidateImageFilter.cpp (right): https://codereview.chromium.org/204543006/diff/1/src/effects/SkValidateImageF... src/effects/SkValidateImageFilter.cpp:2: * Copyright 2013 Google Inc. Nit: 2014. https://codereview.chromium.org/204543006/diff/1/src/effects/SkValidateImageF... src/effects/SkValidateImageFilter.cpp:21: void validatePMColor(const SkBitmap* src, SkBitmap* dst, const SkIRect& bounds) { Nit: "src" should be a const ref, according to skia style. https://codereview.chromium.org/204543006/diff/1/src/effects/SkValidateImageF... src/effects/SkValidateImageFilter.cpp:120: static GrEffectRef* Create(GrTexture* color) { Same here. https://codereview.chromium.org/204543006/diff/1/src/effects/SkValidateImageF... src/effects/SkValidateImageFilter.cpp:161: GrTexture* color = source.getTexture(); Nit: maybe call this sourceTexture instead.
https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... src/effects/SkArithmeticMode.cpp:178: dst[i] = SkPackARGB32NoCheck(a, r, g, b); Is this ok or should I still call SkPackARGB32() when we have fValidatePMColor set to true?
https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... src/effects/SkArithmeticMode.cpp:178: dst[i] = SkPackARGB32NoCheck(a, r, g, b); On 2014/03/27 19:07:36, sugoi1 wrote: > Is this ok or should I still call SkPackARGB32() when we have fValidatePMColor > set to true? Hmm, that's a good point. I guess just to make sure nobody removes the SkMin32() lines above, doing SkPackARGB32() conditionally on the flag makes sense.
+reed
Ping. https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/80001/src/effects/SkArithmetic... src/effects/SkArithmeticMode.cpp:178: dst[i] = SkPackARGB32NoCheck(a, r, g, b); On 2014/03/27 19:38:16, Stephen White wrote: > On 2014/03/27 19:07:36, sugoi1 wrote: > > Is this ok or should I still call SkPackARGB32() when we have fValidatePMColor > > set to true? > > Hmm, that's a good point. I guess just to make sure nobody removes the SkMin32() > lines above, doing SkPackARGB32() conditionally on the flag makes sense. Done.
LGTM. Mike?
+bsalomon Brian, would you take a look?
"validate" seems like the wrong verb, "enforce"? https://codereview.chromium.org/204543006/diff/90001/src/effects/SkArithmetic... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/90001/src/effects/SkArithmetic... src/effects/SkArithmeticMode.cpp:237: class GrGLArithmeticEffect : public GrGLEffect { This class needs a GenKey function since it generates different code based on whether there is a background texture and now whether it ensures that the color is PM.
On 2014/04/01 20:18:26, bsalomon wrote: > "validate" seems like the wrong verb, "enforce"? > > https://codereview.chromium.org/204543006/diff/90001/src/effects/SkArithmetic... > File src/effects/SkArithmeticMode.cpp (right): > > https://codereview.chromium.org/204543006/diff/90001/src/effects/SkArithmetic... > src/effects/SkArithmeticMode.cpp:237: class GrGLArithmeticEffect : public > GrGLEffect { > This class needs a GenKey function since it generates different code based on > whether there is a background texture and now whether it ensures that the color > is PM. Done
https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:407: const GrArithmeticEffect& arith = drawEffect.castEffect<GrArithmeticEffect>(); I think we also need to key off whether there is a background texture or not. I just noticed that XferEffect doesn't do that either. I can prepare a separate CL for that.
https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:407: const GrArithmeticEffect& arith = drawEffect.castEffect<GrArithmeticEffect>(); On 2014/04/02 15:41:16, bsalomon wrote: > I think we also need to key off whether there is a background texture or not. I just noticed that XferEffect doesn't do that either. I can prepare a separate CL for that. Ok, but the presence of a background texture doesn't affect emitCode() in this file, right? If so, where is this affecting the shader code and couldn't we modify the key in that file? Just curious.
https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:361: if (backgroundTex) { It affects the code right here.
https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:361: if (backgroundTex) { On 2014/04/02 17:38:28, bsalomon wrote: > It affects the code right here. Oh, hadn't seen that. Will fix. https://codereview.chromium.org/204543006/diff/110001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:407: const GrArithmeticEffect& arith = drawEffect.castEffect<GrArithmeticEffect>(); On 2014/04/02 16:45:59, sugoi1 wrote: > On 2014/04/02 15:41:16, bsalomon wrote: > > I think we also need to key off whether there is a background texture or not. > I just noticed that XferEffect doesn't do that either. I can prepare a separate > CL for that. > > Ok, but the presence of a background texture doesn't affect emitCode() in this > file, right? If so, where is this affecting the shader code and couldn't we > modify the key in that file? Just curious. Done.
On 2014/04/01 20:18:26, bsalomon wrote: > This class needs a GenKey function since it generates different code based on > whether there is a background texture and now whether it ensures that the color > is PM. Good catch, BTW, Brian. Thanks!
lgtm
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/204543006/130001
Message was sent while issue was closed.
Change committed as 14031 |
