Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1743123004:
Handle spans in sampling. (Closed)
DescriptionHandle spans in sampling.
Older version copied over.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1743123004
Committed: https://skia.googlesource.com/skia/+/7dd5db43a2eb0f43437ebc37f5c01b481cd53839
Patch Set 1 #Patch Set 2 : Add assert #Patch Set 3 : Sync in the swizzle changes. #
Total comments: 14
Patch Set 4 : Address comments. #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Handle spans in sampling. Older version copied over. BUG=skia: ========== to ========== Handle spans in sampling. Older version copied over. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Add assert
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/1743123004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1743123004/20001
Description was changed from ========== Handle spans in sampling. Older version copied over. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Handle spans in sampling. Older version copied over. 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sync in the swizzle changes.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:843: pointSpanSlowRate(span); Please do a this-> pass. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:853: void pointSpanSlowRate(Span span) { // When moving through source space more slowly than dst space (zoomed in), // we'll be sampling from the same source pixel more than once. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:861: const uint32_t* const row = fStrategy.row((int)std::floor(Y(start))); Seems like we can't logically write uint32_t here. That's something that's only true for the Pixel8888 strategy. Other strategies will have different formats, no? Better either go to void* everywhere, or use enough auto to make this code agnostic? I'm on board if you want to change this when we add our first non-32 bit source format. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:862: SkLinearBitmapPipeline::PixelPlacerInterface* const next = fNext; Just my opinion: I think marking pointers const usually just creates a readability stumbling block, and only rarely conveys some important reassurance. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:866: auto getNextPixel = [&]() { // As dx gets smaller, we take more and more samples from the same source pixel, // so with a little extra bookkeeping (all the SkFixed) we can avoid reloading // and re-format-converting each sample after the first. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:889: void pointSpanUnitRate(Span span) { // We're moving through source space at a rate of 1 source pixel per 1 dst pixel. // We'll never re-use pixels, but we can at least load contiguous pixels. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:909: void pointSpanFastRate(Span span) { // We're moving through source space faster than dst (zoomed out), // so we'll never reuse a source pixel or be able to do contiguous loads.
Address comments.
https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:843: pointSpanSlowRate(span); On 2016/03/01 18:53:05, mtklein_C wrote: > Please do a this-> pass. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:853: void pointSpanSlowRate(Span span) { On 2016/03/01 18:53:05, mtklein_C wrote: > // When moving through source space more slowly than dst space (zoomed in), > // we'll be sampling from the same source pixel more than once. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:861: const uint32_t* const row = fStrategy.row((int)std::floor(Y(start))); On 2016/03/01 18:53:05, mtklein_C wrote: > Seems like we can't logically write uint32_t here. That's something that's only > true for the Pixel8888 strategy. Other strategies will have different formats, > no? Better either go to void* everywhere, or use enough auto to make this code > agnostic? > > I'm on board if you want to change this when we add our first non-32 bit source > format. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:862: SkLinearBitmapPipeline::PixelPlacerInterface* const next = fNext; On 2016/03/01 18:53:06, mtklein_C wrote: > Just my opinion: I think marking pointers const usually just creates a > readability stumbling block, and only rarely conveys some important reassurance. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:866: auto getNextPixel = [&]() { On 2016/03/01 18:53:05, mtklein_C wrote: > // As dx gets smaller, we take more and more samples from the same source pixel, > // so with a little extra bookkeeping (all the SkFixed) we can avoid reloading > // and re-format-converting each sample after the first. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:889: void pointSpanUnitRate(Span span) { On 2016/03/01 18:53:05, mtklein_C wrote: > // We're moving through source space at a rate of 1 source pixel per 1 dst > pixel. > // We'll never re-use pixels, but we can at least load contiguous pixels. Done. https://codereview.chromium.org/1743123004/diff/40001/src/core/SkLinearBitmap... src/core/SkLinearBitmapPipeline.cpp:909: void pointSpanFastRate(Span span) { On 2016/03/01 18:53:06, mtklein_C wrote: > // We're moving through source space faster than dst (zoomed out), > // so we'll never reuse a source pixel or be able to do contiguous loads. 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/1743123004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1743123004/60001
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/1743123004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1743123004/60001
Message was sent while issue was closed.
Description was changed from ========== Handle spans in sampling. Older version copied over. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Handle spans in sampling. Older version copied over. 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/+/7dd5db43a2eb0f43437ebc37f5c01b481cd53839 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/7dd5db43a2eb0f43437ebc37f5c01b481cd53839 |
