|
|
|
Allow copying an Index8 bitmap when srcConfig and dstConfig are both
Index8.
Also, change the logic of SkBitmap.copyTo() to do memcpy() if srcConfig
and dstConfig are the same.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add Patch Set 2 #
Total comments: 4
Patch Set 3 : Done with the pervious comments and pass tests #
Total messages: 8
|
weita
|
16 years, 8 months ago (2009年05月01日 17:49:41 UTC) #1 | |||||||||||||||||||||||||||||||||||
http://codereview.appspot.com/53082/diff/1/2 File include/core/SkBitmap.h (right): http://codereview.appspot.com/53082/diff/1/2#newcode490 Line 490: SkColorTable(const SkColorTable& src); Is the comment above this constructor correct? It should say: makes a deep-copy of the src colortable. http://codereview.appspot.com/53082/diff/1/3 File src/core/SkBitmap.cpp (right): http://codereview.appspot.com/53082/diff/1/3#newcode699 Line 699: case kIndex8_Config: indent the "if" block, as the return false is indented below http://codereview.appspot.com/53082/diff/1/3#newcode708 Line 708: tmp.setConfig(dstConfig, this->width(), this->height()); Can we refactor this so there is only one shared call to allocPixels, perhaps with a ctable ptr that is sometimes NULL? Also, you can use SkAutoUnref au(ctable) after you (optionally) allocated it. This guy will take care of calling unref() for you, so you can just return false if allocPixels() fails. http://codereview.appspot.com/53082/diff/1/3#newcode731 Line 731: don't exceed 80-columns in the comment http://codereview.appspot.com/53082/diff/1/4 File src/core/SkColorTable.cpp (right): http://codereview.appspot.com/53082/diff/1/4#newcode39 Line 39: SkColorTable::SkColorTable(const SkColorTable& src) { Need to initialize f16Bitcache and fFlags
http://codereview.appspot.com/53082/diff/1/2 File include/core/SkBitmap.h (right): http://codereview.appspot.com/53082/diff/1/2#newcode490 Line 490: SkColorTable(const SkColorTable& src); On 2009年05月01日 18:04:59, reed wrote: > Is the comment above this constructor correct? It should say: makes a deep-copy > of the src colortable. Done. The original comment should stick with SkColorTalbe(int count). I add a new comment for the new constructor. http://codereview.appspot.com/53082/diff/1/3 File src/core/SkBitmap.cpp (right): http://codereview.appspot.com/53082/diff/1/3#newcode699 Line 699: case kIndex8_Config: On 2009年05月01日 18:04:59, reed wrote: > indent the "if" block, as the return false is indented below Done. http://codereview.appspot.com/53082/diff/1/3#newcode708 Line 708: tmp.setConfig(dstConfig, this->width(), this->height()); On 2009年05月01日 18:04:59, reed wrote: > Can we refactor this so there is only one shared call to allocPixels, perhaps > with a ctable ptr that is sometimes NULL? > > Also, you can use SkAutoUnref au(ctable) after you (optionally) allocated it. > This guy will take care of calling unref() for you, so you can just return false > if allocPixels() fails. Done. http://codereview.appspot.com/53082/diff/1/3#newcode731 Line 731: On 2009年05月01日 18:04:59, reed wrote: > don't exceed 80-columns in the comment Done. http://codereview.appspot.com/53082/diff/1/4 File src/core/SkColorTable.cpp (right): http://codereview.appspot.com/53082/diff/1/4#newcode39 Line 39: SkColorTable::SkColorTable(const SkColorTable& src) { On 2009年05月01日 18:04:59, reed wrote: > Need to initialize f16Bitcache and fFlags Done.
Add Patch Set 2
much better! http://codereview.appspot.com/53082/diff/9/1006 File include/core/SkBitmap.h (right): http://codereview.appspot.com/53082/diff/9/1006#newcode488 Line 488: /** Lets just say we make a deep copy of the colors. Mentioning fColors is a little impl-specific (and won't make sense if read from some external doxygen documentation.) http://codereview.appspot.com/53082/diff/9/1006#newcode493 Line 493: /** Constructs an empty color table (zero colors). I know this isn't your comment (its mine!), but it should really say something like: preallocate the colortable to have 'count' colors, which are initially set to 0
Done with the pervious comments and pass tests
http://codereview.appspot.com/53082/diff/9/1006 File include/core/SkBitmap.h (right): http://codereview.appspot.com/53082/diff/9/1006#newcode488 Line 488: /** On 2009年05月01日 19:58:18, reed wrote: > Lets just say we make a deep copy of the colors. Mentioning fColors is a little > impl-specific (and won't make sense if read from some external doxygen > documentation.) Done. http://codereview.appspot.com/53082/diff/9/1006#newcode493 Line 493: /** Constructs an empty color table (zero colors). On 2009年05月01日 19:58:18, reed wrote: > I know this isn't your comment (its mine!), but it should really say something > like: preallocate the colortable to have 'count' colors, which are initially set > to 0 Done.