Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887103003:
spriteblitter for memcpy case (for all configs) (Closed)
Descriptionspriteblitter for memcpy case (for all configs)
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1887103003
Committed: https://skia.googlesource.com/skia/+/8c3fd4f1b49a77634b73f787c6c49d34039a16f0
Patch Set 1 #Patch Set 2 : fix shiftPerPixel #
Total comments: 6
Patch Set 3 : #
Total comments: 1
Patch Set 4 : use normal left shift operator #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== spriteblitter for memcpy case (for all configs) BUG=skia: ========== to ========== spriteblitter for memcpy case (for all configs) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by reed@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/1887103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/1
reed@google.com changed reviewers: + herb@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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/1887103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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/1887103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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/1887103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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/1887103003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/20001
fixed the crasher shiftPerPixel()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + fmalita@chromium.org, mtklein@google.com
ptal
https://codereview.chromium.org/1887103003/diff/20001/include/core/SkPixmap.h File include/core/SkPixmap.h (left): https://codereview.chromium.org/1887103003/diff/20001/include/core/SkPixmap.h... include/core/SkPixmap.h:83: int shiftPerPixel() const { return fInfo.bytesPerPixel() >> 1; } :) It had to break down at some point. https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... File src/core/SkBlitter_Sprite.cpp (right): https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... src/core/SkBlitter_Sprite.cpp:82: void* dst = fDst.writable_addr(x, y); It might be clearer to cast to char* early: auto dst = ( char*)fDst.writeable_addr(x,y); auto src = (const char*)fSource.addr(x-fLeft, y-fTop); size_t dstRB = ...; size_t srcRB = ...; size_t bytesToCopy = ...; while (height --> 0) { memcpy(dst, src, bytesToCopy); dst += dstRB; src += srcRB; } https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... src/core/SkBlitter_Sprite.cpp:86: size_t bytesToCopy = SkLeftShift(width, fSource.shiftPerPixel()); Seems like it's not sensible for width to be negative here, so I am skeptical of SkLeftShift. We've asserted that width is positive, and if it were negative, bytesToCopy would become impossibly large. Might want to write this the old usual way, width << fSource.shiftPerPixel() which would let ubsan point out any negative values should that case ever come up. (The fix wouldn't be use SkLeftShift... it'd be to figure out how width got negative.)
https://codereview.chromium.org/1887103003/diff/20001/include/core/SkPixmap.h File include/core/SkPixmap.h (left): https://codereview.chromium.org/1887103003/diff/20001/include/core/SkPixmap.h... include/core/SkPixmap.h:83: int shiftPerPixel() const { return fInfo.bytesPerPixel() >> 1; } On 2016/04/15 13:29:21, mtklein wrote: > :) It had to break down at some point. :( yep https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... File src/core/SkBlitter_Sprite.cpp (right): https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... src/core/SkBlitter_Sprite.cpp:82: void* dst = fDst.writable_addr(x, y); On 2016/04/15 13:29:21, mtklein wrote: > It might be clearer to cast to char* early: > > auto dst = ( char*)fDst.writeable_addr(x,y); > auto src = (const char*)fSource.addr(x-fLeft, y-fTop); > > size_t dstRB = ...; > size_t srcRB = ...; > size_t bytesToCopy = ...; > > while (height --> 0) { > memcpy(dst, src, bytesToCopy); > dst += dstRB; > src += srcRB; > } > Done. https://codereview.chromium.org/1887103003/diff/20001/src/core/SkBlitter_Spri... src/core/SkBlitter_Sprite.cpp:86: size_t bytesToCopy = SkLeftShift(width, fSource.shiftPerPixel()); On 2016/04/15 13:29:21, mtklein wrote: > Seems like it's not sensible for width to be negative here, so I am skeptical of > SkLeftShift. We've asserted that width is positive, and if it were negative, > bytesToCopy would become impossibly large. Might want to write this the old > usual way, > > width << fSource.shiftPerPixel() > > which would let ubsan point out any negative values should that case ever come > up. (The fix wouldn't be use SkLeftShift... it'd be to figure out how width got > negative.) Done.
The CQ bit was checked by reed@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/1887103003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/60001
https://codereview.chromium.org/1887103003/diff/40001/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/1887103003/diff/40001/include/core/SkImageInf... include/core/SkImageInfo.h:109: static const uint8_t gShift[] = { Isn't this going to add the static array to every compilation unit which makes use of it? (ditto for gSize above) nm out/Release/SampleApp | grep gSize 000000000083a5e0 T _ZN10SkWriter3215WriteStringSizeEPKcm 0000000000bdc950 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bdd4a0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bde841 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bdffa0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be0320 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be1080 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be15c7 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be1708 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be1e0b r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be3663 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be3bf8 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be4068 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be4ba0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be93d0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bf059c r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bf8968 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000bf8aa0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cac990 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cad318 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cae9b8 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000be2af5 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c04f48 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c812c0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c81870 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c81e30 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c82088 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c833d0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c84328 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c85110 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c85119 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c851b8 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c86390 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c86970 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c89130 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c89408 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8aa90 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8ac30 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8b608 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8cb20 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8cd18 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8d508 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8dc90 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8f550 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8f618 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8fb50 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8fc10 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000c8fe60 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cab978 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cbbb90 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cbf198 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000cca6d0 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 0000000000ccaa28 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize 00000000016b6fd4 r _ZZL24SkColorTypeBytesPerPixel11SkColorTypeE5gSize
lgtm
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887103003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887103003/60001
Message was sent while issue was closed.
Description was changed from ========== spriteblitter for memcpy case (for all configs) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== spriteblitter for memcpy case (for all configs) 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/+/8c3fd4f1b49a77634b73f787c6c49d34039a16f0 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/8c3fd4f1b49a77634b73f787c6c49d34039a16f0 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
