Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1751943002:
Introduce bilerp spans (Closed)
DescriptionIntroduce bilerp spans
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1751943002
Committed: https://skia.googlesource.com/skia/+/1ef8972ee15899996c4a8b208c471a94cce8dd1c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #Patch Set 3 : Sync #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Introduce bilerp spans BUG=skia: ========== to ========== Introduce bilerp spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Introduce bilerp spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Introduce bilerp 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/1751943002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751943002/1
This is the introduction of a bilerp span. It basically, introduces the interface, but all the implementations fallback.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:135: Let'd document what a BilerpSpan represents, especially how it differs from a Span. I think the major first question that comes up is, why do we need two y coordinates? Do they somehow represent the range (y0,y1), or is this a compact way to represent two start points (x,y0), (x,y1)? https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:225: Stray newline. https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:229: float dx = length / (count - 1); Do we not need the usual inf/NaN worries here, or does SkASSERT(!span.isEmpty()) cover that? If so, maybe move it down a bit and add some whitespace to make it clear it is dx's precondition? SkScalar x, y0, y1; SkScalar length; int count; std::tie(x, y0, y1, length, count) = span; SkASSERT(!span.isEmpty()); float dx = length / (count - 1); ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Address comments.
Sync
https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:135: On 2016/03/01 16:34:09, mtklein_C wrote: > Let'd document what a BilerpSpan represents, especially how it differs from a > Span. > > I think the major first question that comes up is, why do we need two y > coordinates? Do they somehow represent the range (y0,y1), or is this a compact > way to represent two start points (x,y0), (x,y1)? Done. https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:225: On 2016/03/01 16:34:09, mtklein_C wrote: > Stray newline. Done. https://codereview.chromium.org/1751943002/diff/1/src/core/SkLinearBitmapPipe... src/core/SkLinearBitmapPipeline.cpp:229: float dx = length / (count - 1); On 2016/03/01 16:34:09, mtklein_C wrote: > Do we not need the usual inf/NaN worries here, or does SkASSERT(!span.isEmpty()) > cover that? If so, maybe move it down a bit and add some whitespace to make it > clear it is dx's precondition? > > SkScalar x, y0, y1; SkScalar length; int count; > std::tie(x, y0, y1, length, count) = span; > > SkASSERT(!span.isEmpty()); > float dx = length / (count - 1); > > ... 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/1751943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751943002/40001
lgtm
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/1751943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751943002/40001
Message was sent while issue was closed.
Description was changed from ========== Introduce bilerp spans BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Introduce bilerp 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/+/1ef8972ee15899996c4a8b208c471a94cce8dd1c ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/1ef8972ee15899996c4a8b208c471a94cce8dd1c |
