Made shadows blurry (thru implementing variance mapping)

Made shadows blurry (thru implementing variance mapping)

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2224163005

Committed: https://skia.googlesource.com/skia/+/e6f5d5623160a69e1585f5121a3695092327dfe0

[vjiaoblack, 2016-08-09 20:43:34]

Description was changed from

==========
Made shadows blurry (thru implementing variance mapping)

BUG=skia:
==========

to

==========
Made shadows blurry (thru implementing variance mapping)

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2224163005
==========

[vjiaoblack, 2016-08-09 21:31:00]

vjiaoblack@google.com changed reviewers:
+ jvanverth@google.com, robertphillips@google.com

[reed1, 2016-08-10 11:51:44]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-08-10 11:51:45]

does all the new code need to live in canvas.cpp?

[vjiaoblack, 2016-08-10 14:05:39]

On 2016/08/10 11:51:45, reed1 wrote:
> does all the new code need to live in canvas.cpp?

Well, it lives in the drawShadowedPic function, which is in canvas - where else
would you suggest it go?

[robertphillips, 2016-08-10 14:23:19]

We're going to need words somewhere to:

a) describe variance shadow mapping
b) explain how it interacts with the indexed shadow mapping
c) explain the precision issues (we're not using F16 or F32)

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:3090: 
Let's parameterize this so we can turn variance shadow maps on & off

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:3154: 
Why was this moved down?

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp...
src/core/SkShadowShader.cpp:252: depthMapWidthUniName[i],
depthMapHeightUniName[i]);
??

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp...
src/core/SkShadowShader.cpp:276: 
can we parameterize the shadow generation ?

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
src/utils/SkShadowPaintFilterCanvas.cpp:12: 
Add parameter

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
src/utils/SkShadowPaintFilterCanvas.cpp:27: 
parameterize & comment

[vjiaoblack, 2016-08-11 18:28:51]

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:3090: 
On 2016/08/10 14:23:18, robertphillips wrote:
> Let's parameterize this so we can turn variance shadow maps on & off

Done.

https://codereview.chromium.org/2224163005/diff/1/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:3154: 
On 2016/08/10 14:23:18, robertphillips wrote:
> Why was this moved down?

Done.

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp...
src/core/SkShadowShader.cpp:252: depthMapWidthUniName[i],
depthMapHeightUniName[i]);
On 2016/08/10 14:23:18, robertphillips wrote:
> ??

Done.

https://codereview.chromium.org/2224163005/diff/1/src/core/SkShadowShader.cpp...
src/core/SkShadowShader.cpp:276: 
On 2016/08/10 14:23:18, robertphillips wrote:
> can we parameterize the shadow generation ?

Done.

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
src/utils/SkShadowPaintFilterCanvas.cpp:12: 
On 2016/08/10 14:23:18, robertphillips wrote:
> Add parameter

Done.

https://codereview.chromium.org/2224163005/diff/1/src/utils/SkShadowPaintFilt...
src/utils/SkShadowPaintFilterCanvas.cpp:27: 
On 2016/08/10 14:23:18, robertphillips wrote:
> parameterize & comment

Done.

[jvanverth1, 2016-08-12 17:39:09]

https://codereview.chromium.org/2224163005/diff/40001/gm/shadowmaps.cpp
File gm/shadowmaps.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/gm/shadowmaps.cpp#newco...
gm/shadowmaps.cpp:108: SkShadowType fShType;
Nit: it'd be clearer to me if you wrote this out as fShadowType. Though using
sType seems fine as a local variable/parameter.

