|
|
|
Created:
13 years, 9 months ago by Stephen White Modified:
13 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Shadow blurs of RGBA bitmaps currently apply the bitmap alpha after blurring, which leads to incorrect results. Instead, we extract the alpha before the blur. There was already a special case for A8 masks; this code simply extends it to ARGB8 masks with transform-only matrices. This restriction is OK for blur masks, which are always drawn 1:1, but could be extended to scaled/rotated masks if required.
This patch does the raster case; an upcoming patch will handle the GPU case.
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixes per tom's comments; handle zoom case; cleanup #
Total comments: 2
Patch Set 3 : Don't set fInitialized twice #Patch Set 4 : Only drawBitmapAsMask() w/RGBA when a mask filter is active #Patch Set 5 : Handle SkBitmapProcShaders in drawPath() #
Total messages: 18
|
Stephen White
|
13 years, 9 months ago (2012年04月04日 22:11:59 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp File gm/shadows.cpp (right): http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp#newcode44 gm/shadows.cpp:44: canvas.drawRect(SkRect::MakeXYWH(0, 10, 10, 10), p); IIRC we're trying to get away from drawing in constructors, and do it lazily in onDraw().
On 2012年04月04日 22:11:59, Stephen White wrote: TBH I don't feel qualified to review this change. I know there was some disagreement on blur semantics between you and Mike. IIRC the Chrome bug has been around for a long time and therefore the fix isn't likely to be pushed to M18. If that's right, I'd feel more comfortable sitting on this until Monday when Mike is back and you guys can hash it out. Alternatively, if getting it in this week is urgent I'd say let's put an #if SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME around the changes.
On 2012年04月05日 14:34:11, bsalomon wrote: > On 2012年04月04日 22:11:59, Stephen White wrote: > > TBH I don't feel qualified to review this change. I know there was some > disagreement on blur semantics between you and Mike. IIRC the Chrome bug has > been around for a long time and therefore the fix isn't likely to be pushed to > M18. If that's right, I'd feel more comfortable sitting on this until Monday > when Mike is back and you guys can hash it out. Alternatively, if getting it in > this week is urgent I'd say let's put an #if SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME > around the changes. No, there's no rush. This more a patch for discussion.
On 2012年04月05日 15:35:34, Stephen White wrote: > On 2012年04月05日 14:34:11, bsalomon wrote: > > On 2012年04月04日 22:11:59, Stephen White wrote: > > > > TBH I don't feel qualified to review this change. I know there was some > > disagreement on blur semantics between you and Mike. IIRC the Chrome bug has > > been around for a long time and therefore the fix isn't likely to be pushed to > > M18. If that's right, I'd feel more comfortable sitting on this until Monday > > when Mike is back and you guys can hash it out. Alternatively, if getting it > in > > this week is urgent I'd say let's put an #if > SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME > > around the changes. > > No, there's no rush. This more a patch for discussion. Ok cool :) It's a shame we have to copy out the alpha channel. I suppose adding a blitter that only takes A from an RGBA source is a bit of an undertaking.
http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp File gm/shadows.cpp (right): http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp#newcode44 gm/shadows.cpp:44: canvas.drawRect(SkRect::MakeXYWH(0, 10, 10, 10), p); On 2012年04月05日 14:25:00, TomH wrote: > IIRC we're trying to get away from drawing in constructors, and do it lazily in > onDraw(). Done.
On 2012年04月05日 15:54:57, bsalomon wrote: > It's a shame we have to copy out the alpha channel. I suppose adding a blitter > that only takes A from an RGBA source is a bit of an undertaking. I thought of that, but aside from the usual macro hell^H^H^H^Hfun of writing a blitter, I was worried it might require multiple permutations, for the other bits in the paint. I could be wrong about that, though.
On 2012年04月05日 17:32:18, Stephen White wrote: > On 2012年04月05日 15:54:57, bsalomon wrote: > > It's a shame we have to copy out the alpha channel. I suppose adding a blitter > > that only takes A from an RGBA source is a bit of an undertaking. > > I thought of that, but aside from the usual macro hell^H^H^H^Hfun of writing a > blitter, I was worried it might require multiple permutations, for the other > bits in the paint. I could be wrong about that, though. FYI - PDF always has to extract the alpha channel to a separate mask image.
This is a tricky area. drawBitmap(bm, paing) needs to always draw the same as drawRect(bm_bounds, paint_with_bm_shader) and drawPath(bm_bounds_as_path, paint_with_bm_shader) Does this CL preserve that invariant? Bitmap shaders must be treated just like any shader (e.g. gradients), including just a single color (which has to draw the same as SkColorShader), which are not referenced until after the mask has been computed.
On 2012年04月10日 14:06:20, reed1 wrote: > This is a tricky area. > > drawBitmap(bm, paing) > > needs to always draw the same as > > drawRect(bm_bounds, paint_with_bm_shader) > > and > > drawPath(bm_bounds_as_path, paint_with_bm_shader) > > Does this CL preserve that invariant? That invariant does not hold today, even without my changes, since drawBitmapAsMask() is only called in drawBitmap(), not drawPath() or drawRect(). I'll file a bug once project hosting comes out of the cryochamber. > Bitmap shaders must be treated just like any shader (e.g. gradients), including > just a single color (which has to draw the same as SkColorShader), which are not > referenced until after the mask has been computed.
Yes and no. The invariant I stated doesn't hold, but alpha-only bitmaps are themselves breaking other "rules", since they respect the paint's color (and shader), while all other bitmaps do *not*.
On 2012年04月10日 18:19:00, reed1 wrote: > Yes and no. > > The invariant I stated doesn't hold, but alpha-only bitmaps are themselves > breaking other "rules", since they respect the paint's color (and shader), while > all other bitmaps do *not*. One idea would be to have an SkMaskFilter::filterBitmap() function which handles drawing of masked bitmaps, similar to how we have an SkMaskFilter::filterPath() function for filtering paths. Then any special-case logic could be put in there. Another option would be to pass along the original paint to SkMaskFilter::filterPath() (and SkDraw::DrawToMask), check for the presence of an SkBitmapProcShader, and use a shader that modulates the primitive's mask with the bitmap alpha at that point (maybe the existing SkBitmapProcShader would work when drawing to a mask? Not sure). That would handle the SkBitmapProcShader drawRect/drawPath cases too.
We could change skia's architecture to always sample the shader/color when computing the mask, but that is a huge change, and one that will seriously affect performance, and breaks the current feature that modulating the color's alpha is always applied *after* the maskfilter (as the proposed change would mean we applied it before the filter). The current architecture offer's two distinct features: 1. filter the mask and then compose it with the color (SkMaskFilter) 2. filter the composed mask+color (SkImageFilter) Does the current SkBlurImageFilter meet the current webkit need? If it is a question of performance and/or not wanting to blur the color changes, can we add an option to SkBlurImageFilter that only blurs the resulting alpha channel?
On 2012年04月11日 14:59:53, reed1 wrote: > We could change skia's architecture to always sample the shader/color when > computing the mask, but that is a huge change, and one that will seriously > affect performance, and breaks the current feature that modulating the color's > alpha is always applied *after* the maskfilter (as the proposed change would > mean we applied it before the filter). > > The current architecture offer's two distinct features: > > 1. filter the mask and then compose it with the color (SkMaskFilter) > 2. filter the composed mask+color (SkImageFilter) > > Does the current SkBlurImageFilter meet the current webkit need? If it is a > question of performance and/or not wanting to blur the color changes, can we add > an option to SkBlurImageFilter that only blurs the resulting alpha channel? Yes, we could do the work to extend image filters to A8, but even if we did, we'd have to do all of the shadow passes differently at the WebKit level, depending on whether there was a blur or not, whether the shadow alpha needed to modulate the image color, etc. This is all doable, but they are also cases that SkBlurMaskFilter already handles. Could we add a flag to the maskfilter to indicate that shaders should be respected? That would be a much smaller amount of work, and would make the SkBlurMaskFilter more generally useful to other clients, IMHO. It might also mean we could get rid of the drawBitmapAsMask() workaround as well.
http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp#newcode1065 src/core/SkDraw.cpp:1065: for (int y = 0; y < height; y++) { Do you really need your own pixel loop here? Looking at the code for SkA8_Shader_Blitter, it seems to me like blitting RGBA to A8 could be done by setting an bitmap shader on the paint. I think doing that blit directly would eliminate the need for an intermediate copy in 'mask'.
http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp#newcode1065 src/core/SkDraw.cpp:1065: for (int y = 0; y < height; y++) { On 2012年04月11日 17:16:22, junov1 wrote: > Do you really need your own pixel loop here? Looking at the code for > SkA8_Shader_Blitter, it seems to me like blitting RGBA to A8 could be done by > setting an bitmap shader on the paint. I think doing that blit directly would > eliminate the need for an intermediate copy in 'mask'. Never mind that comment I see why you absolutely need an SkMask. Argh.
This version handles SkBitmapProcShader, by deferring the blitter choice and passing the shader through to draw_into_mask(). Note that this implementation is probably all kinds of wrong (and needs a flag); I just wanted to see how hard it would be to get it working. I do think the change in SkBlitter_A8.cpp is good, and perhaps should be pulled out: if there's no xfermode, SkA8_Shader_Blitter::blitMask() does nothing! (took me a while to figure that one out..)
On 2012年04月11日 20:02:15, Stephen White wrote: > I do think the change in SkBlitter_A8.cpp is good, and perhaps should be pulled > out: if there's no xfermode, SkA8_Shader_Blitter::blitMask() does nothing! > (took me a while to figure that one out..) ... although it should probably be doing a blend, instead of a straight copy.