Use SkPngEncoder in gfx jpeg_codec

ui/gfx/codec/png_codec.cc

Index: ui/gfx/codec/png_codec.cc
diff --git a/ui/gfx/codec/png_codec.cc b/ui/gfx/codec/png_codec.cc
index fe4020596fc9c3ba32609719060a727554b07e81..956e89d9ba1b24a90e6cca073034df6f4e43c897 100644
--- a/ui/gfx/codec/png_codec.cc
+++ b/ui/gfx/codec/png_codec.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/codec/vector_wstream.h"

[Elliot Glaysher, 2017/06/23 22:55:18]

nit: vector_wstream should be in the block of includes below. only <filename>.h
should be above the C includes.

[liyuqian, 2017/07/05 15:59:39]

Done.

 
 #include <stdint.h>
 
@@ -13,60 +14,13 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColorPriv.h"
 #include "third_party/skia/include/core/SkUnPreMultiply.h"
+#include "third_party/skia/include/encode/SkPngEncoder.h"
 #include "third_party/zlib/zlib.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/skia_util.h"
 
 namespace gfx {
 
-namespace {
-
-// Converts BGRA->RGBA and RGBA->BGRA.
-void ConvertBetweenBGRAandRGBA(const unsigned char* input, int pixel_width,
-                               unsigned char* output, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const unsigned char* pixel_in = &input[x * 4];
-    unsigned char* pixel_out = &output[x * 4];
-    pixel_out[0] = pixel_in[2];
-    pixel_out[1] = pixel_in[1];
-    pixel_out[2] = pixel_in[0];
-    pixel_out[3] = pixel_in[3];
-  }
-}
-
-void ConvertRGBAtoRGB(const unsigned char* rgba, int pixel_width,
-                      unsigned char* rgb, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++)
-    memcpy(&rgb[x * 3], &rgba[x * 4], 3);
-}
-
-void ConvertSkiaToRGB(const unsigned char* skia, int pixel_width,
-                      unsigned char* rgb, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const uint32_t pixel_in = *reinterpret_cast<const uint32_t*>(&skia[x * 4]);
-    unsigned char* pixel_out = &rgb[x * 3];
-
-    int alpha = SkGetPackedA32(pixel_in);
-    if (alpha != 0 && alpha != 255) {
-      SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(pixel_in);
-      pixel_out[0] = SkColorGetR(unmultiplied);
-      pixel_out[1] = SkColorGetG(unmultiplied);
-      pixel_out[2] = SkColorGetB(unmultiplied);
-    } else {
-      pixel_out[0] = SkGetPackedR32(pixel_in);
-      pixel_out[1] = SkGetPackedG32(pixel_in);
-      pixel_out[2] = SkGetPackedB32(pixel_in);
-    }
-  }
-}
-
-void ConvertSkiaToRGBA(const unsigned char* skia, int pixel_width,
-                       unsigned char* rgba, bool* is_opaque) {
-  gfx::ConvertSkiaToRGBA(skia, pixel_width, rgba);
-}
-
-}  // namespace
-
 // Decoder --------------------------------------------------------------------
 //
 // This code is based on WebKit libpng interface (PNGImageDecoder), which is
@@ -369,15 +323,6 @@ void LogLibPNGDecodeWarning(png_structp png_ptr, png_const_charp warning_msg) {
   DLOG(ERROR) << "libpng decode warning: " << warning_msg;
 }
 
-void LogLibPNGEncodeError(png_structp png_ptr, png_const_charp error_msg) {
-  DLOG(ERROR) << "libpng encode error: " << error_msg;
-  longjmp(png_jmpbuf(png_ptr), 1);
-}
-
-void LogLibPNGEncodeWarning(png_structp png_ptr, png_const_charp warning_msg) {
-  DLOG(ERROR) << "libpng encode warning: " << warning_msg;
-}
-
 }  // namespace
 
 // static
@@ -454,287 +399,73 @@ bool PNGCodec::Decode(const unsigned char* input, size_t input_size,
 }
 
 // Encoder --------------------------------------------------------------------