https://codereview.chromium.org/2224163005/diff/40001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/40001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1144: SkShadowType sType);
We pass a struct as const& if it's not modified.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3146: SkPaint paint2;
Suggestion: SkPaint blurPaint;

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3152: SkImageInfo info2 =
SkImageInfo::Make(shMapSize.fWidth, shMapSize.fHeight,
Suggestion: rename to blurInfo, blurSurface, blurredDepthMap, etc.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3153: kBGRA_8888_SkColorType,
Nit: align just past (

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
File src/core/SkShadowMapShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:107: fLightDir = lights->light(0).dir();
Code indented too far.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:203: //            fragBuilder->codeAppendf(" %s
*= %s;\n",
Is this needed?

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:212: //            fragBuilder->codeAppendf("if
(%s.b == %s.b) {"
Is this needed?

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:236: const SkVector3& lightDir =
shadowMapFP.lightDir();
Code indented too far.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:269: fLightDirUni;
Put on same line as type.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:274: fDepthMapWidthUni;
Same here.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:278: fDepthMapHeightUni;
same here.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:314: if (fAmbientColor !=
shadowMapFP.fAmbientColor || fNumDirLights != shadowMapFP.fNumDirLights) {
Line too long?

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:353: sk_sp<GrFragmentProcessor>
SkShadowMapShaderImpl::asFragmentProcessor(const AsFPArgs& fpargs) const {
Line too long?

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:472: bool isAmbient = buf.readBool();
Indented too far.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:515: const SkLights::Light& light =
fLights->light(0);
Indented too far.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowMapSha...
src/core/SkShadowMapShader.cpp:576:
SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkShadowMapShaderImpl)
Remove indent.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:148: //        printf("init: %d\n",
sType.fBlurAlgorithm);
Remove this or put in #ifdef block (e.g. #ifdef DUMP_BLUR_PARAMS)? Also, use
SkDebugf instead.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:340: //                         lightProb.c_str(),
clamp.c_str(), clamp.c_str(), clamp.c_str());
Line length > 100? And are these commented out lines needed?

https://codereview.chromium.org/2224163005/diff/40001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:38: color |= (z2 / 256) << 8;
I think (z2/256) << 8 is the same as (z2 & ~0xff). But if you use that, add a
comment to explain what's going on.

[vjiaoblack, 2016-08-12 19:07:42]

https://codereview.chromium.org/2224163005/diff/40001/gm/shadowmaps.cpp
File gm/shadowmaps.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/gm/shadowmaps.cpp#newco...
gm/shadowmaps.cpp:108: SkShadowType fShType;
On 2016/08/12 17:39:08, jvanverth1 wrote:
> Nit: it'd be clearer to me if you wrote this out as fShadowType. Though using
> sType seems fine as a local variable/parameter.

Done.

https://codereview.chromium.org/2224163005/diff/40001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/40001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1144: SkShadowType sType);
On 2016/08/12 17:39:08, jvanverth1 wrote:
> We pass a struct as const& if it's not modified.

Done.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3146: SkPaint paint2;
On 2016/08/12 17:39:08, jvanverth1 wrote:
> Suggestion: SkPaint blurPaint;

Done.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3152: SkImageInfo info2 =
SkImageInfo::Make(shMapSize.fWidth, shMapSize.fHeight,
On 2016/08/12 17:39:08, jvanverth1 wrote:
> Suggestion: rename to blurInfo, blurSurface, blurredDepthMap, etc.

Done.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3153: kBGRA_8888_SkColorType,
On 2016/08/12 17:39:08, jvanverth1 wrote:
> Nit: align just past (

Done.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:148: //        printf("init: %d\n",
sType.fBlurAlgorithm);
On 2016/08/12 17:39:09, jvanverth1 wrote:
> Remove this or put in #ifdef block (e.g. #ifdef DUMP_BLUR_PARAMS)? Also, use
> SkDebugf instead.

Thanks! Done.

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:340: //                         lightProb.c_str(),
clamp.c_str(), clamp.c_str(), clamp.c_str());
On 2016/08/12 17:39:09, jvanverth1 wrote:
> Line length > 100? And are these commented out lines needed?

I would like to keep them as comments to document other possible smooth step
functions to use - is that ok?

https://codereview.chromium.org/2224163005/diff/40001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:38: color |= (z2 / 256) << 8;
On 2016/08/12 17:39:09, jvanverth1 wrote:
> I think (z2/256) << 8 is the same as (z2 & ~0xff). But if you use that, add a
> comment to explain what's going on.

Done.

[jvanverth1, 2016-08-12 19:44:10]

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/40001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:340: //                         lightProb.c_str(),
clamp.c_str(), clamp.c_str(), clamp.c_str());
On 2016/08/12 19:07:42, vjiaoblack wrote:
> On 2016/08/12 17:39:09, jvanverth1 wrote:
> > Line length > 100? And are these commented out lines needed?
> 
> I would like to keep them as comments to document other possible smooth step
> functions to use - is that ok?

Then I would block them out with an #ifdef, just like the debug output.
Otherwise it's hard to tell whether it belongs there or not.

[jvanverth1, 2016-08-15 15:14:43]

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3073: const SkShadowType sType) {
const SkShadowType& sType. By using a ref you pass it as a pointer as opposed to
putting the whole struct on the stack.

[jvanverth1, 2016-08-15 16:37:18]

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1144: const SkShadowType sType);
const &

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1148: const SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/include/private/SkRecor...
File include/private/SkRecords.h (right):

