Eliminate copies of encoded image data

third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp

+// Copyright 2016 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.
+
+#include "platform/image-decoders/ROBufferSegmentReader.h"
+
+#include "wtf/Assertions.h"
+
+namespace blink {
+
+ROBufferSegmentReader::ROBufferSegmentReader(const skia::RefPtr<SkROBuffer>& buffer)
+    : m_roBuffer(buffer)
+    , m_positionOfBlock(0)
+    , m_iter(buffer.get())
+    {}
+
+size_t ROBufferSegmentReader::getSomeData(const char*& data, size_t position) const
+{
+    if (!m_roBuffer)
+        return 0;
+
+    if (position < m_positionOfBlock) {
+        // SkROBuffer::Iter only iterates forwards. Start from the
+        // beginning.
+        m_iter.reset(m_roBuffer.get());
+        m_positionOfBlock = 0;
+    }
+
+    while (true) {
+        ASSERT(m_positionOfBlock <= position);
+
+        const size_t sizeOfBlock = m_iter.size();
+        if (sizeOfBlock == 0) {
+            return 0;
+        }
+
+        if (m_positionOfBlock + sizeOfBlock > position) {
+            // position is in this block.

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

Nit: Either capitalize Position or wrap it in ||.

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

Done.

+            const size_t positionInBlock = position - m_positionOfBlock;
+            data = static_cast<const char*>(m_iter.data()) + positionInBlock;
+            return sizeOfBlock - positionInBlock;
+        }
+
+        m_positionOfBlock += sizeOfBlock;
+
+        // Move to next block.
+        if (!m_iter.next()) {

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

Hmm.  If this returns false, should we have incremented m_positionOfBlock yet? 
I think no?

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

Good catch - this is a bug! But the fix is slightly more than that - we need to
reset our state, so the next call can succeed. Will add a test.

[scroggo_chromium, 2016/03/24 22:16:28]

I was wrong. This was not a bug. m_roBuffer is const (will never get more data),
so moving m_positionOfBlock before calling iter.next() is okay - it means that
any valid calls will reset the iter because a valid position will be <
m_positionOfBlock. That said, the old implementation may have been too tricky
for its own good, since it wasn't obvious to you, and I had forgotten its
cleverness.

Either way, I added tests in the latest patch set.

+            return 0;
+        }
+    }
+}
+
+static void unrefRobuffer(const void* ptr, void* context)
+{
+    auto roBuffer = static_cast<const SkROBuffer*>(context);

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

Nit: Just inline into next statement

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

Done.

+    roBuffer->unref();
+}
+
+PassRefPtr<SkData> ROBufferSegmentReader::getAsSkData() const
+{
+    if (!m_roBuffer)
+        return nullptr;
+
+    // Check to see if the data is already contiguous.
+    SkROBuffer::Iter iter(m_roBuffer.get());
+    const bool multipleBlocks = iter.next();

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

Nit: Maybe there should be a hasNext() method (or similar)?

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

That seems reasonable. It would require landing something like
crrev.com/1822413002 in Skia.

But resetting the iterator is fairly cheap, so I don't think there's any reason
to hold this up.

+    iter.reset(m_roBuffer.get());
+
+    if (multipleBlocks) {
+        SkData* data = SkData::NewUninitialized(m_roBuffer->size());
+        void* dst = data->writable_data();

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

Nit: I think there would be less casting if this was a char* and you only cast
here.

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

Done.

+        do {
+            size_t size = iter.size();
+            memcpy(dst, iter.data(), size);
+            dst = static_cast<void*>(static_cast<char*>(dst) + size);
+        } while (iter.next());
+        return adoptRef(data);
+    }
+
+    // Contiguous data. No need to copy.
+    m_roBuffer->ref();
+    return adoptRef(SkData::NewWithProc(iter.data(), iter.size(), &unrefRobuffer,

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

Nit: Begin all lines of args at the same position

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

This is where the presubmit hooks complained (I tried doing that), but thanks to
Florin's comments below, I've made this a (cleaner) one line with no comments
needed.

+        // uref_robuffer will not modify this, besides the mutable field
+        // fRefCnt, but NewWithProc expects a void* (not const).
+        const_cast<SkROBuffer*>(m_roBuffer.get())));

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

SkROBuffer is intrinsically const (I think :), so it should be ok to relax
m_roBuffer's constness (as I suggested above).

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

I missed your comment above, but you're right! I've removed all this nonsense
about whether or not the SkROBuffer is const, making this a lot cleaner :)

+}
+
+} // namespace blink