Support for interlaced PNGs (in gfx::PNGCodec)

ui/gfx/codec/png_codec.cc

Index: ui/gfx/codec/png_codec.cc
===================================================================
--- ui/gfx/codec/png_codec.cc	(revision 123937)
+++ ui/gfx/codec/png_codec.cc	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -30,58 +30,6 @@
 
 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++) {
-    const unsigned char* pixel_in = &rgba[x * 4];
-    unsigned char* pixel_out = &rgb[x * 3];
-    pixel_out[0] = pixel_in[0];
-    pixel_out[1] = pixel_in[1];
-    pixel_out[2] = pixel_in[2];
-  }
-}

[Francois, 2012/02/28 15:00:36]

The work done by this converter and the one above it is now done by libpng
itself (e.g. by png_set_strip_alpha and png_set_bgr).

-
-void ConvertRGBtoSkia(const unsigned char* rgb, int pixel_width,
-                      unsigned char* rgba, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const unsigned char* pixel_in = &rgb[x * 3];
-    uint32_t* pixel_out = reinterpret_cast<uint32_t*>(&rgba[x * 4]);
-    *pixel_out = SkPackARGB32(0xFF, pixel_in[0], pixel_in[1], pixel_in[2]);
-  }
-}
-
-void ConvertRGBAtoSkia(const unsigned char* rgb, int pixel_width,
-                       unsigned char* rgba, bool* is_opaque) {
-  int total_length = pixel_width * 4;
-  for (int x = 0; x < total_length; x += 4) {
-    const unsigned char* pixel_in = &rgb[x];
-    uint32_t* pixel_out = reinterpret_cast<uint32_t*>(&rgba[x]);
-
-    unsigned char alpha = pixel_in[3];
-    if (alpha != 255) {
-      *is_opaque = false;
-      *pixel_out = SkPreMultiplyARGB(alpha,
-           pixel_in[0], pixel_in[1], pixel_in[2]);
-    } else {
-      *pixel_out = SkPackARGB32(alpha,
-           pixel_in[0], pixel_in[1], pixel_in[2]);
-    }
-  }
-}

[Francois, 2012/02/28 15:00:36]

The work done by this converter and the one above it is now done by built-in
libpng transforms (png_set_add_alpha to convert from RGB to RGBA) and a single
libpng user transform called ConvertRGBARowToSkia.