https://codereview.chromium.org/2224163005/diff/80001/include/private/SkRecor...
include/private/SkRecords.h:261: SkShadowType sType);
const SkShadowType&

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3084: const SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp#n...
src/core/SkLiteDL.cpp:226: const SkPaint* paint, SkShadowType sType)
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp#n...
src/core/SkLiteDL.cpp:548: const SkPaint* paint, SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.h
File src/core/SkLiteDL.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.h#new...
src/core/SkLiteDL.h:53: const SkPaint*, SkShadowType sType);
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
File src/core/SkLiteRecorder.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.cpp:183: SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder.h
File src/core/SkLiteRecorder.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.h:76: const SkPaint*, SkShadowType sType) override;
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.h:80: const SkPaint*, SkShadowType sType);
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.cpp:698: SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:220: const SkPaint*, SkShadowType sType) override;
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:223: const SkPaint*, SkShadowType sType);
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.cpp...
src/core/SkRecorder.cpp:299: const SkPaint* paint, SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h
File src/core/SkRecorder.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h#n...
src/core/SkRecorder.h:141: SkShadowType sType) override;
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h#n...
src/core/SkRecorder.h:146: SkShadowType sType);
const &

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:30: SkShadowType sType)
const SkShadowType&

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:113: SkShadowType sType,
const SkShadowType&

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:487: SkShadowType shadowType() const {return
fShadowType; }
return const SkShadowType&

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:793: SkShadowType sType) {
const SkShadowType&

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader.h
File src/core/SkShadowShader.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.h:39: SkShadowType sType);
const SkShadowType& sType

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:61: void
SkShadowPaintFilterCanvas::setShadowType(SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.h:37: 
const &

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
File tools/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.cpp:64: SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.cpp:620: SkShadowType sType) {
const &

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
File tools/debugger/SkDebugCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.h:261: SkShadowType sType) override;
const &

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.h:266: SkShadowType sType);
const &

