Remove unused PackBits methods and fix length checks
Also a bit of general cleanup.
BUG=chromium:486944
Review URL: https://codereview.chromium.org/1152163004
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true
Review URL: https://codereview.chromium.org/1235163002
diff --git a/include/core/SkPackBits.h b/include/core/SkPackBits.h
index f0614a0..1e32ee0 100644
--- a/include/core/SkPackBits.h
+++ b/include/core/SkPackBits.h
@@ -14,11 +14,6 @@
class SkPackBits {
public:
- /** Given the number of 16bit values that will be passed to Pack16,
- returns the worst-case size needed for the dst[] buffer.
- */
- static size_t ComputeMaxSize16(int count);
-
/** Given the number of 8bit values that will be passed to Pack8,
returns the worst-case size needed for the dst[] buffer.
*/
@@ -26,54 +21,26 @@
/** Write the src array into a packed format. The packing process may end
up writing more bytes than it read, so dst[] must be large enough.
- @param src Input array of 16bit values
- @param count Number of entries in src[]
- @param dst Buffer (allocated by caller) to write the packed data
- into
- @return the number of bytes written to dst[]
- */
- static size_t Pack16(const uint16_t src[], int count, uint8_t dst[]);
-
- /** Write the src array into a packed format. The packing process may end
- up writing more bytes than it read, so dst[] must be large enough.
@param src Input array of 8bit values
- @param count Number of entries in src[]
+ @param srcSize Number of entries in src[]
@param dst Buffer (allocated by caller) to write the packed data
into
+ @param dstSize Number of bytes in the output buffer.
@return the number of bytes written to dst[]
*/
- static size_t Pack8(const uint8_t src[], int count, uint8_t dst[]);
-
- /** Unpack the data in src[], and expand it into dst[]. The src[] data was
- written by a previous call to Pack16.
- @param src Input data to unpack, previously created by Pack16.
- @param srcSize Number of bytes of src to unpack
- @param dst Buffer (allocated by caller) to expand the src[] into.
- @return the number of dst elements (not bytes) written into dst.
- */
- static int Unpack16(const uint8_t src[], size_t srcSize, uint16_t dst[]);
+ static size_t Pack8(const uint8_t src[], size_t srcSize, uint8_t dst[],
+ size_t dstSize);
/** Unpack the data in src[], and expand it into dst[]. The src[] data was
written by a previous call to Pack8.
@param src Input data to unpack, previously created by Pack8.
@param srcSize Number of bytes of src to unpack
@param dst Buffer (allocated by caller) to expand the src[] into.
+ @param dstSize Number of bytes in the output buffer.
@return the number of bytes written into dst.
*/
- static int Unpack8(const uint8_t src[], size_t srcSize, uint8_t dst[]);
-
- /** Unpack the data from src[], skip the first dstSkip bytes, then write
- dstWrite bytes into dst[]. The src[] data was written by a previous
- call to Pack8. Return the number of bytes actually writtten into dst[]
- @param src Input data to unpack, previously created by Pack8.
- @param dst Buffer (allocated by caller) to expand the src[] into.
- @param dstSkip Number of bytes of unpacked src to skip before writing
- into dst
- @param dstWrite Number of bytes of unpacked src to write into dst (after
- skipping dstSkip bytes)
- */
- static void Unpack8(uint8_t dst[], size_t dstSkip, size_t dstWrite,
- const uint8_t src[]);
+ static int Unpack8(const uint8_t src[], size_t srcSize, uint8_t dst[],
+ size_t dstSize);
};
#endif
diff --git a/src/core/SkPackBits.cpp b/src/core/SkPackBits.cpp
index 3c6197b..a3424e2 100644
--- a/src/core/SkPackBits.cpp
+++ b/src/core/SkPackBits.cpp
@@ -1,4 +1,3 @@
-
/*
* Copyright 2011 Google Inc.
*
@@ -7,150 +6,14 @@
*/
#include "SkPackBits.h"
-#define GATHER_STATSx
-
-static inline void small_memcpy(void* SK_RESTRICT dst,
- const void* SK_RESTRICT src, size_t n) {
- SkASSERT(n > 0 && n <= 15);
- uint8_t* d = (uint8_t*)dst;
- const uint8_t* s = (const uint8_t*)src;
- switch (n) {
- case 15: *d++ = *s++;
- case 14: *d++ = *s++;
- case 13: *d++ = *s++;
- case 12: *d++ = *s++;
- case 11: *d++ = *s++;
- case 10: *d++ = *s++;
- case 9: *d++ = *s++;
- case 8: *d++ = *s++;
- case 7: *d++ = *s++;
- case 6: *d++ = *s++;
- case 5: *d++ = *s++;
- case 4: *d++ = *s++;
- case 3: *d++ = *s++;
- case 2: *d++ = *s++;
- case 1: *d++ = *s++;
- case 0: break;
- }
-}
-
-static inline void small_memset(void* dst, uint8_t value, size_t n) {
- SkASSERT(n > 0 && n <= 15);
- uint8_t* d = (uint8_t*)dst;
- switch (n) {
- case 15: *d++ = value;
- case 14: *d++ = value;
- case 13: *d++ = value;
- case 12: *d++ = value;
- case 11: *d++ = value;
- case 10: *d++ = value;
- case 9: *d++ = value;
- case 8: *d++ = value;
- case 7: *d++ = value;
- case 6: *d++ = value;
- case 5: *d++ = value;
- case 4: *d++ = value;
- case 3: *d++ = value;
- case 2: *d++ = value;
- case 1: *d++ = value;
- case 0: break;
- }
-}
-
-// can we do better for small counts with our own inlined memcpy/memset?
-
-#define PB_MEMSET(addr, value, count) \
-do { \
-if ((count) > 15) { \
-memset(addr, value, count); \
-} else { \
-small_memset(addr, value, count); \
-} \
-} while (0)
-
-#define PB_MEMCPY(dst, src, count) \
-do { \
- if ((count) > 15) { \
- memcpy(dst, src, count); \
- } else { \
- small_memcpy(dst, src, count); \
- } \
-} while (0)
-
-///////////////////////////////////////////////////////////////////////////////
-
-#ifdef GATHER_STATS
- static int gMemSetBuckets[129];
- static int gMemCpyBuckets[129];
- static int gCounter;
-
-static void register_memset_count(int n) {
- SkASSERT((unsigned)n <= 128);
- gMemSetBuckets[n] += 1;
- gCounter += 1;
-
- if ((gCounter & 0xFF) == 0) {
- SkDebugf("----- packbits memset stats: ");
- for (size_t i = 0; i < SK_ARRAY_COUNT(gMemSetBuckets); i++) {
- if (gMemSetBuckets[i]) {
- SkDebugf(" %d:%d", i, gMemSetBuckets[i]);
- }
- }
- }
-}
-static void register_memcpy_count(int n) {
- SkASSERT((unsigned)n <= 128);
- gMemCpyBuckets[n] += 1;
- gCounter += 1;
-
- if ((gCounter & 0x1FF) == 0) {
- SkDebugf("----- packbits memcpy stats: ");
- for (size_t i = 0; i < SK_ARRAY_COUNT(gMemCpyBuckets); i++) {
- if (gMemCpyBuckets[i]) {
- SkDebugf(" %d:%d", i, gMemCpyBuckets[i]);
- }
- }
- }
-}
-#else
-#define register_memset_count(n)
-#define register_memcpy_count(n)
-#endif
-
-
-///////////////////////////////////////////////////////////////////////////////
-
-size_t SkPackBits::ComputeMaxSize16(int count) {
- // worst case is the number of 16bit values (times 2) +
- // 1 byte per (up to) 128 entries.
- return ((count + 127) >> 7) + (count << 1);
-}
-
size_t SkPackBits::ComputeMaxSize8(int count) {
// worst case is the number of 8bit values + 1 byte per (up to) 128 entries.
return ((count + 127) >> 7) + count;
}
-static uint8_t* flush_same16(uint8_t dst[], uint16_t value, int count) {
+static uint8_t* flush_same8(uint8_t dst[], uint8_t value, size_t count) {
while (count > 0) {
- int n = count;
- if (n > 128) {
- n = 128;
- }
- *dst++ = (uint8_t)(n - 1);
- *dst++ = (uint8_t)(value >> 8);
- *dst++ = (uint8_t)value;
- count -= n;
- }
- return dst;
-}
-
-static uint8_t* flush_same8(uint8_t dst[], uint8_t value, int count) {
- while (count > 0) {
- int n = count;
- if (n > 128) {
- n = 128;
- }
+ int n = count > 128 ? 128 : count;
*dst++ = (uint8_t)(n - 1);
*dst++ = (uint8_t)value;
count -= n;
@@ -158,31 +21,12 @@
return dst;
}
-static uint8_t* flush_diff16(uint8_t* SK_RESTRICT dst,
- const uint16_t* SK_RESTRICT src, int count) {
- while (count > 0) {
- int n = count;
- if (n > 128) {
- n = 128;
- }
- *dst++ = (uint8_t)(n + 127);
- PB_MEMCPY(dst, src, n * sizeof(uint16_t));
- src += n;
- dst += n * sizeof(uint16_t);
- count -= n;
- }
- return dst;
-}
-
static uint8_t* flush_diff8(uint8_t* SK_RESTRICT dst,
- const uint8_t* SK_RESTRICT src, int count) {
+ const uint8_t* SK_RESTRICT src, size_t count) {
while (count > 0) {
- int n = count;
- if (n > 128) {
- n = 128;
- }
+ int n = count > 128 ? 128 : count;
*dst++ = (uint8_t)(n + 127);
- PB_MEMCPY(dst, src, n);
+ memcpy(dst, src, n);
src += n;
dst += n;
count -= n;
@@ -190,64 +34,20 @@
return dst;
}
-size_t SkPackBits::Pack16(const uint16_t* SK_RESTRICT src, int count,
- uint8_t* SK_RESTRICT dst) {
- uint8_t* origDst = dst;
- const uint16_t* stop = src + count;
-
- for (;;) {
- count = SkToInt(stop - src);
- SkASSERT(count >= 0);
- if (count == 0) {
- return dst - origDst;
- }
- if (1 == count) {
- *dst++ = 0;
- *dst++ = (uint8_t)(*src >> 8);
- *dst++ = (uint8_t)*src;
- return dst - origDst;
- }
-
- unsigned value = *src;
- const uint16_t* s = src + 1;
-
- if (*s == value) { // accumulate same values...
- do {
- s++;
- if (s == stop) {
- break;
- }
- } while (*s == value);
- dst = flush_same16(dst, value, SkToInt(s - src));
- } else { // accumulate diff values...
- do {
- if (++s == stop) {
- goto FLUSH_DIFF;
- }
- } while (*s != s[-1]);
- s -= 1; // back up so we don't grab one of the "same" values that follow
- FLUSH_DIFF:
- dst = flush_diff16(dst, src, SkToInt(s - src));
- }
- src = s;
+size_t SkPackBits::Pack8(const uint8_t* SK_RESTRICT src, size_t srcSize,
+ uint8_t* SK_RESTRICT dst, size_t dstSize) {
+ if (dstSize < ComputeMaxSize8(srcSize)) {
+ return 0;
}
-}
-size_t SkPackBits::Pack8(const uint8_t* SK_RESTRICT src, int count,
- uint8_t* SK_RESTRICT dst) {
- uint8_t* origDst = dst;
- const uint8_t* stop = src + count;
+ uint8_t* const origDst = dst;
+ const uint8_t* stop = src + srcSize;
- for (;;) {
- count = SkToInt(stop - src);
- SkASSERT(count >= 0);
- if (count == 0) {
- return dst - origDst;
- }
+ for (intptr_t count = stop - src; count > 0; count = stop - src) {
if (1 == count) {
*dst++ = 0;
*dst++ = *src;
- return dst - origDst;
+ break;
}
unsigned value = *src;
@@ -275,137 +75,35 @@
}
src = s;
}
+ return dst - origDst;
}
#include "SkUtils.h"
-int SkPackBits::Unpack16(const uint8_t* SK_RESTRICT src, size_t srcSize,
- uint16_t* SK_RESTRICT dst) {
- uint16_t* origDst = dst;
- const uint8_t* stop = src + srcSize;
-
- while (src < stop) {
- unsigned n = *src++;
- if (n <= 127) { // repeat count (n + 1)
- n += 1;
- sk_memset16(dst, (src[0] << 8) | src[1], n);
- src += 2;
- } else { // same count (n - 127)
- n -= 127;
- PB_MEMCPY(dst, src, n * sizeof(uint16_t));
- src += n * sizeof(uint16_t);
- }
- dst += n;
- }
- SkASSERT(src == stop);
- return SkToInt(dst - origDst);
-}
-
int SkPackBits::Unpack8(const uint8_t* SK_RESTRICT src, size_t srcSize,
- uint8_t* SK_RESTRICT dst) {
- uint8_t* origDst = dst;
+ uint8_t* SK_RESTRICT dst, size_t dstSize) {
+ uint8_t* const origDst = dst;
+ uint8_t* const endDst = dst + dstSize;
const uint8_t* stop = src + srcSize;
while (src < stop) {
unsigned n = *src++;
if (n <= 127) { // repeat count (n + 1)
n += 1;
- PB_MEMSET(dst, *src++, n);
+ if (dst >(endDst - n)) {
+ return 0;
+ }
+ memset(dst, *src++, n);
} else { // same count (n - 127)
n -= 127;
- PB_MEMCPY(dst, src, n);
+ if (dst > (endDst - n)) {
+ return 0;
+ }
+ memcpy(dst, src, n);
src += n;
}
dst += n;
}
- SkASSERT(src == stop);
+ SkASSERT(src <= stop);
return SkToInt(dst - origDst);
}
-
-enum UnpackState {
- CLEAN_STATE,
- REPEAT_BYTE_STATE,
- COPY_SRC_STATE
-};
-
-void SkPackBits::Unpack8(uint8_t* SK_RESTRICT dst, size_t dstSkip,
- size_t dstWrite, const uint8_t* SK_RESTRICT src) {
- if (dstWrite == 0) {
- return;
- }
-
- UnpackState state = CLEAN_STATE;
- size_t stateCount = 0;
-
- // state 1: do the skip-loop
- while (dstSkip > 0) {
- size_t n = *src++;
- if (n <= 127) { // repeat count (n + 1)
- n += 1;
- if (n > dstSkip) {
- state = REPEAT_BYTE_STATE;
- stateCount = n - dstSkip;
- n = dstSkip;
- // we don't increment src here, since its needed in stage 2
- } else {
- src++; // skip the src byte
- }
- } else { // same count (n - 127)
- n -= 127;
- if (n > dstSkip) {
- state = COPY_SRC_STATE;
- stateCount = n - dstSkip;
- n = dstSkip;
- }
- src += n;
- }
- dstSkip -= n;
- }
-
- // stage 2: perform any catchup from the skip-stage
- if (stateCount > dstWrite) {
- stateCount = dstWrite;
- }
- switch (state) {
- case REPEAT_BYTE_STATE:
- SkASSERT(stateCount > 0);
- register_memset_count(stateCount);
- PB_MEMSET(dst, *src++, stateCount);
- break;
- case COPY_SRC_STATE:
- SkASSERT(stateCount > 0);
- register_memcpy_count(stateCount);
- PB_MEMCPY(dst, src, stateCount);
- src += stateCount;
- break;
- default:
- SkASSERT(stateCount == 0);
- break;
- }
- dst += stateCount;
- dstWrite -= stateCount;
-
- // copy at most dstWrite bytes into dst[]
- while (dstWrite > 0) {
- size_t n = *src++;
- if (n <= 127) { // repeat count (n + 1)
- n += 1;
- if (n > dstWrite) {
- n = dstWrite;
- }
- register_memset_count(n);
- PB_MEMSET(dst, *src++, n);
- } else { // same count (n - 127)
- n -= 127;
- if (n > dstWrite) {
- n = dstWrite;
- }
- register_memcpy_count(n);
- PB_MEMCPY(dst, src, n);
- src += n;
- }
- dst += n;
- dstWrite -= n;
- }
- SkASSERT(0 == dstWrite);
-}
diff --git a/src/effects/SkTableColorFilter.cpp b/src/effects/SkTableColorFilter.cpp
index 7298960..8e54a01 100644
--- a/src/effects/SkTableColorFilter.cpp
+++ b/src/effects/SkTableColorFilter.cpp
@@ -201,8 +201,8 @@
void SkTable_ColorFilter::flatten(SkWriteBuffer& buffer) const {
uint8_t storage[5*256];
int count = gCountNibBits[fFlags & 0xF];
- size_t size = SkPackBits::Pack8(fStorage, count * 256, storage);
- SkASSERT(size <= sizeof(storage));
+ size_t size = SkPackBits::Pack8(fStorage, count * 256, storage,
+ sizeof(storage));
buffer.write32(fFlags);
buffer.writeByteArray(storage, size);
@@ -223,7 +223,8 @@
}
uint8_t unpackedStorage[4*256];
- size_t unpackedSize = SkPackBits::Unpack8(packedStorage, packedSize, unpackedStorage);
+ size_t unpackedSize = SkPackBits::Unpack8(packedStorage, packedSize,
+ unpackedStorage, sizeof(unpackedStorage));
// now check that we got the size we expected
if (!buffer.validate(unpackedSize == count*256)) {
return NULL;
diff --git a/tests/PackBitsTest.cpp b/tests/PackBitsTest.cpp
index ce4e8be..ac9a0ae 100644
--- a/tests/PackBitsTest.cpp
+++ b/tests/PackBitsTest.cpp
@@ -8,57 +8,8 @@
#include "SkPackBits.h"
#include "Test.h"
-static const uint16_t gTest0[] = { 0, 0, 1, 1 };
-static const uint16_t gTest1[] = { 1, 2, 3, 4, 5, 6 };
-static const uint16_t gTest2[] = { 0, 0, 0, 1, 2, 3, 3, 3 };
-static const uint16_t gTest3[] = { 0, 0, 0, 0, 0, 0, 1, 2, 3, 3, 3, 0, 0, 1 };
-
#include "SkRandom.h"
static SkRandom gRand;
-static void rand_fill(uint16_t buffer[], int count) {
- for (int i = 0; i < count; i++)
- buffer[i] = (uint16_t)gRand.nextU();
-}
-
-static void test_pack16(skiatest::Reporter* reporter) {
- static const struct {
- const uint16_t* fSrc;
- int fCount;
- } gTests[] = {
- { gTest0, SK_ARRAY_COUNT(gTest0) },
- { gTest1, SK_ARRAY_COUNT(gTest1) },
- { gTest2, SK_ARRAY_COUNT(gTest2) },
- { gTest3, SK_ARRAY_COUNT(gTest3) }
- };
-
- for (size_t i = 0; i < SK_ARRAY_COUNT(gTests); i++) {
- uint8_t dst[100];
- size_t dstSize = SkPackBits::Pack16(gTests[i].fSrc,
- gTests[i].fCount, dst);
- uint16_t src[100];
- int srcCount = SkPackBits::Unpack16(dst, dstSize, src);
- bool match = gTests[i].fCount == srcCount && memcmp(gTests[i].fSrc, src,
- gTests[i].fCount * sizeof(uint16_t)) == 0;
- REPORTER_ASSERT(reporter, match);
- }
-
- for (int n = 1000; n; n--) {
- int size = 50;
- uint16_t src[100], src2[100];
- uint8_t dst[200];
- rand_fill(src, size);
-
- size_t dstSize = SkPackBits::Pack16(src, size, dst);
- size_t maxSize = SkPackBits::ComputeMaxSize16(size);
- REPORTER_ASSERT(reporter, maxSize >= dstSize);
-
- int srcCount = SkPackBits::Unpack16(dst, dstSize, src2);
- REPORTER_ASSERT(reporter, size == srcCount);
- bool match = memcmp(src, src2, size * sizeof(uint16_t)) == 0;
- REPORTER_ASSERT(reporter, match);
- }
-}
-
static const uint8_t gTest80[] = { 0, 0, 1, 1 };
static const uint8_t gTest81[] = { 1, 2, 3, 4, 5, 6 };
static const uint8_t gTest82[] = { 0, 0, 0, 1, 2, 3, 3, 3 };
@@ -86,10 +37,15 @@
uint8_t dst[100];
size_t maxSize = SkPackBits::ComputeMaxSize8(gTests[i].fCount);
size_t dstSize = SkPackBits::Pack8(gTests[i].fSrc,
- gTests[i].fCount, dst);
+ gTests[i].fCount, dst, maxSize - 1);
+ REPORTER_ASSERT(reporter, dstSize == 0);
+ dstSize = SkPackBits::Pack8(gTests[i].fSrc,
+ gTests[i].fCount, dst, sizeof(dst));
REPORTER_ASSERT(reporter, dstSize <= maxSize);
uint8_t src[100];
- int srcCount = SkPackBits::Unpack8(dst, dstSize, src);
+ int srcCount = SkPackBits::Unpack8(dst, dstSize, src, gTests[i].fCount - 1);
+ REPORTER_ASSERT(reporter, srcCount == 0);
+ srcCount = SkPackBits::Unpack8(dst, dstSize, src, sizeof(src));
bool match = gTests[i].fCount == srcCount &&
memcmp(gTests[i].fSrc, src,
gTests[i].fCount * sizeof(uint8_t)) == 0;
@@ -102,30 +58,18 @@
uint8_t dst[600];
rand_fill(src, size);
- size_t dstSize = SkPackBits::Pack8(src, size, dst);
+ size_t dstSize = SkPackBits::Pack8(src, size, dst, sizeof(dst));
size_t maxSize = SkPackBits::ComputeMaxSize8(size);
REPORTER_ASSERT(reporter, maxSize >= dstSize);
- size_t srcCount = SkPackBits::Unpack8(dst, dstSize, src2);
+ size_t srcCount = SkPackBits::Unpack8(dst, dstSize, src2, size);
REPORTER_ASSERT(reporter, size == srcCount);
bool match = memcmp(src, src2, size * sizeof(uint8_t)) == 0;
REPORTER_ASSERT(reporter, match);
-
- for (int j = 0; j < 100; j++) {
- uint32_t skip = gRand.nextU() % size;
- uint32_t write = gRand.nextU() % size;
- if (skip + write > size) {
- write = size - skip;
- }
- SkPackBits::Unpack8(src, skip, write, dst);
- bool match = memcmp(src, src2 + skip, write) == 0;
- REPORTER_ASSERT(reporter, match);
- }
}
}
}
DEF_TEST(PackBits, reporter) {
test_pack8(reporter);
- test_pack16(reporter);
}