-
 void ConvertSkiatoRGB(const unsigned char* skia, int pixel_width,
                       unsigned char* rgb, bool* is_opaque) {
   for (int x = 0; x < pixel_width; x++) {
@@ -148,7 +96,6 @@
         bitmap(NULL),
         is_opaque(true),
         output(o),
-        row_converter(NULL),
         width(0),
         height(0),
         done(false) {
@@ -161,7 +108,6 @@
         bitmap(skbitmap),
         is_opaque(true),
         output(NULL),
-        row_converter(NULL),
         width(0),
         height(0),
         done(false) {
@@ -181,11 +127,6 @@
   // instead of directly to an SkBitmap.
   std::vector<unsigned char>* output;
 
-  // Called to convert a row from the library to the correct output format.
-  // When NULL, no conversion is necessary.
-  void (*row_converter)(const unsigned char* in, int w, unsigned char* out,
-                        bool* is_opaque);

[Francois, 2012/02/28 15:00:36]

All decode converters have been removed in favor of built-in libpng conversions
and a single user transform function for decoding to Skia.

-
   // Size of the image, set in the info callback.
   int width;
   int height;
@@ -197,27 +138,29 @@
   DISALLOW_COPY_AND_ASSIGN(PngDecoderState);
 };
 
-void ConvertRGBtoRGBA(const unsigned char* rgb, int pixel_width,
-                      unsigned char* rgba, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const unsigned char* pixel_in = &rgb[x * 3];
-    unsigned char* pixel_out = &rgba[x * 4];
-    pixel_out[0] = pixel_in[0];
-    pixel_out[1] = pixel_in[1];
-    pixel_out[2] = pixel_in[2];
-    pixel_out[3] = 0xff;

[Francois, 2012/02/28 15:00:36]

This converter's work is now done by libpng itself (png_set_add_alpha).

-  }
-}
+// User transform (passed to libpng) which converts a row decoded by libpng to
+// Skia format. Expects the row to have 4 channels, otherwise there won't be
+// enough room in |data|.
+void ConvertRGBARowToSkia(png_structp png_ptr,
+                          png_row_infop row_info,
+                          png_bytep data) {
+  const int channels = row_info->channels;
+  DCHECK_EQ(channels, 4);
 
-void ConvertRGBtoBGRA(const unsigned char* rgb, int pixel_width,
-                      unsigned char* bgra, bool* is_opaque) {
-  for (int x = 0; x < pixel_width; x++) {
-    const unsigned char* pixel_in = &rgb[x * 3];
-    unsigned char* pixel_out = &bgra[x * 4];
-    pixel_out[0] = pixel_in[2];
-    pixel_out[1] = pixel_in[1];
-    pixel_out[2] = pixel_in[0];
-    pixel_out[3] = 0xff;
+  PngDecoderState* state =
+      static_cast<PngDecoderState*>(png_get_user_transform_ptr(png_ptr));
+  DCHECK(state) << "LibPNG user transform pointer is NULL";
+
+  unsigned char* const end = data + row_info->rowbytes;
+  for (unsigned char* p = data; p < end; p += channels) {
+    uint32_t* sk_pixel = reinterpret_cast<uint32_t*>(p);
+    const unsigned char alpha = p[channels - 1];
+    if (alpha != 255) {
+      state->is_opaque = false;
+      *sk_pixel = SkPreMultiplyARGB(alpha, p[0], p[1], p[2]);
+    } else {
+      *sk_pixel = SkPackARGB32(alpha, p[0], p[1], p[2]);
+    }

[Francois, 2012/02/28 15:00:36]

Replaces the converters called ConvertRGBtoSkia and ConvertRGBAtoSkia.

   }
 }
 

[Francois, 2012/02/28 15:00:36]

This converter's work is now done by libpng itself (png_set_strip_alpha).

@@ -227,11 +170,11 @@
   PngDecoderState* state = static_cast<PngDecoderState*>(
       png_get_progressive_ptr(png_ptr));
 
-  int bit_depth, color_type, interlace_type, compression_type;
-  int filter_type, channels;
+  int bit_depth, color_type, interlace_type;
   png_uint_32 w, h;
+
   png_get_IHDR(png_ptr, info_ptr, &w, &h, &bit_depth, &color_type,
-               &interlace_type, &compression_type, &filter_type);
+               &interlace_type, NULL, NULL);
 
   // Bounds check. When the image is unreasonably big, we'll error out and
   // end up back at the setjmp call when we set up decoding.  "Unreasonably big"
@@ -242,22 +185,53 @@
       static_cast<unsigned long long>(w) * static_cast<unsigned long long>(h);
   if (total_size > ((1 << 29) - 1))
     longjmp(png_jmpbuf(png_ptr), 1);
+
   state->width = static_cast<int>(w);
   state->height = static_cast<int>(h);
 
+  // The order of the following png_set_* calls have to be done in the order
+  // dictated by the libpng docs. This is why certain things are done outside
+  // of the switch, even though they look like they belong there.
+
   // Expand to ensure we use 24-bit for RGB and 32-bit for RGBA.
   if (color_type == PNG_COLOR_TYPE_PALETTE ||
       (color_type == PNG_COLOR_TYPE_GRAY && bit_depth < 8))
     png_set_expand(png_ptr);
 
+  bool input_has_alpha = (color_type & PNG_COLOR_MASK_ALPHA);
+
   // Transparency for paletted images.
-  if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS))
+  if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
     png_set_expand(png_ptr);
+    input_has_alpha = true;
+  }
 
   // Convert 16-bit to 8-bit.
   if (bit_depth == 16)
     png_set_strip_16(png_ptr);
 