[vjiaoblack, 2016-08-15 17:43:32]

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1144: const SkShadowType sType);
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1148: const SkShadowType sType) {
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/include/private/SkRecor...
File include/private/SkRecords.h (right):

https://codereview.chromium.org/2224163005/diff/80001/include/private/SkRecor...
include/private/SkRecords.h:261: SkShadowType sType);
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const SkShadowType&

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3073: const SkShadowType sType) {
On 2016/08/15 15:14:43, jvanverth1 wrote:
> const SkShadowType& sType. By using a ref you pass it as a pointer as opposed
to
> putting the whole struct on the stack.

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:3084: const SkShadowType sType) {
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp#n...
src/core/SkLiteDL.cpp:226: const SkPaint* paint, SkShadowType sType)
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.cpp#n...
src/core/SkLiteDL.cpp:548: const SkPaint* paint, SkShadowType sType) {
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.h
File src/core/SkLiteDL.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteDL.h#new...
src/core/SkLiteDL.h:53: const SkPaint*, SkShadowType sType);
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
File src/core/SkLiteRecorder.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.cpp:183: SkShadowType sType) {
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder.h
File src/core/SkLiteRecorder.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.h:76: const SkPaint*, SkShadowType sType) override;
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkLiteRecorder...
src/core/SkLiteRecorder.h:80: const SkPaint*, SkShadowType sType);
On 2016/08/15 16:37:16, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.cpp:698: SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:220: const SkPaint*, SkShadowType sType) override;
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:223: const SkPaint*, SkShadowType sType);
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.cpp
File src/core/SkRecorder.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.cpp...
src/core/SkRecorder.cpp:299: const SkPaint* paint, SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h
File src/core/SkRecorder.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h#n...
src/core/SkRecorder.h:141: SkShadowType sType) override;
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h#n...
src/core/SkRecorder.h:141: SkShadowType sType) override;
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkRecorder.h#n...
src/core/SkRecorder.h:146: SkShadowType sType);
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:30: SkShadowType sType)
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const SkShadowType&

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:113: SkShadowType sType,
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const SkShadowType&

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:487: SkShadowType shadowType() const {return
fShadowType; }
On 2016/08/15 16:37:17, jvanverth1 wrote:
> return const SkShadowType&

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.cpp:793: SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const SkShadowType&

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader.h
File src/core/SkShadowShader.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/core/SkShadowShader...
src/core/SkShadowShader.h:39: SkShadowType sType);
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const SkShadowType& sType

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.cpp:61: void
SkShadowPaintFilterCanvas::setShadowType(SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
File src/utils/SkShadowPaintFilterCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/src/utils/SkShadowPaint...
src/utils/SkShadowPaintFilterCanvas.h:37: 
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
File tools/debugger/SkDebugCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.cpp:64: SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.cpp:620: SkShadowType sType) {
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
File tools/debugger/SkDebugCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.h:261: SkShadowType sType) override;
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

https://codereview.chromium.org/2224163005/diff/80001/tools/debugger/SkDebugC...
tools/debugger/SkDebugCanvas.h:266: SkShadowType sType);
On 2016/08/15 16:37:17, jvanverth1 wrote:
> const &

Done.

[robertphillips, 2016-08-15 23:00:25]

https://codereview.chromium.org/2224163005/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/120001/include/core/SkCanvas....
include/core/SkCanvas.h:42: 
Would SkShadowParams be clearer?

dox!

https://codereview.chromium.org/2224163005/diff/120001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/120001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:48: 
toggling on & off blurry shadows would be cool

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:15: #include "SkDrawable.h"
Why do we need GrDrawContext ?

Or most of these new headers for that matter ?

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3138: sk_sp<SkLights> curLight = builder.finish();
why rm the std::move ?

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3145: SkPaint blurPaint;
For no blur we shouldn't be doing the draw at all

https://codereview.chromium.org/2224163005/diff/120001/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.h (right):

https://codereview.chromium.org/2224163005/diff/120001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.h:461: public:
Add the shadow parameter to the information string

[vjiaoblack, 2016-08-16 14:16:40]

https://codereview.chromium.org/2224163005/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/120001/include/core/SkCanvas....
include/core/SkCanvas.h:42: 
On 2016/08/15 23:00:24, robertphillips wrote:
> Would SkShadowParams be clearer?
> 
> dox!

Done.

https://codereview.chromium.org/2224163005/diff/120001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/120001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:48: 
On 2016/08/15 23:00:24, robertphillips wrote:
> toggling on & off blurry shadows would be cool

Done.

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:15: #include "SkDrawable.h"
On 2016/08/15 23:00:25, robertphillips wrote:
> Why do we need GrDrawContext ?
> 
> Or most of these new headers for that matter ?

Done.

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3138: sk_sp<SkLights> curLight = builder.finish();
On 2016/08/15 23:00:25, robertphillips wrote:
> why rm the std::move ?

Done.

