Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248493002:
raster shadows (Closed)
|
Created:
4 years, 4 months ago by vjiaoblack Modified:
4 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionraster shadows
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002
Committed: https://skia.googlesource.com/skia/+/babc697ae8f7cb2191b049f446bf44d029376d69
Patch Set 1 #
Total comments: 12
Patch Set 2 : made req changes #
Total comments: 4
Patch Set 3 : made req changes - some #
Total comments: 6
Patch Set 4 : made req changes #
Total comments: 4
Patch Set 5 : allocated pixmaps with new[] #
Total comments: 14
Patch Set 6 : made req changes #
Total comments: 8
Patch Set 7 : made req changes #
Total comments: 12
Patch Set 8 : made req changes #
Total comments: 2
Patch Set 9 : made req change #
Total comments: 2
Patch Set 10 : compressed shadow-map indexing code #Messages
Total messages: 32 (9 generated)
Description was changed from ========== raster shadows branch accessed povmap from raster BUG=skia: ========== to ========== raster shadows branch accessed povmap from raster BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002 ==========
Description was changed from ========== raster shadows branch accessed povmap from raster BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002 ========== to ========== raster shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
None of my other CL's are based of this, and this piece of code is easily moved around manually, so there's no rush to review this right away. That being said, I did try to get these ~20 lines of code as clean as I could :P
https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:564: int xOffset = round(light.dir().fX * (SkScalar) SkColorGetB(povDepthColor)); SkScalarRoundToInt instead of round(), and SkIntToScalar rather than explicit cast. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:570: if (shX < 0) { shX = 0; } shX = SkClampPos(shX); https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:573: if (shX >= light.getShadowMap()->width()) { Or replace both the bounds checks with SkClampMax(shX, light.getShadowMap()->width()); https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:583: int pvDepth = SkColorGetB(povDepthColor); Why B? https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:585: if (light.getShadowMap()->peekPixels(shPixels)) { This seems very inefficient -- instead I suggest building an array of Pixmaps (one for each light) outside of the i to n loop and then referencing them here. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:586: uint32_t pix = *shPixels->addr32(shX, shY); This looks like it will do nearest neighbor sampling. Is that what we want for shadow map lookup (I really don't know, just making sure)?
https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:564: int xOffset = round(light.dir().fX * (SkScalar) SkColorGetB(povDepthColor)); On 2016/08/16 00:57:18, jvanverth1 wrote: > SkScalarRoundToInt instead of round(), and SkIntToScalar rather than explicit > cast. Done. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:570: if (shX < 0) { shX = 0; } On 2016/08/16 00:57:18, jvanverth1 wrote: > shX = SkClampPos(shX); Done. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:573: if (shX >= light.getShadowMap()->width()) { On 2016/08/16 00:57:18, jvanverth1 wrote: > Or replace both the bounds checks with SkClampMax(shX, > light.getShadowMap()->width()); Done. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:583: int pvDepth = SkColorGetB(povDepthColor); On 2016/08/16 00:57:18, jvanverth1 wrote: > Why B? We stidk the depth into the blue channel. I'll make note of this in a comment https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:585: if (light.getShadowMap()->peekPixels(shPixels)) { On 2016/08/16 00:57:18, jvanverth1 wrote: > This seems very inefficient -- instead I suggest building an array of Pixmaps > (one for each light) outside of the i to n loop and then referencing them here. Done. https://codereview.chromium.org/2248493002/diff/1/src/core/SkShadowShader.cpp... src/core/SkShadowShader.cpp:586: uint32_t pix = *shPixels->addr32(shX, shY); On 2016/08/16 00:57:18, jvanverth1 wrote: > This looks like it will do nearest neighbor sampling. Is that what we want for > shadow map lookup (I really don't know, just making sure)? Well, we want it to alias - interpolation would be bad here.
https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:546: SkPixmap* shPixels[SkShadowShader::kMaxNonAmbientLights]; If you just put the Pixmap at shPixels[i], then you don't need to track nonAmbLightCnt in the loop below. https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:552: shPixels[nonAmbLightCnt] = nullptr; You have a memory leak here. Use sk_sp<SkPixmap> instead or be sure to delete it.
https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:546: SkPixmap* shPixels[SkShadowShader::kMaxNonAmbientLights]; On 2016/08/17 15:42:43, jvanverth1 wrote: > If you just put the Pixmap at shPixels[i], then you don't need to track > nonAmbLightCnt in the loop below. Well, that'd be inefficient, because I'd have to make the SkPixmap* array based on the number of lights instead, which there isn't really a bound on. (i.e. ambient lights) I could dynamically create an array based on fLights->numLights(), but I think this is a simpler solution. https://codereview.chromium.org/2248493002/diff/20001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:552: shPixels[nonAmbLightCnt] = nullptr; On 2016/08/17 15:42:43, jvanverth1 wrote: > You have a memory leak here. Use sk_sp<SkPixmap> instead or be sure to delete > it. Done.
https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:545: Let's (you, me, Jim & maybe Mike) talk about changing the SkLights contract so it will compress the ambient lights for you. https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:549: if (lightShader.fLights->light(i).type() == SkLights::Light::kDirectional_LightType) { Eeek! A new&delete for each span, for each shadowed light! https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:566: SkColor diffColor = SkUnPreMultiply::PMColorToColor(diffuse[i]); It seems like the depth map won't be in premul space
https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:545: On 2016/08/18 17:33:16, robertphillips wrote: > Let's (you, me, Jim & maybe Mike) talk about changing the SkLights contract so > it will compress the ambient lights for you. Acknowledged. https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:549: if (lightShader.fLights->light(i).type() == SkLights::Light::kDirectional_LightType) { On 2016/08/18 17:33:16, robertphillips wrote: > Eeek! A new&delete for each span, for each shadowed light! Done. https://codereview.chromium.org/2248493002/diff/40001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:566: SkColor diffColor = SkUnPreMultiply::PMColorToColor(diffuse[i]); On 2016/08/18 17:33:16, robertphillips wrote: > It seems like the depth map won't be in premul space Done.
https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:511: Since you're densely packing the pixmaps, let's add a numNonAmbientLights call to SkLights and allocate them all at once. This will mean the later code will need to be able to handle an empty pixmap. https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:516: peekPixels(fShadowMapPixels[fNonAmbLightCnt])) { this assignment doesn't seem so good ...
https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:511: On 2016/08/24 14:48:53, robertphillips wrote: > Since you're densely packing the pixmaps, let's add a numNonAmbientLights call > to SkLights and allocate them all at once. > > This will mean the later code will need to be able to handle an empty pixmap. Done. https://codereview.chromium.org/2248493002/diff/60001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:516: peekPixels(fShadowMapPixels[fNonAmbLightCnt])) { On 2016/08/24 14:48:53, robertphillips wrote: > this assignment doesn't seem so good ... Done.
https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:64: int fNonAmbLightCnt; This should be SkPixmap* https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:651: fShadowMapPixels = new SkPixmap[fNonAmbLightCnt]; https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:657: lightShader.fLights->light(i).getShadowMap()-> shouldn't fNonAmbLightCnt be i here ? https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:659: } else { fShadowMapPixels[i].reset(); } - this isn't really necessary though https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:670: This can then become "delete [] fShadowMapPixels;" https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:675: } as is, you're leaking fShadowMapPixels itself here https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:723: Do you intend to be changing fNonAmbLightCnt in this method ?
https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:64: int fNonAmbLightCnt; On 2016/08/29 18:03:40, robertphillips wrote: > This should be SkPixmap* Done. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:651: On 2016/08/29 18:03:39, robertphillips wrote: > fShadowMapPixels = new SkPixmap[fNonAmbLightCnt]; Done. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:657: lightShader.fLights->light(i).getShadowMap()-> On 2016/08/29 18:03:39, robertphillips wrote: > shouldn't fNonAmbLightCnt be i here ? Done. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:659: } On 2016/08/29 18:03:39, robertphillips wrote: > else { > fShadowMapPixels[i].reset(); > } > > - this isn't really necessary though Yeah, because the uninitialized ones just will be empty, right? I won't add it, unless really think I should. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:670: On 2016/08/29 18:03:39, robertphillips wrote: > This can then become "delete [] fShadowMapPixels;" Done. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:675: } On 2016/08/29 18:03:39, robertphillips wrote: > as is, you're leaking fShadowMapPixels itself here Done. https://codereview.chromium.org/2248493002/diff/80001/src/core/SkShadowShader... src/core/SkShadowShader.cpp:723: On 2016/08/29 18:03:40, robertphillips wrote: > Do you intend to be changing fNonAmbLightCnt in this method ? nope, good catch. Thanks.
https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:651: Only allocate fNonAmbLightCnt https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:670: We should probably delete this first https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:715: It doesn't look like you need totalColor https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:738: int shDepth = 0; Can we declare this earlier and then reuse it in the computation of xOffset & yOffset ?
https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:651: On 2016/08/29 19:21:36, robertphillips wrote: > Only allocate fNonAmbLightCnt Done. https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:670: On 2016/08/29 19:21:36, robertphillips wrote: > We should probably delete this first Done. https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:715: On 2016/08/29 19:21:36, robertphillips wrote: > It doesn't look like you need totalColor Well, we need it at the end to multiply with totalLight and diffColor, I think https://codereview.chromium.org/2248493002/diff/100001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:738: int shDepth = 0; On 2016/08/29 19:21:36, robertphillips wrote: > Can we declare this earlier and then reuse it in the computation of xOffset & > yOffset ? Done.
https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:726: You don't really need to SkIntToScalars here https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:729: Why does 'i' factor into the formula for 'shX' ? https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:739: // pixmaps that point to things have nonzero heights move the definition of 'shDepth' here, closer to its use, so the reader can understand that it doesn't participate in the above calculations. https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:743: Shouldn't this '+=' just be a '=' ? https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:746: // TODO: handle not being able to read shadow map pixels // Make lights w/o a shadow map receive the full light contribution shDepth = pvDepth; ? https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:759: If you want to keep totalColor, move it down here - right before its use as a temporary. Initializing to 0 above is a bit misleading in that it makes one expect that it will participate in the loop as an accumulator.
https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:726: On 2016/08/30 12:32:26, robertphillips wrote: > You don't really need to SkIntToScalars here Acknowledged. https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:729: On 2016/08/30 12:32:26, robertphillips wrote: > Why does 'i' factor into the formula for 'shX' ? i is the offset on X. What seems to happen is that you get an initial x and y coordinate, and a number of pixels to draw. You draw them along the same row, in the +x direction. Thus, the set of all pixels drawn are (x + i, y) from i = [0, n) https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:739: // pixmaps that point to things have nonzero heights On 2016/08/30 12:32:26, robertphillips wrote: > move the definition of 'shDepth' here, closer to its use, so the reader can > understand that it doesn't participate in the above calculations. Done. https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:743: On 2016/08/30 12:32:26, robertphillips wrote: > Shouldn't this '+=' just be a '=' ? Done. https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:746: // TODO: handle not being able to read shadow map pixels On 2016/08/30 12:32:26, robertphillips wrote: > // Make lights w/o a shadow map receive the full light contribution > shDepth = pvDepth; > > ? Done. https://codereview.chromium.org/2248493002/diff/120001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:759: On 2016/08/30 12:32:26, robertphillips wrote: > If you want to keep totalColor, move it down here - right before its use as a > temporary. > > Initializing to 0 above is a bit misleading in that it makes one expect that it > will participate in the loop as an accumulator. Done.
https://codereview.chromium.org/2248493002/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:762: Just assign to totalColor rather than initializing to 0.0f and then accumulating
https://codereview.chromium.org/2248493002/diff/140001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/140001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:762: On 2016/08/30 12:55:08, robertphillips wrote: > Just assign to totalColor rather than initializing to 0.0f and then accumulating Done.
https://codereview.chromium.org/2248493002/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:727: Why aren't the following 6 lines just: int shX = SkClampMax(x + i + xOffset, light.getShadowMap()->width() - 1); int shY = SkClampMax(y + yOffset, light.getShadowMap()->height() - 1); ?
https://codereview.chromium.org/2248493002/diff/160001/src/core/SkShadowShade... File src/core/SkShadowShader.cpp (right): https://codereview.chromium.org/2248493002/diff/160001/src/core/SkShadowShade... src/core/SkShadowShader.cpp:727: On 2016/08/30 13:08:49, robertphillips wrote: > Why aren't the following 6 lines just: > > int shX = SkClampMax(x + i + xOffset, light.getShadowMap()->width() - 1); > int shY = SkClampMax(y + yOffset, light.getShadowMap()->height() - 1); > > ? Sorry. My reasoning while transcribing the shader code was fragmented, and I guess I produced fragmented code that would have made sense in raster to compress it / remove excess lines
lgtm
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vjiaoblack@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== raster shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002 ========== to ========== raster shadows BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2248493002 Committed: https://skia.googlesource.com/skia/+/babc697ae8f7cb2191b049f446bf44d029376d69 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/babc697ae8f7cb2191b049f446bf44d029376d69 |
