Eliminate copies of encoded image data

third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 20 matching lines @@
 #include "platform/graphics/PictureSnapshot.h"
 
 #include "platform/geometry/IntSize.h"
 #include "platform/graphics/ImageBuffer.h"
 #include "platform/graphics/LoggingCanvas.h"
 #include "platform/graphics/ProfilingCanvas.h"
 #include "platform/graphics/ReplayingCanvas.h"
 #include "platform/graphics/skia/ImagePixelLocker.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/image-decoders/ImageFrame.h"
+#include "platform/image-decoders/SkDataSegmentReader.h"

[scroggo_chromium, 2016/03/22 20:18:42]

I'm not sure the proper way to name this, since it is not a Skia class, but it
logically made sense to me to include the name of the Skia class in the name,
resulting in it starting with "Sk". I took the opposite route with
ROBufferSegmentReader, but they should be the same (or use some different naming
convention). Suggestions welcome.

[Peter Kasting, 2016/03/23 02:42:58]

I think DataSegmentReader is still readable enough, but I'm not categorically
opposed to using "Sk".

[f(malita), 2016/03/23 16:41:58]

I would normally use SkiaDataSegmentReader to clarify this is not a native Skia
("Sk") class, but in this case I also vote for DataSegmentReader.

[scroggo_chromium, 2016/03/24 13:59:45]

How did you choose between DataSegmentReader and SkiaDataSegmentReader? I'm
happy to use DataSegmentReader (done), but it would be helpful to know for the
future why I would choose one over the other.

I just had an idea - we could make them all hidden classes and just have an
interface with factories:

class SegmentReader {
public:
  // methods...

  // Leaving out smart pointer semantics for simplicity here...
  static SegmentReader* Create(SharedBuffer*);
  static SegmentReader* Create(SkROBuffer*);
  static SegmentReader* Create(SkData*);
};

(Maybe this is independent of the names, but if we go this route the names can
be isolated to a single .cpp file) A downside to this is that if/when we move
ImageDecoder into Skia, we will need to fix it so that it can still support
SharedBuffer. But that may not be a good enough reason not to do it now, if this
is cleaner.

[Peter Kasting, 2016/03/24 22:05:43]

It sounds like that would be a bit cleaner, yes; mostly in the form of reducing
the per-file boilerplate overhead of defining these in several files.  But not
exposing these different classes externally is also somewhat semantically nice
in that it keeps the callers seeing this as a black box; if we were to end up
collapsing the implementations of these into fewer individual classes
internally, no external users would need to change.

[scroggo_chromium, 2016/03/25 01:05:10]

I've left the names alone and switched to using factories. No other files
(besides build files) will need to worry about the naming convention, and if we
want to change the names they are isolated.

 #include "platform/image-encoders/skia/PNGImageEncoder.h"
+#include "skia/ext/refptr.h"
+#include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"
 #include "third_party/skia/include/core/SkStream.h"
 #include "wtf/HexNumber.h"
 #include "wtf/text/Base64.h"
 #include "wtf/text/TextEncoding.h"
 
 namespace blink {
 
 PictureSnapshot::PictureSnapshot(PassRefPtr<const SkPicture> picture)
     : m_picture(picture)
 {
 }
 
 static bool decodeBitmap(const void* data, size_t length, SkBitmap* result)
 {
-    RefPtr<SharedBuffer> buffer = SharedBuffer::create(static_cast<const char*>(data), length);
-    OwnPtr<ImageDecoder> imageDecoder = ImageDecoder::create(*buffer, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileIgnored);
+    // No need to copy the data; this decodes immediately.
+    skia::RefPtr<SkData> skdata = skia::AdoptRef(SkData::NewWithoutCopy(data, length));

[f(malita), 2016/03/23 16:41:58]

RefPtr<SkData> skData = adoptRef(...);

[scroggo_chromium, 2016/03/24 13:59:45]

Now that DataSegmentReader takes a PassRefPtr, I just inlined this.

+    SkDataSegmentReader segmentReader(skdata.get());

[Peter Kasting, 2016/03/23 02:42:58]

I think this needs to be refcounted if it's passed to setData() below.

This is where my suggestion in ImageDecoder.h that that function take a
non-raw-pointer would help to prevent errors.

[scroggo_chromium, 2016/03/24 13:59:45]

I think this is safe since the ImageDecoder will ref it and then be deleted and
unref it when it gets deleted (before the SkDataSegmentReader).

OTOH its more future-proof to change over.

Done.

+    OwnPtr<ImageDecoder> imageDecoder = ImageDecoder::create(segmentReader, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileIgnored);
     if (!imageDecoder)
         return false;
-    imageDecoder->setData(buffer.get(), true);
+    imageDecoder->setData(&segmentReader, true);
     ImageFrame* frame = imageDecoder->frameBufferAtIndex(0);
     if (!frame)
         return true;
     *result = frame->getSkBitmap();
     return true;
 }
 
 PassRefPtr<PictureSnapshot> PictureSnapshot::load(const Vector<RefPtr<TilePictureStream>>& tiles)
 {
     ASSERT(!tiles.isEmpty());
@@ skipping 89 matching lines @@
 
 PassRefPtr<JSONArray> PictureSnapshot::snapshotCommandLog() const
 {
     const SkIRect bounds = m_picture->cullRect().roundOut();
     LoggingCanvas canvas(bounds.width(), bounds.height());
     m_picture->playback(&canvas);
     return canvas.log();
 }
 
 } // namespace blink