https://codereview.chromium.org/2224163005/diff/120001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3145: SkPaint blurPaint;
On 2016/08/15 23:00:25, robertphillips wrote:
> For no blur we shouldn't be doing the draw at all

True.
Done.

https://codereview.chromium.org/2224163005/diff/120001/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.h (right):

https://codereview.chromium.org/2224163005/diff/120001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.h:461: public:
On 2016/08/15 23:00:25, robertphillips wrote:
> Add the shadow parameter to the information string

Done.

[robertphillips, 2016-08-16 15:58:17]

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp
File gm/shadowmaps.cpp (right):

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp#newc...
gm/shadowmaps.cpp:82: fShadowParams.fMinVariance = 1024;
fShadowType -> fType ?

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp#newc...
gm/shadowmaps.cpp:107: sk_sp<SkLights> fLights;
line up ?

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
Specifies which ... ?

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:1176: const SkPaint* paint,
sParams -> params ?

https://codereview.chromium.org/2224163005/diff/140001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/140001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:39: #include "SkTraceEvent.h"
Hmmm, I believe core needs to be able to build w/o effects

https://codereview.chromium.org/2224163005/diff/160001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/160001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:302: 
line up ?

https://codereview.chromium.org/2224163005/diff/160001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/160001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3124:
sk_make_sp<SkShadowPaintFilterCanvas>(surf->getCanvas());
setShadowParams ?

https://codereview.chromium.org/2224163005/diff/160001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3148: sk_sp<SkSurface>
blurSurf(this->makeSurface(blurInfo));
I don't think you want/need to ref count the surface's canvas.

[vjiaoblack, 2016-08-16 16:48:22]

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp
File gm/shadowmaps.cpp (right):

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp#newc...
gm/shadowmaps.cpp:82: fShadowParams.fMinVariance = 1024;
On 2016/08/16 15:58:16, robertphillips wrote:
> fShadowType -> fType ?

Done.

https://codereview.chromium.org/2224163005/diff/140001/gm/shadowmaps.cpp#newc...
gm/shadowmaps.cpp:107: sk_sp<SkLights> fLights;
On 2016/08/16 15:58:16, robertphillips wrote:
> line up ?

Done.

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
On 2016/08/16 15:58:16, robertphillips wrote:
> Specifies which ... ?

Done.

https://codereview.chromium.org/2224163005/diff/140001/include/core/SkCanvas....
include/core/SkCanvas.h:1176: const SkPaint* paint,
On 2016/08/16 15:58:16, robertphillips wrote:
> sParams -> params ?

Done.

[robertphillips, 2016-08-16 19:36:10]

https://codereview.chromium.org/2224163005/diff/150024/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:95: fLightsChanged = true;
"break;" here ?

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3124:
sk_make_sp<SkShadowPaintFilterCanvas>(surf->getCanvas());
setShadowParams ?

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3148: sk_sp<SkSurface>
blurSurf(this->makeSurface(blurInfo));
Don't ref canvas

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkPictureReco...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkPictureReco...
src/core/SkPictureRecord.cpp:714: 
What's going on with serialization here? How does this pass the following
validate test?

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:7: 
Why do you need SkCanvas.h ? It is a very powerful thing to include.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:299: 
What are we buying by putting this in an SkString and then calling c_str
everywhere vs. just putting "lightProbability" explicitly in the shader code ?

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:485: 
shadowParams ?

https://codereview.chromium.org/2224163005/diff/150024/src/utils/SkShadowPain...
File src/utils/SkShadowPaintFilterCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/150024/src/utils/SkShadowPain...
src/utils/SkShadowPaintFilterCanvas.h:37: 
setShadowParams ?

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2412: 
put this copy in the ctor list ?

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2413: fShadowParams = params;
don't make shStr and just appendf to str ?

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2443: xlate_and_scale_to_bounds(canvas,
fPicture->cullRect());
don't we have fMatrix and fPaint ?

[vjiaoblack, 2016-08-17 12:58:57]