+  // See comment about png_set_* call ordering, above.
+  if (state->output_format == PNGCodec::FORMAT_BGRA)
+    png_set_bgr(png_ptr);
+
+  switch (state->output_format) {
+    case PNGCodec::FORMAT_RGB:
+      state->output_channels = 3;
+      if (input_has_alpha)
+        png_set_strip_alpha(png_ptr);
+      break;
+    case PNGCodec::FORMAT_RGBA:
+    case PNGCodec::FORMAT_BGRA:
+    case PNGCodec::FORMAT_SkBitmap:
+      state->output_channels = 4;
+      if (!input_has_alpha)
+        png_set_add_alpha(png_ptr, 0xFF, PNG_FILLER_AFTER);
+      break;
+    default:
+      NOTREACHED() << "Unknown output format";
+      break;
+  }
+
   // Expand grayscale to RGB.
   if (color_type == PNG_COLOR_TYPE_GRAY ||
       color_type == PNG_COLOR_TYPE_GRAY_ALPHA)
@@ -275,64 +249,18 @@
     png_set_gamma(png_ptr, kDefaultGamma, kInverseGamma);
   }
 
+  // See comment about png_set_* call ordering, above.
+  if (state->output_format == PNGCodec::FORMAT_SkBitmap) {
+    png_set_read_user_transform_fn(png_ptr, ConvertRGBARowToSkia);
+    png_set_user_transform_info(png_ptr, state, 0, 0);
+  }
+
   // Tell libpng to send us rows for interlaced pngs.
   if (interlace_type == PNG_INTERLACE_ADAM7)
     png_set_interlace_handling(png_ptr);
 
-  // Update our info now
   png_read_update_info(png_ptr, info_ptr);
-  channels = png_get_channels(png_ptr, info_ptr);
 
