Move gif image decoder to SkCodec

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

+// Copyright (c) 2017 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/SegmentStream.h"
+#include <utility>
+#include "platform/image-decoders/SegmentReader.h"
+
+namespace blink {
+
+SegmentStream::SegmentStream() = default;
+
+SegmentStream::SegmentStream(SegmentStream&& rhs)
+    : reader_(std::move(rhs.reader_)), position_(rhs.position_) {}

[scroggo_chromium, 2017/08/15 19:57:31]

Can this just be = default?

[cblume, 2017/08/16 07:49:16]

Yes. The C++11 standard [1] says in §12.8/15
"The implicitly-defined copy/move constructor for a non-union class X performs a
memberwise copy/move of its bases and members."

(Note, there is a difference between implicitly-declared and implicitly-defined.
But in our case, this is implicitly-defined.)


While we're at it, right now the fact that this class is non-copyable is rather
hidden. I would like to explicitly delete the copy ctor/assign. I think it helps
readability.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3485.pdf

+
+SegmentStream::~SegmentStream() = default;
+
+SegmentStream& SegmentStream::operator=(SegmentStream&& rhs) {

[scroggo_chromium, 2017/08/15 19:57:31]

Same question here.

+  reader_ = std::move(rhs.reader_);
+  position_ = rhs.position_;
+
+  return *this;
+}
+
+void SegmentStream::SetReader(WTF::RefPtr<SegmentReader> reader) {
+  reader_ = std::move(reader);
+}
+
+bool SegmentStream::IsCleared() const {
+  return !reader_ || position_ > reader_->size();
+}
+
+size_t SegmentStream::read(void* buffer, size_t size) {
+  if (IsCleared())

[scroggo_chromium, 2017/08/15 19:57:31]

Oh yeah, I forgot that you used |is_cleared_| internally. So you've traded
computing is_cleared_ once at the bottom of this method for computing the same
twice (once here, and once inside peek()).

That should be fine.

I was wondering if we could DCHECK(!IsCleared()), since we shouldn't be calling
these methods when cleared, but I suppose you didn't because the API allows a
client to call even when cleared?

[cblume, 2017/08/16 07:49:16]

Hrmmm we can.
The GIF decoder has a bunch of checks for IsCleared() which end up preventing
the calls to read/peek.
I'll do this & get rid of the tests which covered this (since they would now
trigger the DCHECK).

[scroggo_chromium, 2017/08/16 13:46:40]

Other than this I think you've addressed my concerns.

[cblume, 2017/08/16 17:02:02]

Oops. Sorry. Not sure why that didn't get uploaded.
It should be up now.



BTW, the reason I wasn't using =default before is because it requires this patch
which hasn't landed yet:
https://skia-review.googlesource.com/c/24580

+    return 0;
+
+  size = std::min(size, reader_->size() - position_);
+
+  size_t bytes_advanced = 0;
+  if (!buffer) {  // skipping, not reading
+    bytes_advanced = size;
+  } else {
+    bytes_advanced = peek(buffer, size);
+  }
+
+  position_ += bytes_advanced;
+
+  return bytes_advanced;
+}
+
+size_t SegmentStream::peek(void* buffer, size_t size) const {
+  if (IsCleared())
+    return 0;
+
+  size = std::min(size, reader_->size() - position_);
+
+  size_t total_bytes_peeked = 0;
+  char* buffer_as_char_ptr = reinterpret_cast<char*>(buffer);
+  while (size) {
+    const char* segment = nullptr;
+    size_t bytes_peeked =
+        reader_->GetSomeData(segment, position_ + total_bytes_peeked);
+    if (!bytes_peeked)
+      break;
+    if (bytes_peeked > size)
+      bytes_peeked = size;
+
+    memcpy(buffer_as_char_ptr, segment, bytes_peeked);
+    buffer_as_char_ptr += bytes_peeked;
+    size -= bytes_peeked;
+    total_bytes_peeked += bytes_peeked;
+  }
+
+  return total_bytes_peeked;
+}
+
+bool SegmentStream::isAtEnd() const {
+  return !reader_ || position_ >= reader_->size();
+}
+
+bool SegmentStream::rewind() {
+  position_ = 0;
+  return true;
+}
+
+bool SegmentStream::seek(size_t position) {
+  position_ = position;
+  return true;
+}
+
+bool SegmentStream::move(long offset) {
+  DCHECK_GT(offset, 0);
+  position_ += offset;
+  return true;
+}
+
+size_t SegmentStream::getLength() const {
+  if (reader_)
+    return reader_->size();
+
+  return 0;
+}
+
+}  // namespace blink