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/SegmentReader.h"
+
+#include "third_party/skia/src/core/SkRWBuffer.h"
+#include "wtf/Assertions.h"
+#include "wtf/Noncopyable.h"
+#include "wtf/PassRefPtr.h"
+#include "wtf/RefPtr.h"
+
+namespace blink {
+
+class ROBufferSegmentReader final : public SegmentReader {
+    WTF_MAKE_NONCOPYABLE(ROBufferSegmentReader);
+public:
+    ROBufferSegmentReader(PassRefPtr<SkROBuffer>);
+
+    size_t getSomeData(const char*& data, size_t position) const override;
+    size_t size() const override { return m_roBuffer ? m_roBuffer->size() : 0; }

[Peter Kasting, 2016/03/25 06:24:37]

Nit: Why define this inline (and nothing else)?

[scroggo, 2016/03/25 15:49:53]

Because it's short. What is your preference?

[Peter Kasting, 2016/03/25 21:15:56]

I generally avoid inlining virtuals (see "stop inlining virtuals" on
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ), but it
doesn't matter much here as this is in a .cpp file and not a .h file.  Do it
however you like.

[scroggo_chromium, 2016/03/25 21:31:59]

Good enough reason to me not to inline, for consistency. Done.

+    PassRefPtr<SkData> getAsSkData() const override;
+
+private:
+    RefPtr<SkROBuffer> m_roBuffer;
+    // Position of the first char in the current block of m_iter.
+    mutable size_t m_positionOfBlock;
+    mutable SkROBuffer::Iter m_iter;
+};
+
+PassRefPtr<SegmentReader> SegmentReader::createFromSkROBuffer(PassRefPtr<SkROBuffer> buffer)
+{
+    return adoptRef(new ROBufferSegmentReader(buffer));
+}
+
+ROBufferSegmentReader::ROBufferSegmentReader(PassRefPtr<SkROBuffer> buffer)
+    : m_roBuffer(buffer)
+    , m_positionOfBlock(0)
+    , m_iter(m_roBuffer.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) {

[Peter Kasting, 2016/03/25 06:24:37]

Nit: Could write this as

    for (size_t sizeOfBlock = m_iter.size(); sizeOfBlock != 0; m_positionOfBlock
+= sizeOfBlock) {
        ...
    }
    return 0;

This would not only be shorter (as it would eliminate several lines of code at
the beginning and end of the loop body), it would be less likely to confuse dumb
compilers (or dumb readers) as to why there's no return statement at the end of
a non-void function.

[scroggo, 2016/03/25 15:49:53]

Done.

+        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.
+            const size_t positionInBlock = position - m_positionOfBlock;
+            data = static_cast<const char*>(m_iter.data()) + positionInBlock;
+            return sizeOfBlock - positionInBlock;
+        }
+
+        // Move to next block.
+        if (!m_iter.next()) {
+            // Reset to the beginning, so future calls can succeed.
+            m_iter.reset(m_roBuffer.get());
+            m_positionOfBlock = 0;
+            return 0;
+        }
+
+        m_positionOfBlock += sizeOfBlock;
+    }
+}
+
+static void unrefRobuffer(const void* ptr, void* context)

[Peter Kasting, 2016/03/25 06:24:37]

Nit: ROBuffer?

[scroggo, 2016/03/25 15:49:53]

Done.

+{
+    static_cast<SkROBuffer*>(context)->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();
+    iter.reset(m_roBuffer.get());
+
+    if (multipleBlocks) {
+        SkData* data = SkData::NewUninitialized(m_roBuffer->size());
+        char* dst = static_cast<char*>(data->writable_data());
+        do {
+            size_t size = iter.size();
+            memcpy(dst, iter.data(), size);
+            dst += size;
+        } while (iter.next());
+        return adoptRef(data);
+    }
+
+    // Contiguous data. No need to copy.

[Peter Kasting, 2016/03/25 06:24:37]

Nit: It's a little weird to see:

    // Check to see if the data is already contiguous.
    <brief check>

    <whole bunch of other code>

    // Contiguous data. No need to copy.
    ...

It might read more clearly to reverse the conditional above so the "contiguous
data" case happens right after the check for it?  (This also results in
indenting fewer lines of code.)

[scroggo, 2016/03/25 15:49:53]

Done.

+    m_roBuffer->ref();
+    return adoptRef(SkData::NewWithProc(iter.data(), iter.size(), &unrefRobuffer, m_roBuffer.get()));
+}
+
+} // namespace blink