-  // Pick our row format converter necessary for this data.
-  if (channels == 3) {
-    switch (state->output_format) {
-      case PNGCodec::FORMAT_RGB:
-        state->row_converter = NULL;  // no conversion necessary
-        state->output_channels = 3;
-        break;
-      case PNGCodec::FORMAT_RGBA:
-        state->row_converter = &ConvertRGBtoRGBA;
-        state->output_channels = 4;
-        break;
-      case PNGCodec::FORMAT_BGRA:
-        state->row_converter = &ConvertRGBtoBGRA;
-        state->output_channels = 4;
-        break;
-      case PNGCodec::FORMAT_SkBitmap:
-        state->row_converter = &ConvertRGBtoSkia;
-        state->output_channels = 4;
-        break;
-      default:
-        NOTREACHED() << "Unknown output format";
-        break;
-    }
-  } else if (channels == 4) {
-    switch (state->output_format) {
-      case PNGCodec::FORMAT_RGB:
-        state->row_converter = &ConvertRGBAtoRGB;
-        state->output_channels = 3;
-        break;
-      case PNGCodec::FORMAT_RGBA:
-        state->row_converter = NULL;  // no conversion necessary
-        state->output_channels = 4;
-        break;
-      case PNGCodec::FORMAT_BGRA:
-        state->row_converter = &ConvertBetweenBGRAandRGBA;
-        state->output_channels = 4;
-        break;
-      case PNGCodec::FORMAT_SkBitmap:
-        state->row_converter = &ConvertRGBAtoSkia;
-        state->output_channels = 4;
-        break;
-      default:
-        NOTREACHED() << "Unknown output format";
-        break;
-    }
-  } else {
-    NOTREACHED() << "Unknown input channels";
-    longjmp(png_jmpbuf(png_ptr), 1);
-  }
-
   if (state->bitmap) {
     state->bitmap->setConfig(SkBitmap::kARGB_8888_Config,
                              state->width, state->height);
@@ -345,11 +273,11 @@
 
 void DecodeRowCallback(png_struct* png_ptr, png_byte* new_row,
                        png_uint_32 row_num, int pass) {
+  if (!new_row) return;  // Interlaced image; row didn't change this pass.
+
   PngDecoderState* state = static_cast<PngDecoderState*>(
       png_get_progressive_ptr(png_ptr));
 
-  DCHECK(pass == 0) << "We didn't turn on interlace handling, but libpng is "
-                       "giving us interlaced data.";
   if (static_cast<int>(row_num) > state->height) {
     NOTREACHED() << "Invalid row";
     return;
@@ -360,12 +288,9 @@
     base = reinterpret_cast<unsigned char*>(state->bitmap->getAddr32(0, 0));
   else if (state->output)
     base = &state->output->front();
-
   unsigned char* dest = &base[state->width * state->output_channels * row_num];
-  if (state->row_converter)
-    state->row_converter(new_row, state->width, dest, &state->is_opaque);
-  else
-    memcpy(dest, new_row, state->width * state->output_channels);
+
+  png_progressive_combine_row(png_ptr, dest, new_row);

[Francois, 2012/02/28 15:00:36]

This call merges the latest changes to the rows of an interlaced PNG with their
prior changes. For non-interlaced images it simply does the memcpy as the old
code used to if the converter was NULL.

 }
 
 void DecodeEndCallback(png_struct* png_ptr, png_info* info) {
@@ -377,19 +302,29 @@
   state->done = true;
 }
 
-// Automatically destroys the given read structs on destruction to make
-// cleanup and error handling code cleaner.
-class PngReadStructDestroyer {
- public:
-  PngReadStructDestroyer(png_struct** ps, png_info** pi) : ps_(ps), pi_(pi) {
-  }
-  ~PngReadStructDestroyer() {
-    png_destroy_read_struct(ps_, pi_, NULL);
-  }
- private:
+// Automatically destroys the given libpng read or write structs on
+// destruction to make cleanup and error handling code cleaner.
+//
+// |IOMode|: 0 indicates read mode; 1 indicates write mode.
+template<int IOMode>

[Elliot Glaysher, 2012/02/28 18:37:43]

We don't use templates like this. Even if you have to copypasta the
ctor/members, it would be quicker for someone who didn't know the code to see
what you're doing if you did this normally instead of w/ partially specialized
destructors.

[Francois, 2012/02/29 09:12:29]

This has been removed from the current CL.

+struct ScopedLibPNGStructs {
+  ScopedLibPNGStructs(png_struct** ps, png_info** pi) : ps_(ps), pi_(pi) {}
+  ~ScopedLibPNGStructs() { NOTIMPLEMENTED(); }
   png_struct** ps_;
   png_info** pi_;
 };
+template<>
+ScopedLibPNGStructs<0>::~ScopedLibPNGStructs() {
+  png_destroy_read_struct(ps_, pi_, NULL);
+}
+template<>
+ScopedLibPNGStructs<1>::~ScopedLibPNGStructs() {
+  png_destroy_write_struct(ps_, pi_);
+}
+// Scoped LibPNG read structs.
+typedef ScopedLibPNGStructs<0> ScopedReadStructs;
+// Scoped LibPNG write structs.
+typedef ScopedLibPNGStructs<1> ScopedWriteStructs;
 
 bool BuildPNGStruct(const unsigned char* input, size_t input_size,
                     png_struct** png_ptr, png_info** info_ptr) {
@@ -413,6 +348,27 @@
   return true;
 }
 
+// Libpng user error and warning functions which allows us to print libpng
+// errors and warnings using Chrome's logging facilities instead of stderr.

[Elliot Glaysher, 2012/02/28 18:37:43]

Sweet!

+
+void LogLibPNGDecodeError(png_structp png_ptr, png_const_charp error_msg) {
+  DLOG(ERROR) << "libpng decode error: " << error_msg;
+  longjmp(png_jmpbuf(png_ptr), 1);
+}
+
+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
@@ -424,7 +380,7 @@
   if (!BuildPNGStruct(input, input_size, &png_ptr, &info_ptr))
     return false;
 
-  PngReadStructDestroyer destroyer(&png_ptr, &info_ptr);
+  ScopedReadStructs scoped_structs(&png_ptr, &info_ptr);
   if (setjmp(png_jmpbuf(png_ptr))) {
     // The destroyer will ensure that the structures are cleaned up in this
     // case, even though we may get here as a jump from random parts of the
@@ -436,6 +392,8 @@
 
   png_set_progressive_read_fn(png_ptr, &state, &DecodeInfoCallback,
                               &DecodeRowCallback, &DecodeEndCallback);
+  png_set_error_fn(png_ptr, NULL, LogLibPNGDecodeError, LogLibPNGDecodeWarning);
+
   png_process_data(png_ptr,
                    info_ptr,
                    const_cast<unsigned char*>(input),
@@ -462,7 +420,7 @@
   if (!BuildPNGStruct(input, input_size, &png_ptr, &info_ptr))
     return false;
 
-  PngReadStructDestroyer destroyer(&png_ptr, &info_ptr);
+  ScopedReadStructs scoped_structs(&png_ptr, &info_ptr);
   if (setjmp(png_jmpbuf(png_ptr))) {
     // The destroyer will ensure that the structures are cleaned up in this
     // case, even though we may get here as a jump from random parts of the
@@ -543,17 +501,6 @@
   // 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];
-  }

[Francois, 2012/02/28 15:00:36]

Replaced by a combination of png_set_filler (stips alpha during encode) and
png_set_bgr.

-}
-
 #ifdef PNG_TEXT_SUPPORTED
 class CommentWriter {
  public:
@@ -610,62 +557,6 @@
 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) {
-  // 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)))
-    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_IHDR(png_ptr, info_ptr, width, height, 8, png_output_color_type,
-               PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
-               PNG_FILTER_TYPE_DEFAULT);
-
-#ifdef PNG_TEXT_SUPPORTED
-  CommentWriter comment_writer(comments);
-  if (comment_writer.HasComments()) {
-    png_set_text(png_ptr, info_ptr, comment_writer.get_png_text(),
-                 comment_writer.size());
-  }
-#endif
-
-  png_write_info(png_ptr, info_ptr);
-
-  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]));
-    }
-  } else {
-    // Needs conversion using a separate buffer.
-    unsigned char* row = new unsigned char[width * output_color_components];
-    for (int y = 0; y < height; y ++) {
-      converter(&input[y * row_byte_width], width, row, NULL);
-      png_write_row(png_ptr, row);
-    }
-    delete[] row;
-  }
-
-  png_write_end(png_ptr, info_ptr);
-  return true;
-}

