Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944633002:
Use SkPngEncoder in gfx jpeg_codec (Closed)
DescriptionUse SkPngEncoder in gfx jpeg_codec
BUG=724616
Review-Url: https://codereview.chromium.org/2944633002
Cr-Commit-Position: refs/heads/master@{#486433}
Committed: https://chromium.googlesource.com/chromium/src/+/88e6a1793be2e7cce6c07a43f3147b80325a18a7
Patch Set 1 #
Total comments: 14
Patch Set 2 : All Changed #
Total comments: 28
Patch Set 3 : Nits #Patch Set 4 : Nits #
Total comments: 2
Patch Set 5 : Nit #
Total comments: 18
Patch Set 6 : DCHECK #Patch Set 7 : Update comments #
Total comments: 4
Patch Set 8 : N32 #
Total comments: 2
Patch Set 9 : No default #Patch Set 10 : Init colorType #Patch Set 11 : Cast #
Messages
Total messages: 41 (16 generated)
Description was changed from ========== Use SkPngEncoder in gfx jpeg_codec BUG=724616 ========== to ========== Use SkPngEncoder in gfx jpeg_codec BUG=724616 ==========
liyuqian@google.com changed reviewers: + dcheng@chromium.org, erg@chromium.org, msarett@google.com, scroggo@google.com
Please check this preliminary CL. I'd like to know if codec_prev.h/cc are Ok. I'll change everything else to use SkPngEncoder later in this CL.
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h File ui/gfx/codec/codec_priv.h (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:17: class VectorWStream : public SkWStream { Are you planning to put more stuff in here? Maybe this should just be vector_wstream.h? https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:19: VectorWStream(std::vector<unsigned char>* dst) : dst_(dst) { Maybe comment here that it does not take ownership? https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec_unit... File ui/gfx/codec/png_codec_unittest.cc (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec_unit... ui/gfx/codec/png_codec_unittest.cc:282: // create an image with known values, a must be opaque because it will be This comment is strange. Why can we not have transparency?
mostly style things https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.cc File ui/gfx/codec/codec_priv.cc (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.cc#... ui/gfx/codec/codec_priv.cc:18: } Same thing with newline and namespace. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h File ui/gfx/codec/codec_priv.h (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:32: } newline before '}', and also, add " // namespace gfx" https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:34: #endif Header guard comment. // UI_GFX_CODEC_PRIV_CODEC_H_
msarett@chromium.org changed reviewers: + msarett@chromium.org
Looks like a good start. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (left): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec.cc#o... ui/gfx/codec/png_codec.cc:743: bool discard_transparency, As we discussed in person, you'll have to find a way to handle discard_transparency. It's a bit of a strange option because, as it is implemented now, the encoder may need to unpremultiply and then throw away the alpha channel. My preference would be to keep that logic here by: ***Unpremultiply in gfx if necessary ***Lie to Skia and say that the pixels are kOpaque_SkAlphaType -------- Might also be worth investigating if users of discard_transparency=true actually need it.
Thanks for all of your useful comments! I've now changed all PNG encoding code. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.cc File ui/gfx/codec/codec_priv.cc (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.cc#... ui/gfx/codec/codec_priv.cc:18: } On 2017/06/16 19:08:11, Elliot Glaysher wrote: > Same thing with newline and namespace. Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h File ui/gfx/codec/codec_priv.h (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:17: class VectorWStream : public SkWStream { On 2017/06/16 18:56:51, scroggo_chromium wrote: > Are you planning to put more stuff in here? Maybe this should just be > vector_wstream.h? Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:19: VectorWStream(std::vector<unsigned char>* dst) : dst_(dst) { On 2017/06/16 18:56:51, scroggo_chromium wrote: > Maybe comment here that it does not take ownership? Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:32: } On 2017/06/16 19:08:11, Elliot Glaysher wrote: > newline before '}', and also, add " // namespace gfx" Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/codec_priv.h#n... ui/gfx/codec/codec_priv.h:34: #endif On 2017/06/16 19:08:11, Elliot Glaysher wrote: > Header guard comment. // UI_GFX_CODEC_PRIV_CODEC_H_ Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (left): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec.cc#o... ui/gfx/codec/png_codec.cc:743: bool discard_transparency, On 2017/06/16 21:29:56, msarett1 wrote: > As we discussed in person, you'll have to find a way to handle > discard_transparency. > > It's a bit of a strange option because, as it is implemented now, the encoder > may need to unpremultiply and then throw away the alpha channel. > > My preference would be to keep that logic here by: > ***Unpremultiply in gfx if necessary > ***Lie to Skia and say that the pixels are kOpaque_SkAlphaType > > -------- > > Might also be worth investigating if users of discard_transparency=true actually > need it. Done. https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec_unit... File ui/gfx/codec/png_codec_unittest.cc (right): https://codereview.chromium.org/2944633002/diff/1/ui/gfx/codec/png_codec_unit... ui/gfx/codec/png_codec_unittest.cc:282: // create an image with known values, a must be opaque because it will be On 2017/06/16 18:56:51, scroggo_chromium wrote: > This comment is strange. Why can we not have transparency? I've no idea what this comment means... Anyway, I think that my code doesn't change any behavior of the test (other than wrapping all format information into SkPixmap)?
https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:425: int zLibLevel = Z_DEFAULT_COMPRESSION) { Also, another optional nit is that I think it was clearer when we didn't use default args and just wrote it out. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:425: int zLibLevel = Z_DEFAULT_COMPRESSION) { Nit: naming (same throughout for unpremulInfo, unpremulPixmap, opaqueBitmap, etc) https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:431: options.fZLibLevel = zLibLevel == Z_DEFAULT_COMPRESSION ? 6 : zLibLevel; Why do we override Z_DEFAULT_COMPRESSION here? We should have a comment at least. Seems like maybe the skia routines should just do this default mapping internally to avoid having to update all the callers if it ever changes.
Change jpeg_codec to png_codec in commit message. ----- This generally looks good to me. Just some nits. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:431: options.fZLibLevel = zLibLevel == Z_DEFAULT_COMPRESSION ? 6 : zLibLevel; On 2017/06/23 21:45:55, dcheng wrote: > Why do we override Z_DEFAULT_COMPRESSION here? We should have a comment at > least. Seems like maybe the skia routines should just do this default mapping > internally to avoid having to update all the callers if it ever changes. What if we have a gfx::PngCodec::kDefaultZLibCompression = 6? That way gfx behavior doesn't change if zlib decides to change what Z_DEFAULT_COMPRESSION compression corresponds to. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:440: if (!discard_transparency) { nit: I find this more readable as if (discard_transparency). https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:442: } else { nit: No need for an else since we return in the if. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:447: SkImageInfo unpremulInfo = unpremulInfo = src.info().makeAlphaType(kUnpremul_SkAlphaType) https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:455: if (!unpremulCopy.peekPixels(&unpremulPixmap)) { Just assert that peekPixels() succeeds. It must because you've already allocated pixels. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:458: src.readPixels(unpremulPixmap); Also assert that this succeeds. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:463: SkPixmap opaquePixmap(opaqueInfo, unpremulPixmap.addr(), Hmm maybe just call tryAllocPixels() using an opaque info. Then readPixels(opaquePixmap.info().makeAlphaType(kUnpremul), opaquePixmap.addr(), opaquePixmap.rowBytes()). Then you only need to put one pixmap on the stack. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:470: bool PNGCodec::Encode(const unsigned char* input, Can we remove this API and make callers use the SkPIxmap version? https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:510: kGray_8_SkColorType, kOpaque_SkAlphaType); Don't forget the SkColorSpace. What about info = input.info().makeColorSpace(kGray_8_SkColorType)? Can we also assert that input.colorType() is kAlpha_8_SkColorType? https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec_... File ui/gfx/codec/png_codec_unittest.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec_... ui/gfx/codec/png_codec_unittest.cc:289: ASSERT_TRUE(PNGCodec::Encode(&original[0], PNGCodec::FORMAT_RGBA, Size(w, h), Why is this file changed?
https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:6: #include "ui/gfx/codec/vector_wstream.h" nit: vector_wstream should be in the block of includes below. only <filename>.h should be above the C includes.
Thank you everyone for the helpful comments! Please see the fixes below. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:6: #include "ui/gfx/codec/vector_wstream.h" On 2017/06/23 22:55:18, Elliot Glaysher wrote: > nit: vector_wstream should be in the block of includes below. only <filename>.h > should be above the C includes. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:425: int zLibLevel = Z_DEFAULT_COMPRESSION) { On 2017/06/23 21:45:55, dcheng (OOO Jun 25 - Jul 1) wrote: > Also, another optional nit is that I think it was clearer when we didn't use > default args and just wrote it out. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:425: int zLibLevel = Z_DEFAULT_COMPRESSION) { On 2017/06/23 21:45:55, dcheng (OOO Jun 25 - Jul 1) wrote: > Nit: naming (same throughout for unpremulInfo, unpremulPixmap, opaqueBitmap, > etc) Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:431: options.fZLibLevel = zLibLevel == Z_DEFAULT_COMPRESSION ? 6 : zLibLevel; On 2017/06/23 21:45:55, dcheng (OOO Jun 25 - Jul 1) wrote: > Why do we override Z_DEFAULT_COMPRESSION here? We should have a comment at > least. Seems like maybe the skia routines should just do this default mapping > internally to avoid having to update all the callers if it ever changes. Acknowledged. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:431: options.fZLibLevel = zLibLevel == Z_DEFAULT_COMPRESSION ? 6 : zLibLevel; On 2017/06/23 22:50:31, msarett1 wrote: > On 2017/06/23 21:45:55, dcheng wrote: > > Why do we override Z_DEFAULT_COMPRESSION here? We should have a comment at > > least. Seems like maybe the skia routines should just do this default mapping > > internally to avoid having to update all the callers if it ever changes. > > What if we have a gfx::PngCodec::kDefaultZLibCompression = 6? That way gfx > behavior doesn't change if zlib decides to change what Z_DEFAULT_COMPRESSION > compression corresponds to. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:440: if (!discard_transparency) { On 2017/06/23 22:50:31, msarett1 wrote: > nit: I find this more readable as if (discard_transparency). Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:442: } else { On 2017/06/23 22:50:31, msarett1 wrote: > nit: No need for an else since we return in the if. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:447: SkImageInfo unpremulInfo = On 2017/06/23 22:50:31, msarett1 wrote: > unpremulInfo = src.info().makeAlphaType(kUnpremul_SkAlphaType) Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:455: if (!unpremulCopy.peekPixels(&unpremulPixmap)) { On 2017/06/23 22:50:31, msarett1 wrote: > Just assert that peekPixels() succeeds. It must because you've already > allocated pixels. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:458: src.readPixels(unpremulPixmap); On 2017/06/23 22:50:32, msarett1 wrote: > Also assert that this succeeds. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:463: SkPixmap opaquePixmap(opaqueInfo, unpremulPixmap.addr(), On 2017/06/23 22:50:31, msarett1 wrote: > Hmm maybe just call tryAllocPixels() using an opaque info. > > Then readPixels(opaquePixmap.info().makeAlphaType(kUnpremul), > opaquePixmap.addr(), opaquePixmap.rowBytes()). > > Then you only need to put one pixmap on the stack. Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:470: bool PNGCodec::Encode(const unsigned char* input, On 2017/06/23 22:50:31, msarett1 wrote: > Can we remove this API and make callers use the SkPIxmap version? I prefer to do it in another CL because it will change many files. I'd like to keep the size of this CL not too large. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:510: kGray_8_SkColorType, kOpaque_SkAlphaType); On 2017/06/23 22:50:31, msarett1 wrote: > Don't forget the SkColorSpace. What about info = > input.info().makeColorSpace(kGray_8_SkColorType)? > > Can we also assert that input.colorType() is kAlpha_8_SkColorType? Done. https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec_... File ui/gfx/codec/png_codec_unittest.cc (right): https://codereview.chromium.org/2944633002/diff/20001/ui/gfx/codec/png_codec_... ui/gfx/codec/png_codec_unittest.cc:289: ASSERT_TRUE(PNGCodec::Encode(&original[0], PNGCodec::FORMAT_RGBA, Size(w, h), On 2017/06/23 22:50:32, msarett1 wrote: > Why is this file changed? I've reverted the change. I think it's because that I changed it in an earlier version, and my revert isn't complete.
Ping...
Ping again.
(Almost everyone is out on vacation, probably until the 17th because of No Meetings Week.) https://codereview.chromium.org/2944633002/diff/60001/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (right): https://codereview.chromium.org/2944633002/diff/60001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.cc:6: #include "ui/gfx/codec/vector_wstream.h" this shouldn't be here, it should be in the include section. only the header for the current file goes above the system headers.
Thank you Elliot. Sorry that I didn't notice the no-meeting weeks. Take your time and wish you all have a nice vacation! https://codereview.chromium.org/2944633002/diff/60001/ui/gfx/codec/jpeg_codec.cc File ui/gfx/codec/jpeg_codec.cc (right): https://codereview.chromium.org/2944633002/diff/60001/ui/gfx/codec/jpeg_codec... ui/gfx/codec/jpeg_codec.cc:6: #include "ui/gfx/codec/vector_wstream.h" On 2017/07/10 17:59:07, Elliot Glaysher wrote: > this shouldn't be here, it should be in the include section. only the header for > the current file goes above the system headers. Done.
https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:447: SkASSERT(copy.peekPixels(&opaque_pixmap)); This only happens in debug. Maybe you want: bool success = copy.peekPixels(&opaque_pixmap); DCHECK(success); https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:451: SkASSERT(src.readPixels(opaque_info.makeAlphaType(kUnpremul_SkAlphaType), Again, this only happens in debug, and we should be using DCHECK anyway. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:468: format == FORMAT_RGBA ? kRGBA_8888_SkColorType : kBGRA_8888_SkColorType; Just to make sure I understand - if format is FORMAT_SkBitmap, we also want kBGRA_8888? The header says it's treated as kARGB_8888_Config (probably needs updating). https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:486: std::vector<PNGCodec::Comment> empty_comments; Too bad we need to create this empty vector just to ignore it. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:502: SkASSERT(input.colorType() == kAlpha_8_SkColorType); DCHECK. There might be a DCHECK_EQ, so this could be DCHECK_EQ(input.colorType(), kAlpha_8_SkColorType); https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:506: SkPixmap src(info, input.getAddr(0, 0), input.rowBytes()); You could instead use readPixels here, as you do elsewhere. Not sure why to choose one or the other.
Thanks Leon for the helpful comments. I clearly forgot to run the unit tests in release mode. Now it's fixed and tested in release mode as well as debug mode. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:447: SkASSERT(copy.peekPixels(&opaque_pixmap)); On 2017/07/11 16:01:06, scroggo_chromium wrote: > This only happens in debug. Maybe you want: > > bool success = copy.peekPixels(&opaque_pixmap); > DCHECK(success); Done. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:451: SkASSERT(src.readPixels(opaque_info.makeAlphaType(kUnpremul_SkAlphaType), On 2017/07/11 16:01:06, scroggo_chromium wrote: > Again, this only happens in debug, and we should be using DCHECK anyway. Done. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:468: format == FORMAT_RGBA ? kRGBA_8888_SkColorType : kBGRA_8888_SkColorType; On 2017/07/11 16:01:06, scroggo_chromium wrote: > Just to make sure I understand - if format is FORMAT_SkBitmap, we also want > kBGRA_8888? The header says it's treated as kARGB_8888_Config (probably needs > updating). I'm also very confused because: 1. The method is called EncodeBGRASkBitmap but the comment says kARGB_8888_Config. 2. kARGB_8888_Config seems to no longer exist in Skia except in some experimental code Therefore, I'm going to change kARGB_8888_Config to kBGRA_8888_SkColorType in all comments in this file. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:486: std::vector<PNGCodec::Comment> empty_comments; On 2017/07/11 16:01:06, scroggo_chromium wrote: > Too bad we need to create this empty vector just to ignore it. Agree. I can use {} in Mac but Linux can only compile with this ugly empty_comments... https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:502: SkASSERT(input.colorType() == kAlpha_8_SkColorType); On 2017/07/11 16:01:06, scroggo_chromium wrote: > DCHECK. There might be a DCHECK_EQ, so this could be > > DCHECK_EQ(input.colorType(), kAlpha_8_SkColorType); Done. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:506: SkPixmap src(info, input.getAddr(0, 0), input.rowBytes()); On 2017/07/11 16:01:06, scroggo_chromium wrote: > You could instead use readPixels here, as you do elsewhere. Not sure why to > choose one or the other. In the other place, we need to do unpremul and copy the pixels into a new location so readPixels is used; here, we don't have to modify the memory and we're not copying pixels, so readPixels is not used.
https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:468: format == FORMAT_RGBA ? kRGBA_8888_SkColorType : kBGRA_8888_SkColorType; On 2017/07/11 19:24:55, liyuqian wrote: > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > Just to make sure I understand - if format is FORMAT_SkBitmap, we also want > > kBGRA_8888? The header says it's treated as kARGB_8888_Config (probably needs > > updating). > > I'm also very confused because: > > 1. The method is called EncodeBGRASkBitmap but the comment says > kARGB_8888_Config. > > 2. kARGB_8888_Config seems to no longer exist in Skia except in some > experimental code Yeah, Configs were replaced with SkColorTypes before you got here. When we had kARGB_8888, I think it corresponded to whichever one was the native one. > > Therefore, I'm going to change kARGB_8888_Config to kBGRA_8888_SkColorType in > all comments in this file. Sgtm, so long as we don't accidentally flip the behavior! https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:486: std::vector<PNGCodec::Comment> empty_comments; On 2017/07/11 19:24:55, liyuqian wrote: > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > Too bad we need to create this empty vector just to ignore it. > > Agree. I can use {} in Mac but Linux can only compile with this ugly > empty_comments... Maybe it should take a pointer, which can be null? https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:506: SkPixmap src(info, input.getAddr(0, 0), input.rowBytes()); On 2017/07/11 19:24:55, liyuqian wrote: > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > You could instead use readPixels here, as you do elsewhere. Not sure why to > > choose one or the other. > > In the other place, we need to do unpremul and copy the pixels into a new > location so readPixels is used; here, we don't have to modify the memory and > we're not copying pixels, so readPixels is not used. Oh, oops, you are correct. https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec.h File ui/gfx/codec/png_codec.h (right): https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.h:40: // SkBitmap format. For Encode() kBGRA_8888_SkColorType (4 bytes per pixel) Actually, I think this should probably be kN32_SkColorType. https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.h:82: // to be kBGRA_8888_SkColorType, 32 bits per pixel. The params kN32
https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:468: format == FORMAT_RGBA ? kRGBA_8888_SkColorType : kBGRA_8888_SkColorType; On 2017/07/11 19:33:09, scroggo_chromium wrote: > On 2017/07/11 19:24:55, liyuqian wrote: > > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > > Just to make sure I understand - if format is FORMAT_SkBitmap, we also want > > > kBGRA_8888? The header says it's treated as kARGB_8888_Config (probably > needs > > > updating). > > > > I'm also very confused because: > > > > 1. The method is called EncodeBGRASkBitmap but the comment says > > kARGB_8888_Config. > > > > 2. kARGB_8888_Config seems to no longer exist in Skia except in some > > experimental code > > Yeah, Configs were replaced with SkColorTypes before you got here. When we had > kARGB_8888, I think it corresponded to whichever one was the native one. > > > > > Therefore, I'm going to change kARGB_8888_Config to kBGRA_8888_SkColorType in > > all comments in this file. > > Sgtm, so long as we don't accidentally flip the behavior! Acknowledged. https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:486: std::vector<PNGCodec::Comment> empty_comments; On 2017/07/11 19:33:09, scroggo_chromium wrote: > On 2017/07/11 19:24:55, liyuqian wrote: > > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > > Too bad we need to create this empty vector just to ignore it. > > > > Agree. I can use {} in Mac but Linux can only compile with this ugly > > empty_comments... > > Maybe it should take a pointer, which can be null? That sounds good. However, it will require the public API to change and many places that use this API will need to be changed. I'd prefer to postpone those changes to a later CL so this CL doesn't get too big. Anyway, I just realized that I can use `std::vector<PNGCodec::Comment>()` instead of empty_comments. (It's strange that `{}` won't work but `std::vector<PNGCodec::Comment>()` will.) https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec.h File ui/gfx/codec/png_codec.h (right): https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.h:40: // SkBitmap format. For Encode() kBGRA_8888_SkColorType (4 bytes per pixel) On 2017/07/11 19:33:09, scroggo_chromium wrote: > Actually, I think this should probably be kN32_SkColorType. Done. https://codereview.chromium.org/2944633002/diff/120001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.h:82: // to be kBGRA_8888_SkColorType, 32 bits per pixel. The params On 2017/07/11 19:33:09, scroggo_chromium wrote: > kN32 Done.
lgtm https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/80001/ui/gfx/codec/png_codec.... ui/gfx/codec/png_codec.cc:486: std::vector<PNGCodec::Comment> empty_comments; On 2017/07/11 19:59:25, liyuqian wrote: > On 2017/07/11 19:33:09, scroggo_chromium wrote: > > On 2017/07/11 19:24:55, liyuqian wrote: > > > On 2017/07/11 16:01:06, scroggo_chromium wrote: > > > > Too bad we need to create this empty vector just to ignore it. > > > > > > Agree. I can use {} in Mac but Linux can only compile with this ugly > > > empty_comments... > > > > Maybe it should take a pointer, which can be null? > > That sounds good. However, it will require the public API to change and many > places that use this API will need to be changed. I'd prefer to postpone those > changes to a later CL so this CL doesn't get too big. Anyway, I just realized > that I can use `std::vector<PNGCodec::Comment>()` instead of empty_comments. > (It's strange that `{}` won't work but `std::vector<PNGCodec::Comment>()` will.) Sgtm https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.cc:478: default: I think default is unnecessary, since you've covered all the cases.
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec... ui/gfx/codec/png_codec.cc:478: default: On 2017/07/11 20:09:30, scroggo_chromium wrote: > I think default is unnecessary, since you've covered all the cases. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/07/11 20:21:27, liyuqian wrote: > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec.cc > File ui/gfx/codec/png_codec.cc (right): > > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec... > ui/gfx/codec/png_codec.cc:478: default: > On 2017/07/11 20:09:30, scroggo_chromium wrote: > > I think default is unnecessary, since you've covered all the cases. > > Done. (send out a ping when you have windows compiling and i'll 'l''t''g''m''.)
On 2017/07/11 20:21:27, liyuqian wrote: > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec.cc > File ui/gfx/codec/png_codec.cc (right): > > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec... > ui/gfx/codec/png_codec.cc:478: default: > On 2017/07/11 20:09:30, scroggo_chromium wrote: > > I think default is unnecessary, since you've covered all the cases. > > Done. (send out a ping when you have windows compiling and i'll 'l''t''g''m''.)
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/12 18:14:00, Elliot Glaysher wrote: > On 2017/07/11 20:21:27, liyuqian wrote: > > > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec.cc > > File ui/gfx/codec/png_codec.cc (right): > > > > > https://codereview.chromium.org/2944633002/diff/140001/ui/gfx/codec/png_codec... > > ui/gfx/codec/png_codec.cc:478: default: > > On 2017/07/11 20:09:30, scroggo_chromium wrote: > > > I think default is unnecessary, since you've covered all the cases. > > > > Done. > > (send out a ping when you have windows compiling and i'll 'l''t''g''m''.) Windows is compiling :)
lgtm
The CQ bit was checked by liyuqian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org Link to the patchset: https://codereview.chromium.org/2944633002/#ps200001 (title: "Cast")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499967578475230,
"parent_rev": "3806f787a982a9187d1ad2ec414ad64eac67d39d", "commit_rev":
"88e6a1793be2e7cce6c07a43f3147b80325a18a7"}
Message was sent while issue was closed.
Description was changed from ========== Use SkPngEncoder in gfx jpeg_codec BUG=724616 ========== to ========== Use SkPngEncoder in gfx jpeg_codec BUG=724616 Review-Url: https://codereview.chromium.org/2944633002 Cr-Commit-Position: refs/heads/master@{#486433} Committed: https://chromium.googlesource.com/chromium/src/+/88e6a1793be2e7cce6c07a43f314... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/88e6a1793be2e7cce6c07a43f314... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
