Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
159173004:
SkBitmap now really stores SkImageInfo -- config is just a ruse (Closed)
|
Created:
6 years, 10 months ago by reed1 Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionSkBitmap now really stores SkImageInfo -- config is just a ruse
BUG=skia:
Patch Set 1 #
Total comments: 18
Patch Set 2 : #Patch Set 3 : rebase #
Messages
Total messages: 14 (0 generated)
Tried to make the minimal number of changes for now, to make the review easier. Goal: 1. clients don't have to rev. at all yet : using config should still work 2. clients may start to specify extended colortypes (e.g. unpremul) 2+ no promise that we will draw those correctly yet. In theory, if this lands/sticks, then a follow-on CL could remove the Config8888 from SkCanvas, and remake readPixels/writePixels to just rely on the colortype/alphatype in the bitmap.
chrome and its unit_tests seem to build and run with this CL (applied locally)
https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h File include/core/SkBitmap.h (right): https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:86: int width() const { return fInfo.fWidth; } Why did width and height lose the doc stating that they are in pixels? https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:151: size_t getSize() const { return fInfo.fHeight * fRowBytes; } Is it okay that this will return zero for a new style SkBitmap that is not locked? https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:157: size_t getSafeSize() const { return fInfo.getSafeSize(fRowBytes); } Same question. https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... include/core/SkImageInfo.h:62: static inline bool SkAlphaTypeIsValid(unsigned value) { Does this need to be unsigned? It seems like all the callers pass in an SkAlphaType (similar question for SkColorTypeIsValid)? https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... include/core/SkImageInfo.h:176: int width() const { return fWidth; } I find it interesting that we have const accessors to fields that are publicly accessible. Is the plan to make fWidth etc private? https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:870: case kPMColor_SkColorType: { Should we also handle the non-native 8888? I guess we would need to add a similar case to this one that uses the correct swizzle. https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1138: if (!dst->setConfig(info, rowBytes)) { Can we skip setConfig() altogether? Isn't there a flavor of setPixelRef that sets the info as well? https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1543: fInfo.flatten(buffer); Technically, this ought to bump the PICTURE_VERSION, but I think we might be able to get away with not doing it since our SKPs are captured using bitmap encoding (so SkBitmap::flatten won't get called from Chrome). https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1569: SkAlphaTypeIsValid(info.fAlphaType) && nit: spacing.
https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h File include/core/SkBitmap.h (right): https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:86: int width() const { return fInfo.fWidth; } On 2014/02/10 21:45:44, scroggo wrote: > Why did width and height lose the doc stating that they are in pixels? If you think it adds value, I will add it. I didn't think there was any confusion of what the returned value could possibly mean. https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:151: size_t getSize() const { return fInfo.fHeight * fRowBytes; } On 2014/02/10 21:45:44, scroggo wrote: > Is it okay that this will return zero for a new style SkBitmap that is not > locked? Who knows what the future will bring. https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... include/core/SkBitmap.h:157: size_t getSafeSize() const { return fInfo.getSafeSize(fRowBytes); } On 2014/02/10 21:45:44, scroggo wrote: > Same question. Always in motion is the future. https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h File include/core/SkImageInfo.h (right): https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... include/core/SkImageInfo.h:62: static inline bool SkAlphaTypeIsValid(unsigned value) { On 2014/02/10 21:45:44, scroggo wrote: > Does this need to be unsigned? It seems like all the callers pass in an > SkAlphaType (similar question for SkColorTypeIsValid)? Yea, I wanted to explicitly not let the compiler make this a no-op. If I call the param the enum, then it seems like it could do that. https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... include/core/SkImageInfo.h:176: int width() const { return fWidth; } On 2014/02/10 21:45:44, scroggo wrote: > I find it interesting that we have const accessors to fields that are publicly > accessible. Is the plan to make fWidth etc private? Yes, I want to make them private. https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:870: case kPMColor_SkColorType: { On 2014/02/10 21:45:44, scroggo wrote: > Should we also handle the non-native 8888? I guess we would need to add a > similar case to this one that uses the correct swizzle. Yea, I think that is something to consider for the future: how do we draw-to, and draw-from, those other swizzles. Drawing-from doesn't seem hard, and I expect we'll write a new shaderproc. Drawing-to might be something we just don't support. I'll add a comment to that effect here. https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1138: if (!dst->setConfig(info, rowBytes)) { On 2014/02/10 21:45:44, scroggo wrote: > Can we skip setConfig() altogether? Isn't there a flavor of setPixelRef that > sets the info as well? Not yet, that is the installPixelRef() idea in the other CL -- which is a little tricker than this one I think. https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1543: fInfo.flatten(buffer); On 2014/02/10 21:45:44, scroggo wrote: > Technically, this ought to bump the PICTURE_VERSION, but I think we might be > able to get away with not doing it since our SKPs are captured using bitmap > encoding (so SkBitmap::flatten won't get called from Chrome). Ah, didn't think about that lucky part. https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... src/core/SkBitmap.cpp:1569: SkAlphaTypeIsValid(info.fAlphaType) && On 2014/02/10 21:45:44, scroggo wrote: > nit: spacing. Done.
ptal
On 2014/02/10 21:59:50, reed1 wrote: > https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h > File include/core/SkBitmap.h (right): > > https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... > include/core/SkBitmap.h:86: int width() const { return fInfo.fWidth; } > On 2014/02/10 21:45:44, scroggo wrote: > > Why did width and height lose the doc stating that they are in pixels? > > If you think it adds value, I will add it. I didn't think there was any > confusion of what the returned value could possibly mean. > > https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... > include/core/SkBitmap.h:151: size_t getSize() const { return fInfo.fHeight * > fRowBytes; } > On 2014/02/10 21:45:44, scroggo wrote: > > Is it okay that this will return zero for a new style SkBitmap that is not > > locked? > > Who knows what the future will bring. > > https://codereview.chromium.org/159173004/diff/1/include/core/SkBitmap.h#newc... > include/core/SkBitmap.h:157: size_t getSafeSize() const { return > fInfo.getSafeSize(fRowBytes); } > On 2014/02/10 21:45:44, scroggo wrote: > > Same question. > > Always in motion is the future. > > https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h > File include/core/SkImageInfo.h (right): > > https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... > include/core/SkImageInfo.h:62: static inline bool SkAlphaTypeIsValid(unsigned > value) { > On 2014/02/10 21:45:44, scroggo wrote: > > Does this need to be unsigned? It seems like all the callers pass in an > > SkAlphaType (similar question for SkColorTypeIsValid)? > > Yea, I wanted to explicitly not let the compiler make this a no-op. If I call > the param the enum, then it seems like it could do that. > > https://codereview.chromium.org/159173004/diff/1/include/core/SkImageInfo.h#n... > include/core/SkImageInfo.h:176: int width() const { return fWidth; } > On 2014/02/10 21:45:44, scroggo wrote: > > I find it interesting that we have const accessors to fields that are publicly > > accessible. Is the plan to make fWidth etc private? > > Yes, I want to make them private. > > https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp > File src/core/SkBitmap.cpp (right): > > https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:870: case kPMColor_SkColorType: { > On 2014/02/10 21:45:44, scroggo wrote: > > Should we also handle the non-native 8888? I guess we would need to add a > > similar case to this one that uses the correct swizzle. > > Yea, I think that is something to consider for the future: how do we draw-to, > and draw-from, those other swizzles. Drawing-from doesn't seem hard, and I > expect we'll write a new shaderproc. Drawing-to might be something we just don't > support. > > I'll add a comment to that effect here. > > https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:1138: if (!dst->setConfig(info, rowBytes)) { > On 2014/02/10 21:45:44, scroggo wrote: > > Can we skip setConfig() altogether? Isn't there a flavor of setPixelRef that > > sets the info as well? > > Not yet, that is the installPixelRef() idea in the other CL -- which is a little > tricker than this one I think. > > https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:1543: fInfo.flatten(buffer); > On 2014/02/10 21:45:44, scroggo wrote: > > Technically, this ought to bump the PICTURE_VERSION, but I think we might be > > able to get away with not doing it since our SKPs are captured using bitmap > > encoding (so SkBitmap::flatten won't get called from Chrome). > > Ah, didn't think about that lucky part. > > https://codereview.chromium.org/159173004/diff/1/src/core/SkBitmap.cpp#newcod... > src/core/SkBitmap.cpp:1569: SkAlphaTypeIsValid(info.fAlphaType) && > On 2014/02/10 21:45:44, scroggo wrote: > > nit: spacing. > > Done. lgtm
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/reed@google.com/159173004/110001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for include/core/SkBitmap.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file include/core/SkBitmap.h
Hunk #1 FAILED at 79.
Hunk #2 succeeded at 95 with fuzz 1 (offset 6 lines).
Hunk #3 succeeded at 122 (offset 6 lines).
Hunk #4 succeeded at 143 (offset 6 lines).
Hunk #5 succeeded at 277 (offset 6 lines).
Hunk #6 succeeded at 430 (offset 6 lines).
Hunk #7 succeeded at 722 (offset 6 lines).
Hunk #8 succeeded at 838 (offset 6 lines).
1 out of 8 hunks FAILED -- saving rejects to file include/core/SkBitmap.h.rej
Patch: include/core/SkBitmap.h
Index: include/core/SkBitmap.h
diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h
index
4951cf19cf7b15ef70dd1a09035f2ef7b95dbcce..164663dbae91ef0282f952a54076f023b299b8f8
100644
--- a/include/core/SkBitmap.h
+++ b/include/core/SkBitmap.h
@@ -79,9 +79,41 @@ public:
// This method is not exported to java.
void swap(SkBitmap& other);
+ ///////////////////////////////////////////////////////////////////////////
+
+ const SkImageInfo& info() const { return fInfo; }
+
+ int width() const { return fInfo.fWidth; }
+ int height() const { return fInfo.fHeight; }
+ SkColorType colorType() const { return fInfo.fColorType; }
+ SkAlphaType alphaType() const { return fInfo.fAlphaType; }
+
+ /** Return the number of bytes per pixel based on the config. If the config
+ does not have at least 1 byte per (e.g. kA1_Config) then 0 is returned.
+ */
+ int bytesPerPixel() const { return fInfo.bytesPerPixel(); }
+
+ /** Return the rowbytes expressed as a number of pixels (like width and
+ height). Note, for 1-byte per pixel configs like kA8_Config, this will
+ return the same as rowBytes(). Is undefined for configs that are less
+ than 1-byte per pixel (e.g. kA1_Config)
+ */
+ int rowBytesAsPixels() const {
+ return fRowBytes >> this->shiftPerPixel();
+ }
+
+ /** Return the shift amount per pixel (i.e. 0 for 1-byte per pixel, 1 for
+ 2-bytes per pixel configs, 2 for 4-bytes per pixel configs). Return 0
+ for configs that are not at least 1-byte per pixel (e.g. kA1_Config
+ or kNo_Config)
+ */
+ int shiftPerPixel() const { return this->bytesPerPixel() >> 1; }
+
+ ///////////////////////////////////////////////////////////////////////////
+
/** Return true iff the bitmap has empty dimensions.
*/
- bool empty() const { return 0 == fWidth || 0 == fHeight; }
+ bool empty() const { return fInfo.isEmpty(); }
/** Return true iff the bitmap has no pixelref. Note: this can return true
even if the
dimensions of the bitmap are > 0 (see empty()).
@@ -89,41 +121,14 @@ public:
bool isNull() const { return NULL == fPixelRef; }
/** Return the config for the bitmap. */
- Config config() const { return (Config)fConfig; }
+ Config config() const;
SK_ATTR_DEPRECATED("use config()")
Config getConfig() const { return this->config(); }
- /** Return the bitmap's width, in pixels. */
- int width() const { return fWidth; }
-
- /** Return the bitmap's height, in pixels. */
- int height() const { return fHeight; }
-
/** Return the number of bytes between subsequent rows of the bitmap. */
size_t rowBytes() const { return fRowBytes; }
- /** Return the shift amount per pixel (i.e. 0 for 1-byte per pixel, 1 for
- 2-bytes per pixel configs, 2 for 4-bytes per pixel configs). Return 0
- for configs that are not at least 1-byte per pixel (e.g. kA1_Config
- or kNo_Config)
- */
- int shiftPerPixel() const { return fBytesPerPixel >> 1; }
-
- /** Return the number of bytes per pixel based on the config. If the config
- does not have at least 1 byte per (e.g. kA1_Config) then 0 is returned.
- */
- int bytesPerPixel() const { return fBytesPerPixel; }
-
- /** Return the rowbytes expressed as a number of pixels (like width and
- height). Note, for 1-byte per pixel configs like kA8_Config, this will
- return the same as rowBytes(). Is undefined for configs that are less
- than 1-byte per pixel (e.g. kA1_Config)
- */
- int rowBytesAsPixels() const { return fRowBytes >> (fBytesPerPixel >> 1); }
-
- SkAlphaType alphaType() const { return (SkAlphaType)fAlphaType; }
-
/**
* Set the bitmap's alphaType, returning true on success. If false is
* returned, then the specified new alphaType is incompatible with the
@@ -143,19 +148,19 @@ public:
Note this truncates the result to 32bits. Call getSize64() to detect
if the real size exceeds 32bits.
*/
- size_t getSize() const { return fHeight * fRowBytes; }
+ size_t getSize() const { return fInfo.fHeight * fRowBytes; }
/** Return the number of bytes from the pointer returned by getPixels()
to the end of the allocated space in the buffer. Required in
cases where extractSubset has been called.
*/
- size_t getSafeSize() const ;
+ size_t getSafeSize() const { return fInfo.getSafeSize(fRowBytes); }
/**
* Return the full size of the bitmap, in bytes.
*/
int64_t computeSize64() const {
- return sk_64_mul(fHeight, fRowBytes);
+ return sk_64_mul(fInfo.fHeight, fRowBytes);
}
/**
@@ -164,7 +169,7 @@ public:
* than computeSize64() if there is any rowbytes padding beyond the width.
*/
int64_t computeSafeSize64() const {
- return ComputeSafeSize64((Config)fConfig, fWidth, fHeight, fRowBytes);
+ return fInfo.getSafeSize64(fRowBytes);
}
/** Returns true if this bitmap is marked as immutable, meaning that the
@@ -298,11 +303,18 @@ public:
void* context);
/**
- * If the bitmap's config can be represented as SkImageInfo, return true,
- * and if info is not-null, set it to the bitmap's info. If it cannot be
- * represented as SkImageInfo, return false and ignore the info parameter.
+ * DEPRECATED: call info().
*/
- bool asImageInfo(SkImageInfo* info) const;
+ bool asImageInfo(SkImageInfo* info) const {
+ // compatibility: return false for kUnknown
+ if (kUnknown_SkColorType == this->colorType()) {
+ return false;
+ }
+ if (info) {
+ *info = this->info();
+ }
+ return true;
+ }
/** Use this to assign a new pixel address for an existing bitmap. This
will automatically release any pixelref previously installed. Only call
@@ -444,8 +456,8 @@ public:
*/
GrTexture* getTexture() const;
- /** Return the bitmap's colortable, if it uses one (i.e. fConfig is
- kIndex8_Config) and the pixels are locked.
+ /** Return the bitmap's colortable, if it uses one (i.e. colorType is
+ Index_8) and the pixels are locked.
Otherwise returns NULL. Does not affect the colortable's
reference count.
*/
@@ -736,13 +748,11 @@ private:
#endif
};
+ SkImageInfo fInfo;
+
uint32_t fRowBytes;
- uint32_t fWidth;
- uint32_t fHeight;
- uint8_t fConfig;
- uint8_t fAlphaType;
+
uint8_t fFlags;
- uint8_t fBytesPerPixel; // based on config
void internalErase(const SkIRect&, U8CPU a, U8CPU r, U8CPU g, U8CPU
b)const;
@@ -854,29 +864,29 @@ private:
inline uint32_t* SkBitmap::getAddr32(int x, int y) const {
SkASSERT(fPixels);
- SkASSERT(fConfig == kARGB_8888_Config);
- SkASSERT((unsigned)x < fWidth && (unsigned)y < fHeight);
+ SkASSERT(this->config() == kARGB_8888_Config);
+ SkASSERT((unsigned)x < (unsigned)this->width() && (unsigned)y <
(unsigned)this->height());
return (uint32_t*)((char*)fPixels + y * fRowBytes + (x << 2));
}
inline uint16_t* SkBitmap::getAddr16(int x, int y) const {
SkASSERT(fPixels);
- SkASSERT(fConfig == kRGB_565_Config || fConfig == kARGB_4444_Config);
- SkASSERT((unsigned)x < fWidth && (unsigned)y < fHeight);
+ SkASSERT(this->config() == kRGB_565_Config || this->config() ==
kARGB_4444_Config);
+ SkASSERT((unsigned)x < (unsigned)this->width() && (unsigned)y <
(unsigned)this->height());
return (uint16_t*)((char*)fPixels + y * fRowBytes + (x << 1));
}
inline uint8_t* SkBitmap::getAddr8(int x, int y) const {
SkASSERT(fPixels);
- SkASSERT(fConfig == kA8_Config || fConfig == kIndex8_Config);
- SkASSERT((unsigned)x < fWidth && (unsigned)y < fHeight);
+ SkASSERT(this->config() == kA8_Config || this->config() == kIndex8_Config);
+ SkASSERT((unsigned)x < (unsigned)this->width() && (unsigned)y <
(unsigned)this->height());
return (uint8_t*)fPixels + y * fRowBytes + x;
}
inline SkPMColor SkBitmap::getIndex8Color(int x, int y) const {
SkASSERT(fPixels);
- SkASSERT(fConfig == kIndex8_Config);
- SkASSERT((unsigned)x < fWidth && (unsigned)y < fHeight);
+ SkASSERT(this->config() == kIndex8_Config);
+ SkASSERT((unsigned)x < (unsigned)this->width() && (unsigned)y <
(unsigned)this->height());
SkASSERT(fColorTable);
return (*fColorTable)[*((const uint8_t*)fPixels + y * fRowBytes + x)];
}
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/reed@google.com/159173004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests, BuildTools http://108.170.219.164:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