-//
-// This section of the code is based on nsPNGEncoder.cpp in Mozilla
-// (Copyright 2005 Google Inc.)
 
 namespace {
 
-// Passed around as the io_ptr in the png structs so our callbacks know where
-// to write data.
-struct PngEncoderState {
-  explicit PngEncoderState(std::vector<unsigned char>* o) : out(o) {}
-  std::vector<unsigned char>* out;
-};
-
-// Called by libpng to flush its internal buffer to ours.
-void EncoderWriteCallback(png_structp png, png_bytep data, png_size_t size) {
-  PngEncoderState* state = static_cast<PngEncoderState*>(png_get_io_ptr(png));
-  DCHECK(state->out);
-
-  size_t old_size = state->out->size();
-  state->out->resize(old_size + size);
-  memcpy(&(*state->out)[old_size], data, size);
-}
-
-void FakeFlushCallback(png_structp png) {
-  // We don't need to perform any flushing since we aren't doing real IO, but
-  // we're required to provide this function by libpng.
-}
-
-void ConvertBGRAtoRGB(const unsigned char* bgra, int pixel_width,
-                      unsigned char* rgb, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const unsigned char* pixel_in = &bgra[x * 4];
-    unsigned char* pixel_out = &rgb[x * 3];
-    pixel_out[0] = pixel_in[2];
-    pixel_out[1] = pixel_in[1];
-    pixel_out[2] = pixel_in[0];
+static void AddComments(SkPngEncoder::Options& options,
+                        const std::vector<PNGCodec::Comment>& comments) {
+  std::vector<const char*> commentPointers;
+  std::vector<size_t> commentSizes;
+  for (const auto& comment : comments) {
+    commentPointers.push_back(comment.key.c_str());
+    commentPointers.push_back(comment.text.c_str());
+    commentSizes.push_back(comment.key.length() + 1);
+    commentSizes.push_back(comment.text.length() + 1);
   }
+  options.fComments =
+      SkDataTable::MakeCopyArrays((void const* const*)commentPointers.data(),
+                                  commentSizes.data(), commentPointers.size());
 }
 
-#ifdef PNG_TEXT_SUPPORTED
-class CommentWriter {
- public:
-  explicit CommentWriter(const std::vector<PNGCodec::Comment>& comments)
-      : comments_(comments),
-        png_text_(new png_text[comments.size()]) {
-    for (size_t i = 0; i < comments.size(); ++i)
-      AddComment(i, comments[i]);
-  }
-
-  ~CommentWriter() {
-    for (size_t i = 0; i < comments_.size(); ++i) {
-      free(png_text_[i].key);
-      free(png_text_[i].text);
-    }
-    delete [] png_text_;
-  }
-
-  bool HasComments() {
-    return !comments_.empty();
-  }
-
-  png_text* get_png_text() {
-    return png_text_;
-  }
-
-  int size() {
-    return static_cast<int>(comments_.size());
-  }
-
- private:
-  void AddComment(size_t pos, const PNGCodec::Comment& comment) {
-    png_text_[pos].compression = PNG_TEXT_COMPRESSION_NONE;
-    // A PNG comment's key can only be 79 characters long.
-    DCHECK(comment.key.length() < 79);
-    png_text_[pos].key = base::strdup(comment.key.substr(0, 78).c_str());
-    png_text_[pos].text = base::strdup(comment.text.c_str());
-    png_text_[pos].text_length = comment.text.length();
-#ifdef PNG_iTXt_SUPPORTED
-    png_text_[pos].itxt_length = 0;
-    png_text_[pos].lang = 0;
-    png_text_[pos].lang_key = 0;
-#endif
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(CommentWriter);
-
-  const std::vector<PNGCodec::Comment> comments_;
-  png_text* png_text_;
-};
-#endif  // PNG_TEXT_SUPPORTED
-
-// The type of functions usable for converting between pixel formats.
-typedef void (*FormatConverter)(const unsigned char* in, int w,
-                                unsigned char* out, bool* is_opaque);
-
-// libpng uses a wacky setjmp-based API, which makes the compiler nervous.
-// We constrain all of the calls we make to libpng where the setjmp() is in
-// place to this function.
-// Returns true on success.
-bool DoLibpngWrite(png_struct* png_ptr, png_info* info_ptr,
-                   PngEncoderState* state,
-                   int width, int height, int row_byte_width,
-                   const unsigned char* input, int compression_level,
-                   int png_output_color_type, int output_color_components,
-                   FormatConverter converter,
-                   const std::vector<PNGCodec::Comment>& comments) {
-#ifdef PNG_TEXT_SUPPORTED
-  CommentWriter comment_writer(comments);
-#endif
-  unsigned char* row_buffer = NULL;
-
-  // Make sure to not declare any locals here -- locals in the presence
-  // of setjmp() in C++ code makes gcc complain.
-
-  if (setjmp(png_jmpbuf(png_ptr))) {
-    delete[] row_buffer;
-    return false;
-  }
-
-  png_set_compression_level(png_ptr, compression_level);
-
-  // Set our callback for libpng to give us the data.
-  png_set_write_fn(png_ptr, state, EncoderWriteCallback, FakeFlushCallback);
-  png_set_error_fn(png_ptr, NULL, LogLibPNGEncodeError, LogLibPNGEncodeWarning);
-
-  png_set_IHDR(png_ptr, info_ptr, width, height, 8, png_output_color_type,
-               PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
-               PNG_FILTER_TYPE_DEFAULT);
+}  // namespace
 
-#ifdef PNG_TEXT_SUPPORTED
-  if (comment_writer.HasComments()) {
-    png_set_text(png_ptr, info_ptr, comment_writer.get_png_text(),
-                 comment_writer.size());
-  }
-#endif
+static bool EncodeSkPixmap(const SkPixmap& src,
+                           const std::vector<PNGCodec::Comment>& comments,
+                           std::vector<unsigned char>* output,
+                           int zLibLevel = Z_DEFAULT_COMPRESSION) {

[dcheng, 2017/06/23 21:45:55]

Also, another optional nit is that I think it was clearer when we didn't use
default args and just wrote it out.

[dcheng, 2017/06/23 21:45:55]

Nit: naming (same throughout for unpremulInfo, unpremulPixmap, opaqueBitmap,
etc)

[liyuqian, 2017/07/05 15:59:39]

Done.

[liyuqian, 2017/07/05 15:59:39]

Done.

+  output->clear();
+  VectorWStream dst(output);
 
-  png_write_info(png_ptr, info_ptr);
+  SkPngEncoder::Options options;
+  AddComments(options, comments);
+  options.fZLibLevel = zLibLevel == Z_DEFAULT_COMPRESSION ? 6 : zLibLevel;

[dcheng, 2017/06/23 21:45:55]

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.

[msarett1, 2017/06/23 22:50:31]

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.

[liyuqian, 2017/07/05 15:59:39]

Acknowledged.

[liyuqian, 2017/07/05 15:59:39]

Done.

+  return SkPngEncoder::Encode(&dst, src, options);
+}
 
-  if (!converter) {
-    // No conversion needed, give the data directly to libpng.
-    for (int y = 0; y < height; y ++) {
-      png_write_row(png_ptr,
-                    const_cast<unsigned char*>(&input[y * row_byte_width]));
-    }
+static bool EncodeSkPixmap(const SkPixmap& src,
+                           bool discard_transparency,
+                           const std::vector<PNGCodec::Comment>& comments,
+                           std::vector<unsigned char>* output,
+                           int zLibLevel = Z_DEFAULT_COMPRESSION) {
+  if (!discard_transparency) {

[msarett1, 2017/06/23 22:50:31]

nit: I find this more readable as if (discard_transparency).

[liyuqian, 2017/07/05 15:59:39]

Done.

+    return EncodeSkPixmap(src, comments, output, zLibLevel);
   } else {

[msarett1, 2017/06/23 22:50:31]

nit: No need for an else since we return in the if.

[liyuqian, 2017/07/05 15:59:39]

Done.

-    // Needs conversion using a separate buffer.
-    row_buffer = new unsigned char[width * output_color_components];
-    for (int y = 0; y < height; y ++) {
-      converter(&input[y * row_byte_width], width, row_buffer, NULL);
-      png_write_row(png_ptr, row_buffer);
+    auto w = src.info().width();
+    auto h = src.info().height();
+    auto ct = src.info().colorType();
+    auto cs = src.info().refColorSpace();
+    SkImageInfo unpremulInfo =

[msarett1, 2017/06/23 22:50:31]

unpremulInfo = src.info().makeAlphaType(kUnpremul_SkAlphaType)

[liyuqian, 2017/07/05 15:59:39]

Done.

+        SkImageInfo::Make(w, h, ct, kUnpremul_SkAlphaType, cs);
+
+    SkBitmap unpremulCopy;
+    if (!unpremulCopy.tryAllocPixels(unpremulInfo)) {
+      return false;
     }
-    delete[] row_buffer;
-  }
-
-  png_write_end(png_ptr, info_ptr);
-  return true;
-}
-
-bool EncodeWithCompressionLevel(const unsigned char* input,
-                                PNGCodec::ColorFormat format,
-                                const Size& size,
-                                int row_byte_width,
-                                bool discard_transparency,
-                                const std::vector<PNGCodec::Comment>& comments,
-                                int compression_level,
-                                std::vector<unsigned char>* output) {
-  // Run to convert an input row into the output row format, NULL means no
-  // conversion is necessary.
-  FormatConverter converter = NULL;
-
-  int input_color_components, output_color_components;
-  int png_output_color_type;
-  switch (format) {
-    case PNGCodec::FORMAT_RGBA:
-      input_color_components = 4;
-      if (discard_transparency) {
-        output_color_components = 3;
-        png_output_color_type = PNG_COLOR_TYPE_RGB;
-        converter = ConvertRGBAtoRGB;
-      } else {
-        output_color_components = 4;
-        png_output_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
-        converter = NULL;
-      }
-      break;
-
-    case PNGCodec::FORMAT_BGRA:
-      input_color_components = 4;
-      if (discard_transparency) {
-        output_color_components = 3;
-        png_output_color_type = PNG_COLOR_TYPE_RGB;
-        converter = ConvertBGRAtoRGB;
-      } else {
-        output_color_components = 4;
-        png_output_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
-        converter = ConvertBetweenBGRAandRGBA;
-      }
-      break;
-
-    case PNGCodec::FORMAT_SkBitmap:
-      // Compare row_byte_width and size.width() to detect the format of
-      // SkBitmap. kA8_Config (1bpp) and kARGB_8888_Config (4bpp) are the two
-      // supported formats.
-      if (row_byte_width < 4 * size.width()) {
-        // Not 4bpp, so must be 1bpp.
-        // Ignore discard_transparency - it doesn't make sense in this context,
-        // since alpha is the only thing we have and it needs to be used for
-        // color intensity.
-        input_color_components = 1;
-        output_color_components = 1;
-        png_output_color_type = PNG_COLOR_TYPE_GRAY;
-        // |converter| is left as null
-      } else {
-        input_color_components = 4;
-        if (discard_transparency) {
-          output_color_components = 3;
-          png_output_color_type = PNG_COLOR_TYPE_RGB;
-          converter = ConvertSkiaToRGB;
-        } else {
-          output_color_components = 4;
-          png_output_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
-          converter = ConvertSkiaToRGBA;
-        }
-      }
-      break;
-
-    default:
-      NOTREACHED() << "Unknown pixel format";
+    SkPixmap unpremulPixmap;
+    if (!unpremulCopy.peekPixels(&unpremulPixmap)) {

[msarett1, 2017/06/23 22:50:31]

Just assert that peekPixels() succeeds.  It must because you've already
allocated pixels.

[liyuqian, 2017/07/05 15:59:39]

Done.

       return false;
-  }
-
-  // Row stride should be at least as long as the length of the data.
-  DCHECK(input_color_components * size.width() <= row_byte_width);
-
-  PngWriteStructInfo si;
-  si.png_ptr_ = png_create_write_struct(PNG_LIBPNG_VER_STRING,
-                                        NULL, NULL, NULL);
-  if (!si.png_ptr_)
-    return false;
-
-  si.info_ptr_ = png_create_info_struct(si.png_ptr_);
-  if (!si.info_ptr_)
-    return false;
-
-  output->clear();
-
-  PngEncoderState state(output);
-  bool success = DoLibpngWrite(si.png_ptr_, si.info_ptr_, &state,
-                               size.width(), size.height(), row_byte_width,
-                               input, compression_level, png_output_color_type,
-                               output_color_components, converter, comments);
+    }
+    src.readPixels(unpremulPixmap);

[msarett1, 2017/06/23 22:50:32]

Also assert that this succeeds.

[liyuqian, 2017/07/05 15:59:38]

Done.

 
-  return success;
-}
+    SkImageInfo opaqueInfo =
+        SkImageInfo::Make(w, h, ct, kOpaque_SkAlphaType, cs);
 
-bool InternalEncodeSkBitmap(const SkBitmap& input,
-                            bool discard_transparency,
-                            int compression_level,
-                            std::vector<unsigned char>* output) {
-  if (input.empty() || input.isNull())
-    return false;
-  int bpp = input.bytesPerPixel();
-  DCHECK(bpp == 1 || bpp == 4);  // We support kA8_Config and kARGB_8888_Config.
-
-  unsigned char* inputAddr = bpp == 1 ?
-      reinterpret_cast<unsigned char*>(input.getAddr8(0, 0)) :
-      reinterpret_cast<unsigned char*>(input.getAddr32(0, 0));    // bpp = 4
-  return EncodeWithCompressionLevel(
-      inputAddr,
-      PNGCodec::FORMAT_SkBitmap,
-      Size(input.width(), input.height()),
-      static_cast<int>(input.rowBytes()),
-      discard_transparency,
-      std::vector<PNGCodec::Comment>(),
-      compression_level,
-      output);
+    SkPixmap opaquePixmap(opaqueInfo, unpremulPixmap.addr(),

[msarett1, 2017/06/23 22:50:31]

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.

[liyuqian, 2017/07/05 15:59:38]

Done.

+                          unpremulPixmap.rowBytes(), unpremulPixmap.ctable());
+    return EncodeSkPixmap(opaquePixmap, comments, output, zLibLevel);
+  }
 }
 
-
-}  // namespace
-
 // static
 bool PNGCodec::Encode(const unsigned char* input,

[msarett1, 2017/06/23 22:50:31]

Can we remove this API and make callers use the SkPIxmap version? 

[liyuqian, 2017/07/05 15:59:38]

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.

                       ColorFormat format,
@@ -743,43 +474,49 @@ bool PNGCodec::Encode(const unsigned char* input,
                       bool discard_transparency,
                       const std::vector<Comment>& comments,
                       std::vector<unsigned char>* output) {
-  return EncodeWithCompressionLevel(input,
-                                    format,
-                                    size,
-                                    row_byte_width,
-                                    discard_transparency,
-                                    comments,
-                                    Z_DEFAULT_COMPRESSION,
-                                    output);
+  auto colorType =
+      format == FORMAT_RGBA ? kRGBA_8888_SkColorType : kBGRA_8888_SkColorType;
+  auto alphaType =
+      format == FORMAT_SkBitmap ? kPremul_SkAlphaType : kUnpremul_SkAlphaType;
+  SkImageInfo info =
+      SkImageInfo::Make(size.width(), size.height(), colorType, alphaType);
+  SkPixmap src(info, input, row_byte_width);
+  return EncodeSkPixmap(src, discard_transparency, comments, output);
+}
+
+static bool EncodeSkBitmap(const SkBitmap& input,
+                           bool discard_transparency,
+                           std::vector<unsigned char>* output,
+                           int zLibLevel = Z_DEFAULT_COMPRESSION) {
+  SkPixmap src;
+  if (!input.peekPixels(&src)) {
+    return false;
+  }
+  return EncodeSkPixmap(src, discard_transparency, {}, output, zLibLevel);
 }
 
 // static
 bool PNGCodec::EncodeBGRASkBitmap(const SkBitmap& input,
                                   bool discard_transparency,
                                   std::vector<unsigned char>* output) {
-  return InternalEncodeSkBitmap(input,
-                                discard_transparency,
-                                Z_DEFAULT_COMPRESSION,
-                                output);
+  return EncodeSkBitmap(input, discard_transparency, output);
 }
 
 // static
 bool PNGCodec::EncodeA8SkBitmap(const SkBitmap& input,
                                 std::vector<unsigned char>* output) {
-  return InternalEncodeSkBitmap(input,
-                                false,
-                                Z_DEFAULT_COMPRESSION,
-                                output);
+  SkImageInfo info =
+      SkImageInfo::Make(input.info().width(), input.info().height(),
+                        kGray_8_SkColorType, kOpaque_SkAlphaType);

[msarett1, 2017/06/23 22:50:31]

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?

[liyuqian, 2017/07/05 15:59:39]

Done.

+  SkPixmap src(info, input.getAddr(0, 0), input.rowBytes());
+  return EncodeSkPixmap(src, {}, output);
 }
 
 // static
 bool PNGCodec::FastEncodeBGRASkBitmap(const SkBitmap& input,
                                       bool discard_transparency,
                                       std::vector<unsigned char>* output) {
-  return InternalEncodeSkBitmap(input,
-                                discard_transparency,
-                                Z_BEST_SPEED,
-                                output);
+  return EncodeSkBitmap(input, discard_transparency, output, Z_BEST_SPEED);
 }
 
 PNGCodec::Comment::Comment(const std::string& k, const std::string& t)