Issue 6354065: Add a zoom filter to Skia. This will be used on ChromeOS to implement the screen magnifier. (Closed)
|
Created:
12 years, 5 months ago by zork Modified:
12 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionAdd a zoom filter to Skia. This will be used on ChromeOS to implement the screen magnifier.
BUG=skia:689
TEST=Run gm
Patch Set 1 #Patch Set 2 : Remove an extra file #
Total comments: 32
Patch Set 3 : Code review fixes #
Total comments: 2
Patch Set 4 : Code review fixes #
Total comments: 38
Patch Set 5 : Code review fixes #
Total comments: 4
Patch Set 6 : Code review fixes #Patch Set 7 : Code review fix #
Total comments: 3
MessagesTotal messages: 28
Hello Stephen, Could you review this at: https://codereview.appspot.com/6354065 Thanks, -Zach
Sign in to reply to this message.
I've added some preliminary comments, but I think Mike and Brian will want to take a look at this too. https://codereview.appspot.com/6354065/diff/5001/gm/imagezoom.cpp File gm/imagezoom.cpp (right): https://codereview.appspot.com/6354065/diff/5001/gm/imagezoom.cpp#newcode9 gm/imagezoom.cpp:9: #include "SkZoomImageFilter.h" Hmm, not sure why, but this is showing up as a diff from imageblur.cpp. Maybe it git detecting it as a file copy as setting it as such in svn? Not sure, but you should probably svn revert and re-svn add to get it to forget the connection. https://codereview.appspot.com/6354065/diff/5001/include/effects/SkZoomImageF... File include/effects/SkZoomImageFilter.h (right): https://codereview.appspot.com/6354065/diff/5001/include/effects/SkZoomImageF... include/effects/SkZoomImageFilter.h:31: SkPoint origin_; Skia coding style is to use lower case 'f' and camel case (e.g., fOrigin) for member variables. See https://sites.google.com/site/skiadocs/developer-documentation/contributing-c.... https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:616: * Zooms a subset of the texture to a larger size with a nice edge. IWBN to know what the algorithm is a little more clearly here. https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:618: * @param temp1 A scratch texture. Must not be NULL. You should probably just rename temp1 to dstTexture. If it's possible to allocate it in the caller, just do that and pass in a GrTexture and not a GrAutoScratchTexture (blur only passes in a GrAutoScratchTexture since the caller doesn't know the size up front, although that should be changed as well). https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:623: * @param xOffset Horizontal offset to the zoomed region. Would it make sense to just pass in a srcRect, instead of xOffset, yOffset, and zoom? If so, rename rect to dstRect. It would allow non-square zooms, though. If that's a problem, I'd replace xOffset and yOffset with an SkPoint, and pass zoom as-is. https://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): https://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:12: #include <algorithm> There's actually no use of STL in Skia. Not sure if that's an actual policy or not, though. (Mike?) https://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:63: float x_dist = std::min(x, width - x - 1) / inset_; Probably faster to do this as a reciprocal, and hoist it out of the loop. https://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:79: int x_val = weight * (origin_.fX + x / zoom_) + (1 - weight) * x; Same here: probably faster to compute 1 / zoom outside the loop and do a multiply here instead. https://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp#newcod... src/gpu/GrContext.cpp:1962: desc.fConfig = srcTexture->config(); If possible, I'd have the caller allocate this texture instead. https://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp#newcod... src/gpu/GrContext.cpp:1969: paint.textureSampler(0)->setFilter(GrSamplerState::kBilinear_Filter); I'm a little surprised that you do both your zoom shader and bilinear filtering. Shouldn't the zoom shader take care of all interpolation, or am I misunderstanding? https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:106: fXOffsetVar->getName().c_str(), Storing the offset as a vec2 uniform would make this code a little shorter, too. Also, since you have to divide the same coords by zoom anyway, it might be easier to just do (offset + samplecoords) / zoom here, then you wouldn't have to do the divide on the CPU. In fact, this divide could probably be done in the vert shader, to save you doing it in every frag. https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:112: code->appendf("\t\tlowp float y_dist = min(coord.t, 1.0 - coord.t);\n"); I think you could use a vec2 here as well, and do this in one line: vec2 delta = min(coord, vec2(1, 1) - coord). https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:115: code->appendf("\t\ty_dist = y_dist / %s;\n", fYInsetVar->getName().c_str()); If inset is also a vec2, this could then be delta = delta / inset; https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:122: "\t\t\tlowp float dist = sqrt(x_dist * x_dist + y_dist * y_dist);\n"); This could then be lowp float dist = length(delta). (Are the precision specifiers really necessary, BTW? Or is that for performance?)
Sign in to reply to this message.
https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:619: * @param temp2 A scratch texture. May be NULL, in which case Seems to be unused; remove it?
Sign in to reply to this message.
Yep, I think the Ganesh side is good, but I'd like Brian to take a look. https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:170: return static_cast<const GrZoomEffect&>(s).zoom(); Am I missing something, or could you return key 0? I don't see zoom making a change in the GLSL generated? (If we have key 0, we only ever need to compile this code once, loosely speaking... Otherwise, we need to recompile for every change of key.)
Sign in to reply to this message.
https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): https://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:126: code->appendf("\t\t\tlowp float x_weight = (x_dist * x_dist);\n"); Is lowp allowed in desktop GLSL?
Sign in to reply to this message.
Would the effect look better if you used bilerp on the borders (or perhaps gradient instruction for a custom filter width)? http://codereview.appspot.com/6354065/diff/12001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): http://codereview.appspot.com/6354065/diff/12001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1506: GrAutoScratchTexture dst(context, desc); Is it possible to hook into this more generic "asA" mechanism? We hope to move all the filters to this or a similar scheme (and same for SkShader, SkColorFilter, etc). The three filters below that aren't using this scheme today need to do multiple passes and we haven't yet defined the more generic API for such filters. It might require moving your gpu implementation to src/effects from src/gpu/effects, which is OK. I'm hoping we could also remove the zoom function from GrContext. We want to move to a world where GrContext doesn't know about SkImageFilter subclass's custom effects.
Sign in to reply to this message.
http://codereview.appspot.com/6354065/diff/5001/include/effects/SkZoomImageFi... File include/effects/SkZoomImageFilter.h (right): http://codereview.appspot.com/6354065/diff/5001/include/effects/SkZoomImageFi... include/effects/SkZoomImageFilter.h:31: SkPoint origin_; On 2012/07/03 15:05:12, Stephen White wrote: > Skia coding style is to use lower case 'f' and camel case (e.g., fOrigin) for > member variables. See > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c.... Done. http://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): http://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:616: * Zooms a subset of the texture to a larger size with a nice edge. Expanded this comment. On 2012/07/03 15:05:12, Stephen White wrote: > IWBN to know what the algorithm is a little more clearly here. http://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:618: * @param temp1 A scratch texture. Must not be NULL. On 2012/07/03 15:05:12, Stephen White wrote: > You should probably just rename temp1 to dstTexture. If it's possible to > allocate it in the caller, just do that and pass in a GrTexture and not a > GrAutoScratchTexture (blur only passes in a GrAutoScratchTexture since the > caller doesn't know the size up front, although that should be changed as well). Done. http://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:619: * @param temp2 A scratch texture. May be NULL, in which case On 2012/07/03 15:06:20, Stephen White wrote: > Seems to be unused; remove it? Done. http://codereview.appspot.com/6354065/diff/5001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:623: * @param xOffset Horizontal offset to the zoomed region. On 2012/07/03 15:05:12, Stephen White wrote: > Would it make sense to just pass in a srcRect, instead of xOffset, yOffset, and > zoom? If so, rename rect to dstRect. It would allow non-square zooms, though. > If that's a problem, I'd replace xOffset and yOffset with an SkPoint, and pass > zoom as-is. Done. http://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilter... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilter... src/effects/SkZoomImageFilter.cpp:63: float x_dist = std::min(x, width - x - 1) / inset_; On 2012/07/03 15:05:12, Stephen White wrote: > Probably faster to do this as a reciprocal, and hoist it out of the loop. Done. http://codereview.appspot.com/6354065/diff/5001/src/effects/SkZoomImageFilter... src/effects/SkZoomImageFilter.cpp:79: int x_val = weight * (origin_.fX + x / zoom_) + (1 - weight) * x; On 2012/07/03 15:05:12, Stephen White wrote: > Same here: probably faster to compute 1 / zoom outside the loop and do a > multiply here instead. Done. http://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): http://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp#newcode... src/gpu/GrContext.cpp:1962: desc.fConfig = srcTexture->config(); The caller would just pass in a GrTexture instead of a scratch texture, then? On 2012/07/03 15:05:12, Stephen White wrote: > If possible, I'd have the caller allocate this texture instead. http://codereview.appspot.com/6354065/diff/5001/src/gpu/GrContext.cpp#newcode... src/gpu/GrContext.cpp:1969: paint.textureSampler(0)->setFilter(GrSamplerState::kBilinear_Filter); Removed this. On 2012/07/03 15:05:12, Stephen White wrote: > I'm a little surprised that you do both your zoom shader and bilinear filtering. > Shouldn't the zoom shader take care of all interpolation, or am I > misunderstanding? http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.cpp File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:106: fXOffsetVar->getName().c_str(), Changed to vec2 Since this varies based on samplecoords, doesn't it need to be done in the fragment shader? On 2012/07/03 15:05:12, Stephen White wrote: > Storing the offset as a vec2 uniform would make this code a little shorter, too. > Also, since you have to divide the same coords by zoom anyway, it might be > easier to just do (offset + samplecoords) / zoom here, then you wouldn't have to > do the divide on the CPU. > > In fact, this divide could probably be done in the vert shader, to save you > doing it in every frag. http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:112: code->appendf("\t\tlowp float y_dist = min(coord.t, 1.0 - coord.t);\n"); On 2012/07/03 15:05:12, Stephen White wrote: > I think you could use a vec2 here as well, and do this in one line: vec2 delta > = min(coord, vec2(1, 1) - coord). Done. http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:115: code->appendf("\t\ty_dist = y_dist / %s;\n", fYInsetVar->getName().c_str()); On 2012/07/03 15:05:12, Stephen White wrote: > If inset is also a vec2, this could then be delta = delta / inset; Done. http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:122: "\t\t\tlowp float dist = sqrt(x_dist * x_dist + y_dist * y_dist);\n"); On 2012/07/03 15:05:12, Stephen White wrote: > This could then be lowp float dist = length(delta). > > (Are the precision specifiers really necessary, BTW? Or is that for > performance?) Done. http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:126: code->appendf("\t\t\tlowp float x_weight = (x_dist * x_dist);\n"); It seems to be. I use it because I'm used to Android/iOS GL. Removed. On 2012/07/03 15:38:27, bsalomon wrote: > Is lowp allowed in desktop GLSL? http://codereview.appspot.com/6354065/diff/5001/src/gpu/effects/GrZoomEffect.... src/gpu/effects/GrZoomEffect.cpp:170: return static_cast<const GrZoomEffect&>(s).zoom(); On 2012/07/03 15:14:25, TomH wrote: > Am I missing something, or could you return key 0? > I don't see zoom making a change in the GLSL generated? > (If we have key 0, we only ever need to compile this code once, loosely > speaking... Otherwise, we need to recompile for every change of key.) Done. http://codereview.appspot.com/6354065/diff/12001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): http://codereview.appspot.com/6354065/diff/12001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1506: GrAutoScratchTexture dst(context, desc); On 2012/07/18 18:39:34, bsalomon wrote: > Is it possible to hook into this more generic "asA" mechanism? We hope to move > all the filters to this or a similar scheme (and same for SkShader, > SkColorFilter, etc). > > The three filters below that aren't using this scheme today need to do multiple > passes and we haven't yet defined the more generic API for such filters. > > It might require moving your gpu implementation to src/effects from > src/gpu/effects, which is OK. I'm hoping we could also remove the zoom function > from GrContext. We want to move to a world where GrContext doesn't know about > SkImageFilter subclass's custom effects. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp File gm/imagezoom.cpp (right): http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode9 gm/imagezoom.cpp:9: #include "SkZoomImageFilter.h" This file still seems to think it was descended from SkBlurImageFilter.h. I don't know if that's an svn/git issue, but it does seem weird. http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode16 gm/imagezoom.cpp:16: class ImageZoomGM : public GM { I hate to bikeshed, but "Zoom" doesn't really convey much information as a name for this effect. Is there a more accurate name for this resizing algorithm that we could use instead? http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode35 gm/imagezoom.cpp:35: SkRect::MakeXYWH(125, 125, WIDTH / 2, HEIGHT / 2), Technically, this should probably be SkRect::MakeXYWH(SkIntToScalar(125), SkIntToScalar(125), ...) OTOH, since we're no longer testing the fixed-point path, perhaps it's a moot point. (Mike?) http://codereview.appspot.com/6354065/diff/19001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): http://codereview.appspot.com/6354065/diff/19001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:611: * (distance_to_edge / inset) ^2, producing a rounded lens effect. Re: bikeshedding.. maybe LensZoom, RoundedCornerZoom, LensUpsample, RoundedCornerUpsample, ... http://codereview.appspot.com/6354065/diff/19001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (left): http://codereview.appspot.com/6354065/diff/19001/src/core/SkPaint.cpp#oldcode... src/core/SkPaint.cpp:2259: Gratuitous whitespace change? http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:69: float inv_inset = 1.0f / fInset; Technically, this should be SkScalarToFloat(fInset), etc. (Not sure if we care.) Protect against divzero? http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:71: float inv_y_zoom = fSrcRect.height() / src.height(); Protect against a zero-sized bitmap? http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1535: SkScalar inset; Now-spurious; please remove. http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1671: SkScalar inset; Now-spurious; please remove. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:16: static const UniformHandle kInvalidUniformHandle = GrGLUniformManager::kInvalidUniformHandle; Brian can comment on these, but I think they're no longer necessary. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:100: code->appendf("\t\t\tweight = min(weight, 1.0);\n"); I think these four lines could be vec2 deltaSquared = delta * delta; weight = min(min(deltaSquared.s, deltaSquared.t), 1.0); http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:104: "\t\tvec2 mix_coord = weight * zoom_coord + (1.0 - weight) * coord;\n"); Could be mix(zoom_coord, coord, weight). http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect.h File src/gpu/effects/GrZoomEffect.h (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.h:1: /* Could move this file (and the .cpp) into src/effects (or into SkZoomFilter.cpp itself). Don't know if we want to enforce that, though (Brian?)
Sign in to reply to this message.
Overall this is looking good. I have some nits. One other detail: We should add this class to our ill-conceived unit test :) GrGpuGL_program.cpp is where it lives. Right now it's ugly, but has proven quite useful at catching issues. (The future work is to have effect subclasses register a factory that creates a randomly configured version of the effect so that the unit test doesn't have knowledge of each effect type and how to configure them.) http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp File gm/imagezoom.cpp (right): http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode16 gm/imagezoom.cpp:16: class ImageZoomGM : public GM { On 2012/07/31 14:28:58, Stephen White wrote: > I hate to bikeshed, but "Zoom" doesn't really convey much information as a name > for this effect. Is there a more accurate name for this resizing algorithm that > we could use instead? I kind of agree though no obvious name comes to mind. Magnifier? TaperedMagnifier? http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:61: if (src.config() != SkBitmap::kARGB_8888_Config) style nit: skia always uses {} http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:67: return false; ditto http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:79: for (int x = 0; x < width; ++x) { style nit: 4-space intends (to my chagrin). http://codereview.appspot.com/6354065/diff/19001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/GrContext.cpp#newcode15 src/gpu/GrContext.cpp:15: #include "effects/GrZoomEffect.h" This can be removed, right? http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:16: static const UniformHandle kInvalidUniformHandle = GrGLUniformManager::kInvalidUniformHandle; On 2012/07/31 14:28:58, Stephen White wrote: > Brian can comment on these, but I think they're no longer necessary. They're still necessary... they just moved from GrGLShaderBuilder to GrGLUniformManager. Though you're using the qualified GrGLUniformManager::kInvalidUniformHandle below. So it'd make sense to either remove the shortcut declared here or use it below. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect.h File src/gpu/effects/GrZoomEffect.h (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.h:1: /* On 2012/07/31 14:28:58, Stephen White wrote: > Could move this file (and the .cpp) into src/effects (or into SkZoomFilter.cpp > itself). Don't know if we want to enforce that, though (Brian?) I think it is a good idea unless the file starts to get too long (which doesn't seem to be the case here).
Sign in to reply to this message.
in general, are we setup to allow app/client added effects (like this one) to be able to completely build and test their subclasses outside of being in skia's core? If not now, I'd like us to document how future extensions could be developed w/o requiring changes to the core.
Sign in to reply to this message.
On 2012/07/31 15:51:42, reed1 wrote: > in general, are we setup to allow app/client added effects (like this one) to be > able to completely build and test their subclasses outside of being in skia's > core? If not now, I'd like us to document how future extensions could be > developed w/o requiring changes to the core. No, we aren't there yet. The interfaces used by custom stages (as well as the virtuals on the base class) are still evolving and we'd break external stages regularly. Once the interfaces stabilize I see no reason that effects can't live in client code. A more minor issues is that we need to do some header shuffling so that custom stages don't have to peer into src/gpu (just include/gpu).
Sign in to reply to this message.
http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp File gm/imagezoom.cpp (right): http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode9 gm/imagezoom.cpp:9: #include "SkZoomImageFilter.h" Definitely a git-svn issue. It's being too smart. On 2012/07/31 14:28:58, Stephen White wrote: > This file still seems to think it was descended from SkBlurImageFilter.h. I > don't know if that's an svn/git issue, but it does seem weird. http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode16 gm/imagezoom.cpp:16: class ImageZoomGM : public GM { Magnifier sounds good to me. On 2012/07/31 15:17:33, bsalomon wrote: > On 2012/07/31 14:28:58, Stephen White wrote: > > I hate to bikeshed, but "Zoom" doesn't really convey much information as a > name > > for this effect. Is there a more accurate name for this resizing algorithm > that > > we could use instead? > > I kind of agree though no obvious name comes to mind. Magnifier? > TaperedMagnifier? http://codereview.appspot.com/6354065/diff/19001/gm/imagezoom.cpp#newcode35 gm/imagezoom.cpp:35: SkRect::MakeXYWH(125, 125, WIDTH / 2, HEIGHT / 2), On 2012/07/31 14:28:58, Stephen White wrote: > Technically, this should probably be > > SkRect::MakeXYWH(SkIntToScalar(125), SkIntToScalar(125), ...) > > OTOH, since we're no longer testing the fixed-point path, perhaps it's a moot > point. (Mike?) Done. http://codereview.appspot.com/6354065/diff/19001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): http://codereview.appspot.com/6354065/diff/19001/include/gpu/GrContext.h#newc... include/gpu/GrContext.h:611: * (distance_to_edge / inset) ^2, producing a rounded lens effect. I'm happy to rename it to whatever you prefer. On 2012/07/31 14:28:58, Stephen White wrote: > Re: bikeshedding.. maybe LensZoom, RoundedCornerZoom, LensUpsample, > RoundedCornerUpsample, ... http://codereview.appspot.com/6354065/diff/19001/src/core/SkPaint.cpp File src/core/SkPaint.cpp (left): http://codereview.appspot.com/6354065/diff/19001/src/core/SkPaint.cpp#oldcode... src/core/SkPaint.cpp:2259: Added and removed code here. Reverted to master. On 2012/07/31 14:28:58, Stephen White wrote: > Gratuitous whitespace change? http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:61: if (src.config() != SkBitmap::kARGB_8888_Config) On 2012/07/31 15:17:33, bsalomon wrote: > style nit: skia always uses {} Done. http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:67: return false; On 2012/07/31 15:17:33, bsalomon wrote: > ditto Done. http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:69: float inv_inset = 1.0f / fInset; On 2012/07/31 14:28:58, Stephen White wrote: > Technically, this should be SkScalarToFloat(fInset), etc. (Not sure if we > care.) > > Protect against divzero? Done. http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:71: float inv_y_zoom = fSrcRect.height() / src.height(); On 2012/07/31 14:28:58, Stephen White wrote: > Protect against a zero-sized bitmap? Done. http://codereview.appspot.com/6354065/diff/19001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:79: for (int x = 0; x < width; ++x) { On 2012/07/31 15:17:33, bsalomon wrote: > style nit: 4-space intends (to my chagrin). Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/GrContext.cpp#newcode15 src/gpu/GrContext.cpp:15: #include "effects/GrZoomEffect.h" On 2012/07/31 15:17:33, bsalomon wrote: > This can be removed, right? Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1535: SkScalar inset; On 2012/07/31 14:28:58, Stephen White wrote: > Now-spurious; please remove. Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/SkGpuDevice.cpp#newc... src/gpu/SkGpuDevice.cpp:1671: SkScalar inset; On 2012/07/31 14:28:58, Stephen White wrote: > Now-spurious; please remove. Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:16: static const UniformHandle kInvalidUniformHandle = GrGLUniformManager::kInvalidUniformHandle; On 2012/07/31 15:17:33, bsalomon wrote: > On 2012/07/31 14:28:58, Stephen White wrote: > > Brian can comment on these, but I think they're no longer necessary. > > They're still necessary... they just moved from GrGLShaderBuilder to > GrGLUniformManager. > > Though you're using the qualified GrGLUniformManager::kInvalidUniformHandle > below. So it'd make sense to either remove the shortcut declared here or use it > below. Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:100: code->appendf("\t\t\tweight = min(weight, 1.0);\n"); On 2012/07/31 14:28:58, Stephen White wrote: > I think these four lines could be > > vec2 deltaSquared = delta * delta; > weight = min(min(deltaSquared.s, deltaSquared.t), 1.0); Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:104: "\t\tvec2 mix_coord = weight * zoom_coord + (1.0 - weight) * coord;\n"); On 2012/07/31 14:28:58, Stephen White wrote: > Could be mix(zoom_coord, coord, weight). Done. http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect.h File src/gpu/effects/GrZoomEffect.h (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.h:1: /* Do you mean that I should inline this class in src/effects/SkZoomImageFilter.cpp? On 2012/07/31 15:17:33, bsalomon wrote: > On 2012/07/31 14:28:58, Stephen White wrote: > > Could move this file (and the .cpp) into src/effects (or into SkZoomFilter.cpp > > itself). Don't know if we want to enforce that, though (Brian?) > > I think it is a good idea unless the file starts to get too long (which doesn't > seem to be the case here).
Sign in to reply to this message.
On 2012/08/01 01:18:16, zork wrote: > http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect.h > File src/gpu/effects/GrZoomEffect.h (right): > > http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... > src/gpu/effects/GrZoomEffect.h:1: /* > Do you mean that I should inline this class in > src/effects/SkZoomImageFilter.cpp? > > On 2012/07/31 15:17:33, bsalomon wrote: > > On 2012/07/31 14:28:58, Stephen White wrote: > > > Could move this file (and the .cpp) into src/effects (or into > SkZoomFilter.cpp > > > itself). Don't know if we want to enforce that, though (Brian?) > > > > I think it is a good idea unless the file starts to get too long (which > doesn't > > seem to be the case here). Yes, that's the danger of writing to an API that's under heavy development: since you started work, we've reordered the dependency tree. We used to be: core <- effects <- gpu That required hand-written fake RTTI code in the gpu classes, and we knew from the outset that we wanted to get rid of that. Now we're: core <- gpu <- effects ...and the only downside is that google3 is asking us for a gpu-free build. :( Patches for that are under review now; you can see http://codereview.appspot.com/6452064/ for our current guess at a best practice. Tom
Sign in to reply to this message.
I think this is looking pretty good. Can we add it to the unit test in GrGpuGL_unittest.cpp? http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect.h File src/gpu/effects/GrZoomEffect.h (right): http://codereview.appspot.com/6354065/diff/19001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.h:1: /* On 2012/08/01 01:18:16, zork wrote: > Do you mean that I should inline this class in > src/effects/SkZoomImageFilter.cpp? > > On 2012/07/31 15:17:33, bsalomon wrote: > > On 2012/07/31 14:28:58, Stephen White wrote: > > > Could move this file (and the .cpp) into src/effects (or into > SkZoomFilter.cpp > > > itself). Don't know if we want to enforce that, though (Brian?) > > > > I think it is a good idea unless the file starts to get too long (which > doesn't > > seem to be the case here). > Yup... this was recently done for gradients: https://codereview.appspot.com/6449067
Sign in to reply to this message.
http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:87: if (x_dist < 2 && y_dist < 2) { '2' feels like a magic number here, particularly with the negation that follows. Could we get enough of an explanation of what's going on in a comment to make sense of this? http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:19: GrGLZoomEffect(const GrProgramStageFactory& factory, nit: 4-space indent
Sign in to reply to this message.
I inlined the effect, and renamed it to magnifier. Where did you want me to add the unittest? I can't find a file named GrGpuGL_unittest.cpp. Do you mean in tests/GLProgramsTest.cpp? http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... File src/effects/SkZoomImageFilter.cpp (right): http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... src/effects/SkZoomImageFilter.cpp:87: if (x_dist < 2 && y_dist < 2) { On 2012/08/01 19:26:20, TomH wrote: > '2' feels like a magic number here, particularly with the negation that follows. > Could we get enough of an explanation of what's going on in a comment to make > sense of this? Done. http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... File src/gpu/effects/GrZoomEffect.cpp (right): http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... src/gpu/effects/GrZoomEffect.cpp:19: GrGLZoomEffect(const GrProgramStageFactory& factory, On 2012/08/01 19:26:20, TomH wrote: > nit: 4-space indent Done.
Sign in to reply to this message.
On 2012/08/08 05:05:57, zork wrote: > I inlined the effect, and renamed it to magnifier. > > Where did you want me to add the unittest? I can't find a file named > GrGpuGL_unittest.cpp. Do you mean in tests/GLProgramsTest.cpp? Sorry, the unit test was restructured in the last week. Now stage subclasses register themselves with the test and implement a static factory function to generate an effect stage for the test. Check out this diff for an example: http://code.google.com/p/skia/source/diff?spec=svn4946&r=4946&format=side&pat... Two dummy textures are passed in the textures param to TestCreate that are available for the effect to use. One is rgba and the other is alpha: http://code.google.com/p/skia/source/browse/trunk/include/gpu/GrCustomStageUn... > > http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... > File src/effects/SkZoomImageFilter.cpp (right): > > http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilte... > src/effects/SkZoomImageFilter.cpp:87: if (x_dist < 2 && y_dist < 2) { > On 2012/08/01 19:26:20, TomH wrote: > > '2' feels like a magic number here, particularly with the negation that > follows. > > Could we get enough of an explanation of what's going on in a comment to make > > sense of this? > > Done. > > http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... > File src/gpu/effects/GrZoomEffect.cpp (right): > > http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect... > src/gpu/effects/GrZoomEffect.cpp:19: GrGLZoomEffect(const GrProgramStageFactory& > factory, > On 2012/08/01 19:26:20, TomH wrote: > > nit: 4-space indent > > Done.
Sign in to reply to this message.
Added a test section. out/Debug/tests seems to pass. Do I need to add any extra hooks in the test code, or do the defines handle it? On Wed, Aug 8, 2012 at 10:02 PM, <bsalomon@google.com> wrote: > On 2012/08/08 05:05:57, zork wrote: > >> I inlined the effect, and renamed it to magnifier. >> > > Where did you want me to add the unittest? I can't find a file named >> GrGpuGL_unittest.cpp. Do you mean in tests/GLProgramsTest.cpp? >> > > Sorry, the unit test was restructured in the last week. Now stage > subclasses register themselves with the test and implement a static > factory function to generate an effect stage for the test. Check out > this diff for an example: > > http://code.google.com/p/skia/**source/diff?spec=svn4946&r=** > 4946&format=side&path=/trunk/**src/effects/gradients/**SkSweepGradient.cpp<http://code.google.com/p/skia/source/diff?spec=svn4946&r=4946&format=side&path=/trunk/src/effects/gradients/SkSweepGradient.cpp> > > Two dummy textures are passed in the textures param to TestCreate that > are available for the effect to use. One is rgba and the other is alpha: > > http://code.google.com/p/skia/**source/browse/trunk/include/** > gpu/GrCustomStageUnitTest.h?**spec=svn4946&r=4946#23<http://code.google.com/p/skia/source/browse/trunk/include/gpu/GrCustomStageUnitTest.h?spec=svn4946&r=4946#23> > > > > > > http://codereview.appspot.com/**6354065/diff/29001/src/** > effects/SkZoomImageFilter.cpp<http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilter.cpp> > >> File src/effects/SkZoomImageFilter.**cpp (right): >> > > > http://codereview.appspot.com/**6354065/diff/29001/src/** > effects/SkZoomImageFilter.cpp#**newcode87<http://codereview.appspot.com/6354065/diff/29001/src/effects/SkZoomImageFilter.cpp#newcode87> > >> src/effects/SkZoomImageFilter.**cpp:87: if (x_dist < 2 && y_dist < 2) { >> On 2012/08/01 19:26:20, TomH wrote: >> > '2' feels like a magic number here, particularly with the negation >> > that > >> follows. >> > Could we get enough of an explanation of what's going on in a >> > comment to make > >> > sense of this? >> > > Done. >> > > > http://codereview.appspot.com/**6354065/diff/29001/src/gpu/** > effects/GrZoomEffect.cpp<http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect.cpp> > >> File src/gpu/effects/GrZoomEffect.**cpp (right): >> > > > http://codereview.appspot.com/**6354065/diff/29001/src/gpu/** > effects/GrZoomEffect.cpp#**newcode19<http://codereview.appspot.com/6354065/diff/29001/src/gpu/effects/GrZoomEffect.cpp#newcode19> > >> src/gpu/effects/GrZoomEffect.**cpp:19: GrGLZoomEffect(const >> > GrProgramStageFactory& > >> factory, >> On 2012/08/01 19:26:20, TomH wrote: >> > nit: 4-space indent >> > > Done. >> > > > http://codereview.appspot.com/**6354065/<http://codereview.appspot.com/6354065/> >
Sign in to reply to this message.
On 2012/08/13 09:28:50, zork wrote:
> Added a test section. out/Debug/tests seems to pass. Do I need to add any
> extra hooks in the test code, or do the defines handle it?
The defines should handle it. If you want to be paranoid, put a SkDebug("Testing
MagnifierImageFilter") in your code & make sure that's spewed a few times during
out/Debug/tests.
I'd hope someday that we can talk about migrating this out of Skia into
Chromium, but we don't have the framework to support that yet. LGTM.
Sign in to reply to this message.
I landed this at r5056. You can see I (unfortunately) had to modify GLProgramsTest.cpp to force the linker not to toss the magnifier cpp from the tests program build. I verified that the TestCreate function was called after making the change. I think the dream of not having any centralized place that lists effects to be tested may have to be abandoned. Early buildbot results look like I need to add some #if SK_SUPPORT_GPUs to fix the gpu-less build. will update....
Sign in to reply to this message.
r5061 replaces std::min and std::max with SkScalarMin and SkScalarMax to fix the Windows build; we're a noSTL project. There's some undisciplined mixing of float & SkScalar going on that ought to be cleaned up: we're phasing out support of fixed point, but we're considering phasing in support of doubles. It triggers warnings, so I trust Rob will notice and fix it one of these days.
Sign in to reply to this message.
On 2012/08/13 15:20:22, TomH wrote: > r5061 replaces std::min and std::max with SkScalarMin and SkScalarMax to fix the > Windows build; we're a noSTL project. There's some undisciplined mixing of float > & SkScalar going on that ought to be cleaned up: we're phasing out support of > fixed point, but we're considering phasing in support of doubles. It triggers > warnings, so I trust Rob will notice and fix it one of these days. r5058 added the #if SK_SUPPORT_GPU checks to build in the gpu-less builds.
Sign in to reply to this message.
Some post-landing comments.. https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... File src/ports/SkGlobalInitialization_chromium.cpp (right): https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... src/ports/SkGlobalInitialization_chromium.cpp:18: #include "SkZoomImageFilter.h" Shouldn't this now be SkMagnifierImageFilter.h? https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... src/ports/SkGlobalInitialization_chromium.cpp:28: SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkZoomImageFilter) Same here.
Sign in to reply to this message.
https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... File src/ports/SkGlobalInitialization_default.cpp (right): https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... src/ports/SkGlobalInitialization_default.cpp:48: #include "SkZoomImageFilter.h" Also here.
Sign in to reply to this message.
On 2012/08/13 15:43:23, Stephen White wrote: > Some post-landing comments.. > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... > File src/ports/SkGlobalInitialization_chromium.cpp (right): > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... > src/ports/SkGlobalInitialization_chromium.cpp:18: #include "SkZoomImageFilter.h" > Shouldn't this now be SkMagnifierImageFilter.h? > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... > src/ports/SkGlobalInitialization_chromium.cpp:28: > SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkZoomImageFilter) > Same here. ... and *that* seems to be what was breaking the Android buildbots, which are the only native Skia configuration that hit that static initializer code path.
Sign in to reply to this message.
I'm working on clearing up all the excess warnings in the android portion of the waterfall asap On Mon, Aug 13, 2012 at 11:45 AM, <tomhudson@google.com> wrote: > > On 2012/08/13 15:43:23, Stephen White wrote: >> >> Some post-landing comments.. > > > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... >> >> File src/ports/SkGlobalInitialization_chromium.cpp (right): > > > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... >> >> src/ports/SkGlobalInitialization_chromium.cpp:18: #include > > "SkZoomImageFilter.h" >> >> Shouldn't this now be SkMagnifierImageFilter.h? > > > > https://codereview.appspot.com/6354065/diff/36001/src/ports/SkGlobalInitializ... >> >> src/ports/SkGlobalInitialization_chromium.cpp:28: >> SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkZoomImageFilter) >> Same here. > > > ... and *that* seems to be what was breaking the Android buildbots, > which are the only native Skia configuration that hit that static > initializer code path. > > > https://codereview.appspot.com/6354065/ > > -- > You received this message because you are subscribed to the Google Groups > "skia-review" group. > To post to this group, send email to skia-review@googlegroups.com. > To unsubscribe from this group, send email to > skia-review+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/skia-review?hl=en. > -- Karol Chudy Phone: (203) 898-4233 chudy@google.com
Sign in to reply to this message.
zork@, since this is landed, please close it when you get a chance.
Sign in to reply to this message.
On 2012/08/17 15:48:20, TomH wrote: > zork@, since this is landed, please close it when you get a chance. zork@, also see https://code.google.com/p/skia/issues/detail?id=781 .
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
