Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1719333002:
tile spans (Closed)
Descriptiontile spans
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1719333002
Committed: https://skia.googlesource.com/skia/+/9466571f8ebde1429cca7b6dcd2d4b85bad1b088
Patch Set 1 #Patch Set 2 : WIP: sync. #Patch Set 3 : Spans for Tiling modes. #Patch Set 4 : After sync #
Total comments: 24
Patch Set 5 : Address comments. #
Total comments: 14
Patch Set 6 : Handle large dx case. #Patch Set 7 : Address comments. #
Total comments: 13
Patch Set 8 : Handle -dx. #
Total comments: 31
Patch Set 9 : WIP: part way through span rewrite. #Patch Set 10 : Spans working in tiles. #Patch Set 11 : Span used over entire pipeline. #Patch Set 12 : Fix comment. #
Total comments: 8
Patch Set 13 : Fix repeat boundary condition. #Patch Set 14 : Address remaining comments. #
Messages
Total messages: 74 (26 generated)
Description was changed from ========== tile spans. BUG=skia: ========== to ========== tile spans. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
WIP: sync.
Spans for Tiling modes.
After sync
Description was changed from ========== tile spans. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== tile spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
herb@google.com changed reviewers: + mtklein@google.com, reed@google.com
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/60001
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 herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/60001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-02-23 21:03 UTC
https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:377: SkScalar xMin = fXMin[0]; It's occurred to me that we should probably be using Sk4s for coordinate-y values, not Sk4f. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:382: SkScalar y = std::min(std::max(yMin, start.fY), yMax); Weird to write X(start) and start.fY... https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:383: Some ideas: - invert the logic so the easy case comes first and returns early? - try SSA style so we don't have to reason about values changing? - explain the clever clamp is len = 0 trick? bool under = xMin > x, over = xMax < x + length; if (!under && !over) { // Our span is fully inside the tile. next->pointSpan(...); return true; } SkASSERT(count > 0); SkScalar dx = length / (count-1); int before = 0; SkScalar xMid = x; if (under) { before = SkScalarFloorToInt((xMin - x) / dx) + 1; xMid = xMin; } int after = 0; if (over) { after = SkScalarFloorToInt((x + length - xMax) / dx) + 1; } int middle = count - before - after; SkScalar lenMid = length - (before+after)*dx; ... // spanning with 0 length effects the clamp on either side... we'll step in source space by 0 between each pixel. SendSpan({xMin ,y}, 0, before); SendSpan({xMid ,y}, lenMid, middle); SendSpan({xMax-1,y}, 0, after); // explain the -1? https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:407: next->pointListFew(1, Sk4f{X(start)}, Sk4f{Y(start)}); Why can't pointSpan handle count == 1? https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:433: static float tile_mod(float l, float base) { I think you might want to pick a different letter---or a longer name---than l. It looks surprisingly similar to 1 here. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:434: return l - std::floor(l / base) * base; We're getting a little loosey-goosey about SkScalar and float here. Shouldn't these arguments be SkScalar? https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:470: while (x + length > fXMax[0]) { Explain how this works?
fmalita@chromium.org changed reviewers: + fmalita@chromium.org
https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:388: SkScalar dx = length / (count - 1); 'count' is the span/pixel count, right - why (count - 1)? https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:404: auto SendSpan = [&](SkPoint start, SkScalar length, int count) { Looks like we only need to capture 'next' here. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:460: SkScalar dx = length / (count - 1); Same question re count - 1. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:472: int countForTile = SkScalarFloorToInt(lengthForTile / dx); I noticed we always pad one to the count in the clamp case, while here we let it floor to zero. Are we guaranteed to make progress in that case? I'm guessing in general we are, since we wrap x back to 0 so the next iteration will pick up a full tile. Still makes me a bit nervous - what if the image is 1px wide? https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:476: count -= countForTile; Looks like we only track count in order to handle the leftover outside the loop. Can we fold that in? Maybe something along these lines: while (length > 0) { SkScalar lengthForTile = std::min(fXMax[0] - x, length); int countForTile = SkScalarFloorToInt(lengthForTile / dx); SkScalar lengthForDx = countForTile * dx; SendSpan({x, y}, lengthForDx, countForTile); length -= lengthForDx; x = 0; }
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:388: SkScalar dx = length / (count - 1); A more general comment re. dx and dst->src mapping via dx multiplication: is this perspective-proof? I do the exact same thing for gradients, and I'm pretty sure it assumes a kLinear_MatrixClass - so I don't have much hope there. Just curious whether you have extra machinery that would make it work for bitmap shaders or just don't care about persp.
Address comments.
PTAL https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:377: SkScalar xMin = fXMin[0]; On 2016/02/23 15:51:56, mtklein wrote: > It's occurred to me that we should probably be using Sk4s for coordinate-y > values, not Sk4f. Done. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:382: SkScalar y = std::min(std::max(yMin, start.fY), yMax); On 2016/02/23 15:51:56, mtklein wrote: > Weird to write X(start) and start.fY... Done. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:383: On 2016/02/23 15:51:56, mtklein wrote: > Some ideas: > - invert the logic so the easy case comes first and returns early? > - try SSA style so we don't have to reason about values changing? > - explain the clever clamp is len = 0 trick? > > bool under = xMin > x, > over = xMax < x + length; > > if (!under && !over) { > // Our span is fully inside the tile. > next->pointSpan(...); > return true; > } > > SkASSERT(count > 0); > SkScalar dx = length / (count-1); > > int before = 0; > SkScalar xMid = x; > if (under) { > before = SkScalarFloorToInt((xMin - x) / dx) + 1; > xMid = xMin; > } > > int after = 0; > if (over) { > after = SkScalarFloorToInt((x + length - xMax) / dx) + 1; > } > > int middle = count - before - after; > SkScalar lenMid = length - (before+after)*dx; > ... > // spanning with 0 length effects the clamp on either side... we'll step in > source space by 0 between each pixel. > SendSpan({xMin ,y}, 0, before); > SendSpan({xMid ,y}, lenMid, middle); > SendSpan({xMax-1,y}, 0, after); // explain the -1? I did this a totally different way. Please re-review. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:433: static float tile_mod(float l, float base) { On 2016/02/23 15:51:56, mtklein wrote: > I think you might want to pick a different letter---or a longer name---than l. > It looks surprisingly similar to 1 here. Done. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:434: return l - std::floor(l / base) * base; On 2016/02/23 15:51:56, mtklein wrote: > We're getting a little loosey-goosey about SkScalar and float here. > Shouldn't these arguments be SkScalar? Done. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:470: while (x + length > fXMax[0]) { On 2016/02/23 15:51:56, mtklein wrote: > Explain how this works? Done.
https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:404: auto SendSpan = [&](SkPoint start, SkScalar length, int count) { On 2016/02/23 20:53:48, f(malita) wrote: > Looks like we only need to capture 'next' here. This is gone. I made Span handle count == 1. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:407: next->pointListFew(1, Sk4f{X(start)}, Sk4f{Y(start)}); On 2016/02/23 15:51:56, mtklein wrote: > Why can't pointSpan handle count == 1? There was a NaN problem with the division in a deeper part of the stack. I fixed that, and count == 1 works now. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:460: SkScalar dx = length / (count - 1); On 2016/02/23 20:53:48, f(malita) wrote: > Same question re count - 1. Added much documentation. See if that answers the question. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:472: int countForTile = SkScalarFloorToInt(lengthForTile / dx); On 2016/02/23 20:53:48, f(malita) wrote: > I noticed we always pad one to the count in the clamp case, while here we let it > floor to zero. Are we guaranteed to make progress in that case? I'm guessing > in general we are, since we wrap x back to 0 so the next iteration will pick up > a full tile. > > Still makes me a bit nervous - what if the image is 1px wide? > If the source is 1 pixel wide, then this should make progress because of the calculation to the start of the next pixel. Please see new documentation and see if it answers the problem. https://codereview.chromium.org/1719333002/diff/60001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:476: count -= countForTile; On 2016/02/23 20:53:48, f(malita) wrote: > Looks like we only track count in order to handle the leftover outside the loop. > Can we fold that in? Maybe something along these lines: > > while (length > 0) { > SkScalar lengthForTile = std::min(fXMax[0] - x, length); > int countForTile = SkScalarFloorToInt(lengthForTile / dx); > SkScalar lengthForDx = countForTile * dx; > SendSpan({x, y}, lengthForDx, countForTile); > length -= lengthForDx; > x = 0; > } > > That would mean a bunch of calculation in the common case.
This is making much more sense to me. Thank you for the docs. I'm not quite there yet. May need you to walk me through once or twice. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (left): https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:81: float fVal; SkScalar https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:99: float dx = length / (count - 1); Let's do a careful search for all 'float' in here. It's okay to use float for colors, but we should use SkScalar for spaces. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:18: #if defined(_MSC_VER) && SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 Seems like it's all the other lines that are wrong. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:97: // Xs must be calculated in the following manner instead of dx * {0.0, 1.0, 2.0, 3.0} because Seems fine to branch on count too, if that makes things simpler and less needs-explanation-y: Sk4s Xs{X(start)}; Sk4s fourDx; if (count > 1) { // This math only works when count > 1, but luckily it's only _used_ when count > 1. SkScalar dx = length / (count-1); Xs = Xs + Sk4f{0,1,2,3} * dx; // FWIW, this is only used when count >= 4. fourDx = Sk4f{4} * dx; } while (count >= 4) { ... https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:408: // The length from A to B is the length in source space or 4 * dx ((count - 1) * dx) 4 * dx ((count - 1) * dx) reads like one expression, not the same expression twice. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:409: // where dx is the distance between samples. There are 5 pixels specified in the A, B There are 5 destination pixels ... ? Or 5 samples? https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:520: // While the span hangs over the edge of the tile. Draw the span covering the tile then While the span hangs over the edge of the tile. ??
Handle large dx case.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address comments.
PTAL https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (left): https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:81: float fVal; On 2016/02/23 23:49:57, mtklein wrote: > SkScalar Done. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:99: float dx = length / (count - 1); On 2016/02/23 23:49:57, mtklein wrote: > Let's do a careful search for all 'float' in here. It's okay to use float for > colors, but we should use SkScalar for spaces. Done. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:18: #if defined(_MSC_VER) && SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 On 2016/02/23 23:49:57, mtklein wrote: > Seems like it's all the other lines that are wrong. Done. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:97: // Xs must be calculated in the following manner instead of dx * {0.0, 1.0, 2.0, 3.0} because On 2016/02/23 23:49:57, mtklein wrote: > Seems fine to branch on count too, if that makes things simpler and less > needs-explanation-y: > > Sk4s Xs{X(start)}; > Sk4s fourDx; > if (count > 1) { > // This math only works when count > 1, but luckily it's only _used_ when > count > 1. > SkScalar dx = length / (count-1); > Xs = Xs + Sk4f{0,1,2,3} * dx; > > // FWIW, this is only used when count >= 4. > fourDx = Sk4f{4} * dx; > } > > while (count >= 4) { > ... Done. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:408: // The length from A to B is the length in source space or 4 * dx ((count - 1) * dx) On 2016/02/23 23:49:57, mtklein wrote: > 4 * dx ((count - 1) * dx) reads like one expression, not the same expression > twice. Done. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:409: // where dx is the distance between samples. There are 5 pixels specified in the A, B On 2016/02/23 23:49:57, mtklein wrote: > There are 5 destination pixels ... ? Or 5 samples? Add explanation. https://codereview.chromium.org/1719333002/diff/80001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:520: // While the span hangs over the edge of the tile. Draw the span covering the tile then On 2016/02/23 23:49:57, mtklein wrote: > While the span hangs over the edge of the tile. > > ?? Fixed sentence fragment.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (left): https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:367: , fXMax{X(max) - 1.0f} Some of this stuff would be clearer if we stuck to ints / SkISize longer. https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:393: SkScalar dx = length / (count - 1); Watch out for NaN? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:396: return false; // TODO: left and right spans could still get some useful mileage out of spans here? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:428: SkScalar EdgeOrXEnd = std::min(xMin, x + length + 1.0f); +1 ? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:429: int underCount = SkScalarFloorToInt((EdgeOrXEnd - x) / dx) + 1; // There are n dx's between n+1 samples. (5 *'s above for 4 ---'s.) https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:429: int underCount = SkScalarFloorToInt((EdgeOrXEnd - x) / dx) + 1; Should be safe to assert (EdgeOrXEnd - x >= 0). https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:430: if (underCount > 0) { how can this not be at least 1? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:444: // If there are more pixels needed, sample from the middle of the tile. Three cases left: - all in the tile - some in the tile, and more off the right edge - all off the right edge are we handling the all off the right edge case? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:449: SkScalar middleLength = (middleCount - 1) * dx; // N samples span N-1 dx's. https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:473: // It would be nice to use fmod, but it uses trunc based rounding where floor rounding is needed. remove comment? https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:503: SkScalar dx = length / (count - 1); NaN https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:505: return false; // No point to spanning if we're not going to have multiple samples from the same tile. https://codereview.chromium.org/1719333002/diff/120001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:550: x += lengthToNextStart - xMax; // step forward the length we've consumed, then mod back to our tile?
Handle -dx.
didn't really look at repeat yet https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:373: bool inRange(SkScalar xMin, SkScalar xMax) const { -> completelyWithin? inRange seems ambiguous about whether partial overlap returns true or false. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:383: Span breakAt(SkScalar breakX, SkScalar dx) { I find the countToBreak() and lengthToBreakAndAdjust() helpers confusing. They seem like they either want to be - countToBreak() const - lengthToBreak() const - break() or all inlined here, with breakAtSingleColor() implemented by calling breakAt(). https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:386: int newCount = countToBreak(breakX, dx); all self calls need this-> https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:388: // The length is one dx less than the count * dx like the example above. There are this comment doesn't belong here given the code that surrounds it https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:395: Span breakAtSingleColor(SkScalar breakX, SkScalar dx) { I don't understand where colors are getting involved? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:407: next->pointSpan(fStart, fLength, fCount); why does this function exist? doesn't this suggest pointSpan() should take a span? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:426: // The length is one dx less than the count * dx like the example above. There are there is no example above https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:458: SkASSERT(std::isfinite(length)); We seem to assert this multiple times, but count > 0 only here. If we're going to assert an invariant about Spans, doing it in its constructor seems like a good consistent place. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:468: // Handle the Span is on the tile if xMin and xMax not infinities or handle the case where please make this a sentence or delete it? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:477: // This is only the case where the count is one and it is in x < xMin || xMax < x, ...and it it outside [xMin, xMax]? Is it really important to explain this here? It should be obvious that we want to avoid calculating dx if count == 1? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:479: if (1 == count || dx < 0) { if (count == 1 || length < 0) { return false; // Fallback to point-by-point tiling. } SkScalar dx = length / (count - 1); ? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:514: Span toDraw = span.breakAtSingleColor(xMin, dx); This breakAtSingleColor seems like something Span doesn't need to know about. It's only going to come up here in clamping right? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:514: Span toDraw = span.breakAtSingleColor(xMin, dx); We're not drawing anything here... https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:517: span.nextStage(next); I don't think I follow this case. It's entirely off the left, correct? Should we not be passing along a zero-length span at xMin to the next stage? https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:521: if (!span.isEmpty() && span.startX() < xMax) { All these if cases are really complicated. Seems like things would be simpler if we made 0-count spans legal (no-ops, but legal to ask for). Then we could just unconditionally break the span into three spans here in Clamp: a span at the left edge, a span in the tile, and a span at the right edge. Any of those might be empty. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:533: } else { How did we get dx == 0 here? Some sort of degenerate matrix? Why don't we just handle that case with fallback?
WIP: part way through span rewrite.
Spans working in tiles.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/180001
I'm going to try running spans through the entire stack. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:373: bool inRange(SkScalar xMin, SkScalar xMax) const { On 2016/02/25 21:47:41, mtklein wrote: > -> completelyWithin? inRange seems ambiguous about whether partial overlap > returns true or false. Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:383: Span breakAt(SkScalar breakX, SkScalar dx) { On 2016/02/25 21:47:41, mtklein wrote: > I find the countToBreak() and lengthToBreakAndAdjust() helpers confusing. They > seem like they either want to be > - countToBreak() const > - lengthToBreak() const > - break() > or all inlined here, with breakAtSingleColor() implemented by calling breakAt(). Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:386: int newCount = countToBreak(breakX, dx); On 2016/02/25 21:47:41, mtklein wrote: > all self calls need this-> Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:388: // The length is one dx less than the count * dx like the example above. There are On 2016/02/25 21:47:40, mtklein wrote: > this comment doesn't belong here given the code that surrounds it Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:395: Span breakAtSingleColor(SkScalar breakX, SkScalar dx) { On 2016/02/25 21:47:41, mtklein wrote: > I don't understand where colors are getting involved? Changed the way this is done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:407: next->pointSpan(fStart, fLength, fCount); On 2016/02/25 21:47:41, mtklein wrote: > why does this function exist? doesn't this suggest pointSpan() should take a > span? This is still a work in progress. Doing it now. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:426: // The length is one dx less than the count * dx like the example above. There are On 2016/02/25 21:47:41, mtklein wrote: > there is no example above Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:458: SkASSERT(std::isfinite(length)); On 2016/02/25 21:47:41, mtklein wrote: > We seem to assert this multiple times, but count > 0 only here. If we're going > to assert an invariant about Spans, doing it in its constructor seems like a > good consistent place. Acknowledged. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:468: // Handle the Span is on the tile if xMin and xMax not infinities or handle the case where On 2016/02/25 21:47:40, mtklein wrote: > please make this a sentence or delete it? Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:477: // This is only the case where the count is one and it is in x < xMin || xMax < x, On 2016/02/25 21:47:41, mtklein wrote: > ...and it it outside [xMin, xMax]? > > Is it really important to explain this here? It should be obvious that we want > to avoid calculating dx if count == 1? Acknowledged. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:479: if (1 == count || dx < 0) { On 2016/02/25 21:47:41, mtklein wrote: > if (count == 1 || length < 0) { > return false; // Fallback to point-by-point tiling. > } > SkScalar dx = length / (count - 1); > > ? Changed logic https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:514: Span toDraw = span.breakAtSingleColor(xMin, dx); On 2016/02/25 21:47:41, mtklein wrote: > We're not drawing anything here... Totally redid logic. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:517: span.nextStage(next); On 2016/02/25 21:47:41, mtklein wrote: > I don't think I follow this case. It's entirely off the left, correct? Should > we not be passing along a zero-length span at xMin to the next stage? Redid logic https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:521: if (!span.isEmpty() && span.startX() < xMax) { On 2016/02/25 21:47:40, mtklein wrote: > All these if cases are really complicated. Seems like things would be simpler > if we made 0-count spans legal (no-ops, but legal to ask for). > > Then we could just unconditionally break the span into three spans here in > Clamp: a span at the left edge, a span in the tile, and a span at the right > edge. Any of those might be empty. Done. https://codereview.chromium.org/1719333002/diff/140001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:533: } else { On 2016/02/25 21:47:41, mtklein wrote: > How did we get dx == 0 here? Some sort of degenerate matrix? Why don't we just > handle that case with fallback? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
Span used over entire pipeline.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fix comment.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
lgtm (l "g" tm) https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:131: void spanNextStage(Next* next) { Let's consider refactoring to remove this. Seems roundabout. https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:483: if (1 == count || 0.0f == length) { SkASSERT(count > 0); ? // We probably could make length == 0 work here, but there's no real reason to do so. We can't break a length-zero span into other spans. https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:536: Span middle = span.breakAt(xMax, dx); xMin (moar tests plz) https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:589: SkScalar dx = length / (count - 1); fallback for count < 2? // Unlike in the Clamp case, our main logic handles length == 0 just fine, so no need to check for that special case.
Fix repeat boundary condition.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/200002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/200002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address remaining comments.
https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:131: void spanNextStage(Next* next) { On 2016/02/26 23:07:40, mtklein_C wrote: > Let's consider refactoring to remove this. Seems roundabout. Done. https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:483: if (1 == count || 0.0f == length) { On 2016/02/26 23:07:40, mtklein_C wrote: > SkASSERT(count > 0); ? > > > // We probably could make length == 0 work here, but there's no real reason to > do so. We can't break a length-zero span into other spans. I think the best direction here is to have spans be able to represent 0 samples, but you are not allowed to pass an empty span to pointSpan. I have added the needed asserts. https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:536: Span middle = span.breakAt(xMax, dx); On 2016/02/26 23:07:40, mtklein_C wrote: > xMin > > (moar tests plz) Done. Tests to follow. https://codereview.chromium.org/1719333002/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:589: SkScalar dx = length / (count - 1); On 2016/02/26 23:07:40, mtklein_C wrote: > fallback for count < 2? > > // Unlike in the Clamp case, our main logic handles length == 0 just fine, so no > need to check for that special case. Done.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/250001
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 herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/1719333002/#ps250001 (title: "Address remaining comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719333002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719333002/250001
Message was sent while issue was closed.
Description was changed from ========== tile spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== tile spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/9466571f8ebde1429cca7b6dcd2d4b85bad1b088 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001) as https://skia.googlesource.com/skia/+/9466571f8ebde1429cca7b6dcd2d4b85bad1b088 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