[Francois, 2012/02/28 15:00:36]

I merged this function back into EncodeWithCompressionLevel because 1) I needed
to set the user transform function in the midst of this function's body, and
adding yet another parameter and the logic to check it seemed sub-optimal; 2)
neither g++ (a newer version than on Ubuntu 10.04/Lucid, admittedly) nor clang
generated warnings due to this change; 3) setjmp was being called by Decode() in
the presence of local variables even before my changes were made; 4) all call
sites of setjmp look to be fine according to the NOTES section of the longjmp(3)
manpage.

-
 }  // namespace
 
 // static
@@ -693,49 +584,51 @@
   // conversion is necessary.
   FormatConverter converter = NULL;
 
-  int input_color_components, output_color_components;
+  png_struct* png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING,
+                                                NULL, NULL, NULL);
+  if (!png_ptr) return false;
+  png_info* info_ptr = png_create_info_struct(png_ptr);
+  if (!info_ptr) {
+    png_destroy_write_struct(&png_ptr, NULL);
+    return false;
+  }
+
+  ScopedWriteStructs scoped_structs(&png_ptr, &info_ptr);
+
+  if (setjmp(png_jmpbuf(png_ptr)))
+    return false;
+
+  PngEncoderState state(output);
+
+  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);
+
+  int input_color_components;