https://codereview.chromium.org/2224163005/diff/150024/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:95: fLightsChanged = true;
On 2016/08/16 19:36:08, robertphillips wrote:
> "break;" here ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3124:
sk_make_sp<SkShadowPaintFilterCanvas>(surf->getCanvas());
On 2016/08/16 19:36:08, robertphillips wrote:
> setShadowParams ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3124:
sk_make_sp<SkShadowPaintFilterCanvas>(surf->getCanvas());
On 2016/08/16 19:36:08, robertphillips wrote:
> setShadowParams ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3148: sk_sp<SkSurface>
blurSurf(this->makeSurface(blurInfo));
On 2016/08/16 19:36:09, robertphillips wrote:
> Don't ref canvas

Er, it seemed to break if I made it into sk_sp instead of sk_ref_sp, so I made
do with the blurDepthMapCanvas variable entirely

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkPictureReco...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkPictureReco...
src/core/SkPictureRecord.cpp:714: 
On 2016/08/16 19:36:09, robertphillips wrote:
> What's going on with serialization here? How does this pass the following
> validate test?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:7: 
On 2016/08/16 19:36:09, robertphillips wrote:
> Why do you need SkCanvas.h ? It is a very powerful thing to include.

We need access to the SkShadowParams struct.

Forward declaring it doesn't seem to work because it's a struct?

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:299: 
On 2016/08/16 19:36:09, robertphillips wrote:
> What are we buying by putting this in an SkString and then calling c_str
> everywhere vs. just putting "lightProbability" explicitly in the shader code ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:485: 
On 2016/08/16 19:36:09, robertphillips wrote:
> shadowParams ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/src/utils/SkShadowPain...
File src/utils/SkShadowPaintFilterCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/150024/src/utils/SkShadowPain...
src/utils/SkShadowPaintFilterCanvas.h:37: 
On 2016/08/16 19:36:09, robertphillips wrote:
> setShadowParams ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2412: 
On 2016/08/16 19:36:09, robertphillips wrote:
> put this copy in the ctor list ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2413: fShadowParams = params;
On 2016/08/16 19:36:09, robertphillips wrote:
> don't make shStr and just appendf to str ?

Done.

https://codereview.chromium.org/2224163005/diff/150024/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2443: xlate_and_scale_to_bounds(canvas,
fPicture->cullRect());
On 2016/08/16 19:36:09, robertphillips wrote:
> don't we have fMatrix and fPaint ?

Done.

[robertphillips, 2016-08-17 13:35:44]

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3148: sk_sp<SkSurface>
blurSurf(this->makeSurface(blurInfo));
On 2016/08/17 12:58:56, vjiaoblack wrote:
> On 2016/08/16 19:36:09, robertphillips wrote:
> > Don't ref canvas
> 
> Er, it seemed to break if I made it into sk_sp instead of sk_ref_sp, so I made
> do with the blurDepthMapCanvas variable entirely

?? Just write:

SkCanvas* blurDepthMapCanvas = blurSurf->getCanvas();

If you read the comment for SkSurface::getCanvas it says that the returned
canvas is "managed/owned by the SkSurface".

[vjiaoblack, 2016-08-17 13:41:33]

On 2016/08/17 13:35:44, robertphillips wrote:
> https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/2224163005/diff/150024/src/core/SkCanvas.cpp#...
> src/core/SkCanvas.cpp:3148: sk_sp<SkSurface>
> blurSurf(this->makeSurface(blurInfo));
> On 2016/08/17 12:58:56, vjiaoblack wrote:
> > On 2016/08/16 19:36:09, robertphillips wrote:
> > > Don't ref canvas
> > 
> > Er, it seemed to break if I made it into sk_sp instead of sk_ref_sp, so I
made
> > do with the blurDepthMapCanvas variable entirely
> 
> ?? Just write:
> 
> SkCanvas* blurDepthMapCanvas = blurSurf->getCanvas();
> 
> If you read the comment for SkSurface::getCanvas it says that the returned
> canvas is "managed/owned by the SkSurface".

Ah. Okay, I see.
In any case, that's essentially what ended up happening - except I just used 
the return value directly instead of creating a variable. Is that okay?

