Index: src/codec/SkCodec_libpng.cpp |
diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp |
index 444e2ad87e0ae14a3bfd95746fd991f592c7ceca..5c02f438544acaa4c21cf442e107dd51b7570532 100644 |
--- a/src/codec/SkCodec_libpng.cpp |
+++ b/src/codec/SkCodec_libpng.cpp |
@@ -214,7 +214,7 @@ bool SkPngCodec::IsPng(SkStream* stream) { |
// png_destroy_read_struct. If it returns false, the passed in fields (except |
// stream) are unchanged. |
static bool read_header(SkStream* stream, png_structp* png_ptrp, |
- png_infop* info_ptrp, SkImageInfo* imageInfo) { |
+ png_infop* info_ptrp, SkImageInfo* imageInfo, int bitDepth) { |
scroggo
2015/06/30 15:38:37
I believe you want this to be an int* rather than
emmaleer
2015/06/30 20:13:41
Done.
|
// The image is known to be a PNG. Decode enough to know the SkImageInfo. |
png_structp png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, |
sk_error_fn, sk_warning_fn); |
@@ -248,7 +248,7 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp, |
// PNG file before the first IDAT (image data chunk). |
png_read_info(png_ptr, info_ptr); |
png_uint_32 origWidth, origHeight; |
- int bitDepth, colorType; |
+ int colorType; |
png_get_IHDR(png_ptr, info_ptr, &origWidth, &origHeight, &bitDepth, |
&colorType, int_p_NULL, int_p_NULL, int_p_NULL); |
@@ -277,8 +277,7 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp, |
png_set_expand_gray_1_2_4_to_8(png_ptr); |
} |
- |
- // Now determine the default SkColorType and SkAlphaType. |
+ // Now determine the default SkColorType and SkAlphaType and set required transforms |
SkColorType skColorType; |
SkAlphaType skAlphaType; |
switch (colorType) { |
@@ -287,57 +286,47 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp, |
skAlphaType = has_transparency_in_palette(png_ptr, info_ptr) ? |
kUnpremul_SkAlphaType : kOpaque_SkAlphaType; |
break; |
- case PNG_COLOR_TYPE_GRAY: |
- if (false) { |
- // FIXME: Is this the wrong default behavior? This means if the |
- // caller supplies the info we gave them, they'll get Alpha 8. |
- skColorType = kAlpha_8_SkColorType; |
- // FIXME: Strangely, the canonical type for Alpha 8 is Premul. |
- skAlphaType = kPremul_SkAlphaType; |
+ case PNG_COLOR_TYPE_RGB: |
+ if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) { |
scroggo
2015/06/30 15:38:38
In other places, we also check to see whether the
emmaleer
2015/06/30 20:13:40
Yes, I think it's important to make that check. I'
|
+ //convert to RGBA with tranparency information in tRNS chunk if it exists |
+ png_set_tRNS_to_alpha(png_ptr); |
+ skAlphaType = kUnpremul_SkAlphaType; |
} else { |
- skColorType = kN32_SkColorType; |
+ //convert to RGBA with Opaque Alpha |
+ png_set_filler(png_ptr, 0xff, PNG_FILLER_AFTER); |
skAlphaType = kOpaque_SkAlphaType; |
} |
+ skColorType = kRGBA_8888_SkColorType; |
scroggo
2015/06/30 15:38:38
Interesting. I assume you chose RGBA because that
emmaleer
2015/06/30 20:13:40
Changed to N32
|
break; |
- default: |
- // Note: This *almost* mimics the code in SkImageDecoder_libpng. |
- // has_transparency_in_palette makes an additional check - whether |
- // numTrans is greater than 0. Why does the other code not make that |
- // check? |
- if (has_transparency_in_palette(png_ptr, info_ptr) |
- || PNG_COLOR_TYPE_RGB_ALPHA == colorType |
- || PNG_COLOR_TYPE_GRAY_ALPHA == colorType) |
- { |
+ case PNG_COLOR_TYPE_GRAY: |
+ if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) { |
+ //convert to RGBA if there is transparency information in the tRNS chunk |
scroggo
2015/06/30 15:38:37
Maybe this should be a FIXME; if we supported gray
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
+ png_set_tRNS_to_alpha(png_ptr); |
+ png_set_gray_to_rgb(png_ptr); |
+ skColorType = kRGBA_8888_SkColorType; |
skAlphaType = kUnpremul_SkAlphaType; |
} else { |
+ skColorType = kGray_8_SkColorType; |
skAlphaType = kOpaque_SkAlphaType; |
} |
- skColorType = kN32_SkColorType; |
break; |
- } |
- |
- { |
- // FIXME: Again, this block needs to go into onGetPixels. |
- bool convertGrayToRGB = PNG_COLOR_TYPE_GRAY == colorType && skColorType != kAlpha_8_SkColorType; |
- |
- // Unless the user is requesting A8, convert a grayscale image into RGB. |
- // GRAY_ALPHA will always be converted to RGB |
- if (convertGrayToRGB || colorType == PNG_COLOR_TYPE_GRAY_ALPHA) { |
+ case PNG_COLOR_TYPE_GRAY_ALPHA: |
+ //convert to RGBA as GreyAlpha is not a Skia color type |
scroggo
2015/06/30 15:38:37
Maybe add a similar FIXME here.
(Also nit: I don'
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
png_set_gray_to_rgb(png_ptr); |
scroggo
2015/06/30 15:38:38
Do we need to also call png_set_tRNS_to_alpha here
emmaleer
2015/06/30 20:13:41
No, as PNG_COLOR_TYPE_GRAY_ALPHA has an alpha chan
|
- } |
- |
- // Add filler (or alpha) byte (after each RGB triplet) if necessary. |
- // FIXME: It seems like we could just use RGB as the SrcConfig here. |
- if (colorType == PNG_COLOR_TYPE_RGB || convertGrayToRGB) { |
- png_set_filler(png_ptr, 0xff, PNG_FILLER_AFTER); |
- } |
+ skColorType = kRGBA_8888_SkColorType; |
+ skAlphaType = kUnpremul_SkAlphaType; |
+ break; |
+ default: |
+ //colorType = PNG_COLOR_TYPE_RGBA |
scroggo
2015/06/30 15:38:37
Is this the only colorType left? (If so, I think i
emmaleer
2015/06/30 20:13:41
Yes, this is the last color type. Okay, I have don
|
+ skColorType = kRGBA_8888_SkColorType; |
+ skAlphaType = kUnpremul_SkAlphaType; |
+ break; |
} |
// FIXME: Also need to check for sRGB (skbug.com/3471). |
if (imageInfo) { |
- *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType, |
- skAlphaType); |
+ *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType, skAlphaType); |
} |
autoClean.detach(); |
if (png_ptrp) { |
@@ -354,21 +343,24 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* stream) { |
png_structp png_ptr; |
png_infop info_ptr; |
SkImageInfo imageInfo; |
- if (read_header(stream, &png_ptr, &info_ptr, &imageInfo)) { |
- return SkNEW_ARGS(SkPngCodec, (imageInfo, streamDeleter.detach(), png_ptr, info_ptr)); |
+ int bitDepth; |
+ if (read_header(stream, &png_ptr, &info_ptr, &imageInfo, bitDepth)) { |
scroggo
2015/06/30 15:38:38
Since the function takes an int, this does not mod
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
+ return SkNEW_ARGS(SkPngCodec, (imageInfo, streamDeleter.detach(), |
+ png_ptr, info_ptr, bitDepth)); |
} |
return NULL; |
} |
#define INVALID_NUMBER_PASSES -1 |
SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream, |
- png_structp png_ptr, png_infop info_ptr) |
+ png_structp png_ptr, png_infop info_ptr, int bitDepth) |
: INHERITED(info, stream) |
, fPng_ptr(png_ptr) |
, fInfo_ptr(info_ptr) |
, fSrcConfig(SkSwizzler::kUnknown) |
, fNumberPasses(INVALID_NUMBER_PASSES) |
, fReallyHasAlpha(false) |
+ , fBitDepth(bitDepth) |
{} |
SkPngCodec::~SkPngCodec() { |
@@ -413,11 +405,13 @@ static bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src) |
return false; |
} |
} |
- |
// Check for supported color types |
switch (dst.colorType()) { |
- // Allow output to kN32 from any type of input |
- case kN32_SkColorType: |
+ // Allow output to RGBA from any type of input |
reed1
2015/06/30 13:51:01
not sure either of these comments are needed (also
emmaleer
2015/06/30 20:13:40
I removed the unneeded comments
In the enum in SkI
scroggo
2015/06/30 20:50:03
I think Mike's comment about ordering is actually
|
+ case kRGBA_8888_SkColorType: |
+ return true; |
+ // Allow output to GBRA from any type of input |
+ case kBGRA_8888_SkColorType: |
return true; |
default: |
return dst.colorType() == src.colorType(); |
@@ -436,33 +430,38 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo, |
return kInvalidInput; |
} |
- // FIXME: We already retrieved this information. Store it in SkPngCodec? |
- png_uint_32 origWidth, origHeight; |
- int bitDepth, pngColorType, interlaceType; |
- png_get_IHDR(fPng_ptr, fInfo_ptr, &origWidth, &origHeight, &bitDepth, |
- &pngColorType, &interlaceType, int_p_NULL, int_p_NULL); |
- |
- fNumberPasses = (interlaceType != PNG_INTERLACE_NONE) ? |
- png_set_interlace_handling(fPng_ptr) : 1; |
- |
// Set to the default before calling decodePalette, which may change it. |
fReallyHasAlpha = false; |
- if (PNG_COLOR_TYPE_PALETTE == pngColorType) { |
- fSrcConfig = SkSwizzler::kIndex; |
- if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(), bitDepth, |
- ctableCount)) { |
- return kInvalidInput; |
- } |
- } else if (kAlpha_8_SkColorType == requestedInfo.colorType()) { |
- // Note: we check the destination, since otherwise we would have |
- // told png to upscale. |
- SkASSERT(PNG_COLOR_TYPE_GRAY == pngColorType); |
- fSrcConfig = SkSwizzler::kGray; |
- } else if (this->getInfo().alphaType() == kOpaque_SkAlphaType) { |
- fSrcConfig = SkSwizzler::kRGBX; |
- } else { |
- fSrcConfig = SkSwizzler::kRGBA; |
- } |
+ |
+ //srcColorType was determined in readHeader() which determined png color type |
+ int srcColorType = this->getInfo().colorType(); |
scroggo
2015/06/30 15:38:37
Why do you store these as ints instead of SkColorT
emmaleer
2015/06/30 20:13:40
Changed to const SkColorType
|
+ int dstColorType = requestedInfo.colorType(); |
+ switch (dstColorType) { |
scroggo
2015/06/30 15:38:37
nit: this should line up with the line before it.
emmaleer
2015/06/30 20:13:40
I switch on dstColorType, since libpng will conver
|
+ case kIndex_8_SkColorType: |
+ //decode palette to Skia format |
+ fSrcConfig = SkSwizzler::kIndex; |
+ if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(), fBitDepth, |
+ ctableCount)) { |
+ return kInvalidInput; |
+ } |
+ break; |
+ case kGray_8_SkColorType: |
+ fSrcConfig = SkSwizzler::kGray; |
+ break; |
+ case kRGBA_8888_SkColorType: |
+ if (this->getInfo().alphaType() == kOpaque_SkAlphaType) { |
+ fSrcConfig = SkSwizzler::kRGBX; |
+ } else { |
+ fSrcConfig = SkSwizzler::kRGBA; |
+ } |
+ default: |
+ //dstColorType == kBGRA_8888_SkColorType |
+ if (this->getInfo().alphaType() == kOpaque_SkAlphaType) { |
+ fSrcConfig = SkSwizzler::kBGRX; |
+ } else { |
+ fSrcConfig = SkSwizzler::kBGRA; |
+ } |
+ } |
// Copy the color table to the client if they request kIndex8 mode |
copy_color_table(requestedInfo, fColorTable, ctable, ctableCount); |
@@ -478,14 +477,52 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo, |
// FIXME: Here is where we should likely insert some of the modifications |
scroggo
2015/06/30 15:38:37
I think this FIXME has been resolved?
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
// made in the factory. |
- png_read_update_info(fPng_ptr, fInfo_ptr); |
return kSuccess; |
} |
-bool SkPngCodec::handleRewind() { |
+SkCodec::Result SkPngCodec::setDataFormat(const SkImageInfo& requestedInfo) { |
+ if (setjmp(png_jmpbuf(fPng_ptr))) { |
+ SkCodecPrintf("setjmp long jump!\n"); |
scroggo
2015/06/30 15:38:37
I know this is the same message that is printed el
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
+ return kInvalidInput; |
+ } |
+ |
+ fNumberPasses = png_set_interlace_handling(fPng_ptr); |
+ |
+ //srcColorType was determined in readHeader() which determined png color type |
+ int srcColorType = this->getInfo().colorType(); |
scroggo
2015/06/30 15:38:37
Again, I think these should be SkColorType instead
emmaleer
2015/06/30 20:13:40
Acknowledged.
|
+ int dstColorType = requestedInfo.colorType(); |
+ if (kRGBA_8888_SkColorType == dstColorType || kBGRA_8888_SkColorType == dstColorType) { |
+ //dstColorType either equals kRGBA_8888_SkColorType or kBGRA_8888_SkColorType |
scroggo
2015/06/30 15:38:37
This seems to just add nothing new to the line abo
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
+ if (kIndex_8_SkColorType == srcColorType) { |
scroggo
2015/06/30 15:38:37
nit: This should only be indented 4 spaces from th
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
+ //convert paletted images to RGBA |
+ png_set_palette_to_rgb(fPng_ptr); |
+ if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) { |
+ png_set_tRNS_to_alpha(fPng_ptr); |
+ } else { |
+ png_set_filler(fPng_ptr, 0xff, PNG_FILLER_AFTER); |
+ } |
+ } else if (kGray_8_SkColorType == srcColorType) { |
+ //convert from Gray to RGBA (need to add filler for alpha as gray has no alpha) |
+ png_set_gray_to_rgb(fPng_ptr); |
+ png_set_filler(fPng_ptr, 0xff, PNG_FILLER_AFTER); |
+ } |
+ if (kBGRA_8888_SkColorType == dstColorType) { |
+ //convert RGBA to BGRA |
+ png_set_bgr(fPng_ptr); |
+ } |
+ } |
+ png_read_update_info(fPng_ptr, fInfo_ptr); |
+ return kSuccess; |
+} |
+ |
+bool SkPngCodec::handleRewind(const SkImageInfo& requestedInfo) { |
switch (this->rewindIfNeeded()) { |
case kNoRewindNecessary_RewindState: |
+ //set transforms needed for requestedInfo format |
+ if (kSuccess != setDataFormat(requestedInfo)) { |
scroggo
2015/06/30 15:38:38
nit: this->setDataFormat
emmaleer
2015/06/30 20:13:40
Acknowledged.
|
+ return false; |
scroggo
2015/06/30 15:38:37
Interestingly, we've changed the nature of the err
emmaleer
2015/06/30 20:13:40
I think it's important to return the correct resul
|
+ } |
return true; |
case kCouldNotRewind_RewindState: |
return false; |
@@ -498,9 +535,13 @@ bool SkPngCodec::handleRewind() { |
this->destroyReadStruct(); |
png_structp png_ptr; |
png_infop info_ptr; |
- if (read_header(this->stream(), &png_ptr, &info_ptr, NULL)) { |
+ if (read_header(this->stream(), &png_ptr, &info_ptr, NULL, NULL)) { |
fPng_ptr = png_ptr; |
fInfo_ptr = info_ptr; |
+ //set transforms needed for requestedInfo format |
+ if (kSuccess != setDataFormat(requestedInfo)) { |
scroggo
2015/06/30 15:38:37
this->setDataFormat
emmaleer
2015/06/30 20:13:40
Acknowledged.
|
+ return false; |
+ } |
return true; |
} |
return false; |
@@ -514,7 +555,7 @@ bool SkPngCodec::handleRewind() { |
SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, |
size_t rowBytes, const Options& options, |
SkPMColor ctable[], int* ctableCount) { |
- if (!this->handleRewind()) { |
+ if (!this->handleRewind(requestedInfo)) { |
scroggo
2015/06/30 15:38:38
I notice that setDataFormat never cares if it sees
emmaleer
2015/06/30 20:13:41
I changed it so that conversion_possible is called
|
return kCouldNotRewind; |
} |
if (requestedInfo.dimensions() != this->getInfo().dimensions()) { |
@@ -527,11 +568,10 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* |
// Note that ctable and ctableCount may be modified if there is a color table |
const Result result = this->initializeSwizzler(requestedInfo, dst, rowBytes, |
options, ctable, ctableCount); |
- |
if (result != kSuccess) { |
return result; |
} |
- |
+ |
scroggo
2015/06/30 15:38:38
It looks like you added some whitespace?
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
// FIXME: Could we use the return value of setjmp to specify the type of |
// error? |
if (setjmp(png_jmpbuf(fPng_ptr))) { |
@@ -597,8 +637,9 @@ public: |
: INHERITED(dstInfo) |
, fCodec(codec) |
, fHasAlpha(false) |
+ , fSrcRowBytes(dstInfo.minRowBytes()) |
scroggo
2015/06/30 15:38:37
Why do we need to store this? It seems like we onl
emmaleer
2015/06/30 20:13:40
I removed this
|
{ |
- fStorage.reset(dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig)); |
+ fStorage.reset(dstInfo.width() * fSrcRowBytes); |
scroggo
2015/06/30 15:38:37
I don't think this is what you want. minRowBytes()
emmaleer
2015/06/30 20:13:41
Yes, this is wrong. I changed this back to using
|
fSrcRow = static_cast<uint8_t*>(fStorage.get()); |
} |
@@ -644,6 +685,7 @@ private: |
bool fHasAlpha; |
SkAutoMalloc fStorage; |
uint8_t* fSrcRow; |
+ size_t fSrcRowBytes; |
typedef SkScanlineDecoder INHERITED; |
}; |
@@ -659,16 +701,18 @@ public: |
, fHeight(dstInfo.height()) |
, fSrcRowBytes(dstInfo.minRowBytes()) |
, fRewindNeeded(false) |
+ , fDstInfo(dstInfo) |
{ |
fGarbageRow.reset(fSrcRowBytes); |
fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get()); |
} |
- SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override { |
+ SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override { |
scroggo
2015/06/30 15:38:38
This should be indented 4 spaces.
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
//rewind stream if have previously called onGetScanlines, |
//since we need entire progressive image to get scanlines |
+ //need to reset transforms in setDataFormat if stream is rewound |
if (fRewindNeeded) { |
- if(false == fCodec->handleRewind()) { |
+ if(false == fCodec->handleRewind(fDstInfo)) { |
scroggo
2015/06/30 15:38:38
Alternatively, you could use this->dstInfo(), whic
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
return SkImageGenerator::kCouldNotRewind; |
} |
} else { |
@@ -710,10 +754,10 @@ public: |
return SkImageGenerator::kSuccess; |
} |
- SkImageGenerator::Result onSkipScanlines(int count) override { |
- //when ongetScanlines is called it will skip to fCurrentRow |
- fCurrentRow += count; |
- return SkImageGenerator::kSuccess; |
+ SkImageGenerator::Result onSkipScanlines(int count) override { |
scroggo
2015/06/30 15:38:37
More added whitespace?
|
+ //when ongetScanlines is called it will skip to fCurrentRow |
+ fCurrentRow += count; |
+ return SkImageGenerator::kSuccess; |
} |
void onFinish() override { |
@@ -731,18 +775,14 @@ private: |
bool fRewindNeeded; |
SkAutoMalloc fGarbageRow; |
uint8_t* fGarbageRowPtr; |
- |
- |
- |
- |
- |
+ const SkImageInfo& fDstInfo; |
typedef SkScanlineDecoder INHERITED; |
}; |
SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo, |
const Options& options, SkPMColor ctable[], int* ctableCount) { |
- if (!this->handleRewind()) { |
+ if (!this->handleRewind(dstInfo)) { |
return NULL; |
} |
@@ -756,7 +796,7 @@ SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo, |
return NULL; |
} |
- // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed, |
+ // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed, |
scroggo
2015/06/30 15:38:37
nit: 4 space indent.
emmaleer
2015/06/30 20:13:41
Acknowledged.
|
// since we'll be manually updating the dstRow, but the SkSwizzler requires it to |
// be at least dstInfo.minRowBytes. |
if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), options, ctable, |