[Francois, 2012/02/28 15:00:36]

This block was merged from what used to be DoLibpngWrite() (and minor changes
made).

   int png_output_color_type;
   switch (format) {
     case FORMAT_RGB:
       input_color_components = 3;
-      output_color_components = 3;
       png_output_color_type = PNG_COLOR_TYPE_RGB;
       break;
 
     case 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 FORMAT_BGRA:
       input_color_components = 4;
-      if (discard_transparency) {
-        output_color_components = 3;
+      if (discard_transparency)
         png_output_color_type = PNG_COLOR_TYPE_RGB;
-        converter = ConvertBGRAtoRGB;
-      } else {
-        output_color_components = 4;
+      else
         png_output_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
-        converter = ConvertBetweenBGRAandRGBA;
-      }
       break;
 
     case FORMAT_SkBitmap:
       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;
       }
@@ -749,24 +642,51 @@
   // Row stride should be at least as long as the length of the data.
   DCHECK(input_color_components * size.width() <= row_byte_width);
 
-  png_struct* png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING,
-                                                NULL, NULL, NULL);
-  if (!png_ptr)
-    return false;
-  png_info* info_ptr = png_create_info_struct(png_ptr);
-  if (!info_ptr) {
-    png_destroy_write_struct(&png_ptr, NULL);
-    return false;
+  png_set_IHDR(png_ptr, info_ptr,
+               size.width(), size.height(),
+               8, png_output_color_type,
+               PNG_INTERLACE_NONE,
+               PNG_COMPRESSION_TYPE_DEFAULT,
+               PNG_FILTER_TYPE_DEFAULT);
+  png_write_info(png_ptr, info_ptr);
+
+  // Done outside of the switch above because the of the order of operations
+  // dictated by libpng.
+  if (format != FORMAT_SkBitmap) {
+    if (input_color_components == 4 && discard_transparency)
+      png_set_filler(png_ptr, 0, PNG_FILLER_AFTER);  // Strip alpha
+    if (format == FORMAT_BGRA)
+      png_set_bgr(png_ptr);
   }
 
-  PngEncoderState state(output);
-  bool success = DoLibpngWrite(png_ptr, info_ptr, &state,
-                               size.width(), size.height(), row_byte_width,
-                               input, compression_level, png_output_color_type,
-                               output_color_components, converter, comments);
-  png_destroy_write_struct(&png_ptr, &info_ptr);
+#ifdef PNG_TEXT_SUPPORTED
+  CommentWriter comment_writer(comments);
+  if (comment_writer.HasComments()) {
+    png_set_text(png_ptr, info_ptr, comment_writer.get_png_text(),
+                 comment_writer.size());
+  }
+#endif
 
-  return success;
+  if (!converter) {
+    // No conversion needed, give the data directly to libpng.
+    for (int y = 0; y < size.height(); y ++) {
+      png_write_row(png_ptr,
+                    const_cast<unsigned char*>(&input[y * row_byte_width]));
+    }
+  } else {
+    // Needs conversion using a separate buffer.
+    const int color_components =
+        (png_output_color_type & PNG_COLOR_MASK_ALPHA ? 4 : 3);
+    unsigned char* row = new unsigned char[size.width() * color_components];
+    for (int y = 0; y < size.height(); y ++) {
+      converter(&input[y * row_byte_width], size.width(), row, NULL);
+      png_write_row(png_ptr, row);
+    }
+    delete[] row;
+  }
+
+  png_write_end(png_ptr, info_ptr);
+  return true;

[Francois, 2012/02/28 15:00:36]

Large parts of this block and the two above it were merged from what used to be
DoLibpngWrite(), after which some changes were made.

 }
 
 // static