[robertphillips, 2016-08-23 17:19:40]

https://codereview.chromium.org/2224163005/diff/210001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/210001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
fType

https://codereview.chromium.org/2224163005/diff/230001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/230001/include/core/SkCanvas....
include/core/SkCanvas.h:1125: *  We also support using variance shadow maps for
blurred shadows; the user can specify
sType -> params

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:88: case 'B':
Switch the order of these so the writeable one is on the right. (Yoda-speak)

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:250: fSelectedSlider = i;
Can we add a subroutine to share this code with the above block ?

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:40: #include "SkTraceEvent.h"
Can we fix this now ?

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3246: 
rm commented out code

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp....
File src/core/SkCanvas.cpp.orig (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp....
src/core/SkCanvas.cpp.orig:1: /*
rm this file

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkLiteDL.cpp....
File src/core/SkLiteDL.cpp.orig (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkLiteDL.cpp....
src/core/SkLiteDL.cpp.orig:1: /*
delete this file

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:8: #include "SkCanvas.h"
rm comment code

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:295: 
This comment seems misplaced

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:525: 
line up

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2464: params.fShadowRadius,
params.fType == X ? "X" : "Y" - make it human readable

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2476: }
put this in the ctor list

[vjiaoblack, 2016-08-23 17:22:34]

Patchset #14 (id:250001) has been deleted

[vjiaoblack, 2016-08-23 18:01:22]

https://codereview.chromium.org/2224163005/diff/230001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/230001/include/core/SkCanvas....
include/core/SkCanvas.h:1125: *  We also support using variance shadow maps for
blurred shadows; the user can specify
On 2016/08/23 17:19:39, robertphillips wrote:
> sType -> params

Done.

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:88: case 'B':
On 2016/08/23 17:19:39, robertphillips wrote:
> Switch the order of these so the writeable one is on the right. (Yoda-speak)

Done.

https://codereview.chromium.org/2224163005/diff/230001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:250: fSelectedSlider = i;
On 2016/08/23 17:19:40, robertphillips wrote:
> Can we add a subroutine to share this code with the above block ?

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:40: #include "SkTraceEvent.h"
On 2016/08/23 17:19:40, robertphillips wrote:
> Can we fix this now ?

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp#...
src/core/SkCanvas.cpp:3246: 
On 2016/08/23 17:19:40, robertphillips wrote:
> rm commented out code

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp....
File src/core/SkCanvas.cpp.orig (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkCanvas.cpp....
src/core/SkCanvas.cpp.orig:1: /*
On 2016/08/23 17:19:40, robertphillips wrote:
> rm this file

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkLiteDL.cpp....
File src/core/SkLiteDL.cpp.orig (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkLiteDL.cpp....
src/core/SkLiteDL.cpp.orig:1: /*
On 2016/08/23 17:19:40, robertphillips wrote:
> delete this file

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkLiteDL.cpp....
src/core/SkLiteDL.cpp.orig:1: /*
On 2016/08/23 17:19:40, robertphillips wrote:
> delete this file

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
File src/core/SkShadowShader.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:8: #include "SkCanvas.h"
On 2016/08/23 17:19:40, robertphillips wrote:
> rm comment code

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:295: 
On 2016/08/23 17:19:40, robertphillips wrote:
> This comment seems misplaced

Done.

https://codereview.chromium.org/2224163005/diff/230001/src/core/SkShadowShade...
src/core/SkShadowShader.cpp:525: 
On 2016/08/23 17:19:40, robertphillips wrote:
> line up

Done.

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2464: params.fShadowRadius,
On 2016/08/23 17:19:40, robertphillips wrote:
> params.fType == X ? "X" : "Y" - make it human readable

Done.

https://codereview.chromium.org/2224163005/diff/230001/tools/debugger/SkDrawC...
tools/debugger/SkDrawCommand.cpp:2476: }
On 2016/08/23 17:19:40, robertphillips wrote:
> put this in the ctor list

Done.

[robertphillips, 2016-08-24 17:34:37]

lgtm + nits

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
This field is no longer called fShadowAlgorithm it is called fType.

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:232: 
this->

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:256: 
this->

[vjiaoblack, 2016-08-24 18:00:39]

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
On 2016/08/24 17:34:37, robertphillips wrote:
> This field is no longer called fShadowAlgorithm it is called fType.

Done.

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:232: 
On 2016/08/24 17:34:37, robertphillips wrote:
> this->

Done.

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:256: 
On 2016/08/24 17:34:37, robertphillips wrote:
> this->

Done.

[vjiaoblack, 2016-08-24 18:00:39]

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/270001/include/core/SkCanvas....
include/core/SkCanvas.h:62: 
On 2016/08/24 17:34:37, robertphillips wrote:
> This field is no longer called fShadowAlgorithm it is called fType.

Done.

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
File samplecode/SampleShadowing.cpp (right):

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:232: 
On 2016/08/24 17:34:37, robertphillips wrote:
> this->

Done.

https://codereview.chromium.org/2224163005/diff/270001/samplecode/SampleShado...
samplecode/SampleShadowing.cpp:256: 
On 2016/08/24 17:34:37, robertphillips wrote:
> this->

Done.

[vjiaoblack, 2016-08-24 18:05:22]

The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 18:05:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vjiaoblack, 2016-08-24 18:08:02]

vjiaoblack@google.com changed reviewers:
+ bsalomon@google.com

[vjiaoblack, 2016-08-24 18:08:03]

Heya, Brian! Could you take a look at this CL?

[commit-bot: I haz the power, 2016-08-24 18:08:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:08:41]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[vjiaoblack, 2016-08-24 18:18:07]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-24 18:18:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2224163005/#ps310001
(title: "fixed compile error for disabled shadowing")

[commit-bot: I haz the power, 2016-08-24 18:18:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 18:20:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:20:50]

Try jobs failed on following builders:
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[bsalomon, 2016-08-24 18:30:35]

https://codereview.chromium.org/2224163005/diff/310001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2224163005/diff/310001/include/core/SkCanvas....
include/core/SkCanvas.h:65: struct SkShadowParams {
I'm not crazy about adding this to the top of SkCanvas.h. This is probably our
most commonly read header by clients and I'm not sure this is the first thing
they should see.

Can we make this a nested type of SkCanvas or put it in its own header?

[vjiaoblack, 2016-08-24 18:34:37]

The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 18:34:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bsalomon, 2016-08-24 19:24:24]

lgtm

[vjiaoblack, 2016-08-24 19:32:31]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-24 19:32:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2224163005/#ps350001
(title: "made SkShadowParams.h")

[commit-bot: I haz the power, 2016-08-24 19:32:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 19:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 19:35:48]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[vjiaoblack, 2016-08-24 19:41:56]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-24 19:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2224163005/#ps370001
(title: "added shadowparams to gyp")

[commit-bot: I haz the power, 2016-08-24 19:42:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 19:44:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 19:44:39]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[vjiaoblack, 2016-08-24 20:05:53]

The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 20:05:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 20:53:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 20:53:56]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[vjiaoblack, 2016-08-25 13:03:02]

The CQ bit was checked by vjiaoblack@google.com

[vjiaoblack, 2016-08-25 13:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2224163005/#ps390001
(title: "Trying different include path")

[commit-bot: I haz the power, 2016-08-25 13:03:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 13:30:25]

Description was changed from

==========
Made shadows blurry (thru implementing variance mapping)

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2224163005
==========

to

==========
Made shadows blurry (thru implementing variance mapping)

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2224163005

Committed:
https://skia.googlesource.com/skia/+/e6f5d5623160a69e1585f5121a3695092327dfe0
==========

[commit-bot: I haz the power, 2016-08-25 13:30:27]

Committed patchset #20 (id:390001) as
https://skia.googlesource.com/skia/+/e6f5d5623160a69e1585f5121a3695092327dfe0