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"
+
+namespace blink {
+
+SegmentStream::SegmentStream()
+    : reader_(),
+      position_(0),

[vmpstr, 2017/07/17 21:28:40]

nit: you can initialize these in the header and = default here. 

[cblume, 2017/07/18 10:23:48]

Done.

+      has_read_all_contents_(true),
+      is_cleared_(true) {}
+
+SegmentStream::SegmentStream(SegmentStream&& rhs)

[vmpstr, 2017/07/17 21:28:40]

nit: = default;

[cblume, 2017/07/18 10:23:48]

No can do on this one.

This class derives from SkStream, which derives from SkNoncopyable.
SkNoncopyable provides an explicitly defaulted move constructor.

However, SkStream does not provide any implicit copy / move constructors. So the
implicitly generated move constructor would rely on the deleted copy
constructor.

Implicitly defaulted move != explicitly defaulted move.
It ends up giving this error:
error: defaulting this move constructor would delete it after its first
declaration

If we want, we can solve this by adding an explicitly defaulted move constructor
in SkStream.
What I provided here is the alternative. Notice I'm not calling :
SkStream(std::move(rhs)), ...

[cblume, 2017/07/18 10:45:57]

*However, SkStream does not provide any *EXPLICIT* copy / cove constructors...

[vmpstr, 2017/07/18 18:02:48]

Acknowledged. I think there is no implicitly generated move ctor here at all due
to there being a dtor. The code you have is fine as is, but SkStream should
probably provide a defaulted move ctor since it's meant to be derived from with
a possibility of being moved (just for code health reasons). 

[cblume, 2017/07/19 23:27:04]

I made a Skia patch for it: https://skia-review.googlesource.com/c/24580/
Once that lands, I'll update this.
That's the much nicer, cleaner approach. :)

+    : reader_(rhs.reader_),
+      position_(rhs.position_),
+      has_read_all_contents_(rhs.has_read_all_contents_),
+      is_cleared_(rhs.is_cleared_) {}
+
+SegmentStream& SegmentStream::operator=(SegmentStream&& rhs) {
+  reader_ = rhs.reader_;

[vmpstr, 2017/07/17 21:28:40]

nit: reader_ = std::move(rhs.reader_);

[cblume, 2017/07/18 10:23:48]

Done.

+  position_ = rhs.position_;
+  has_read_all_contents_ = rhs.has_read_all_contents_;
+  is_cleared_ = rhs.is_cleared_;
+
+  return *this;
+}
+
+void SegmentStream::SetReader(WTF::PassRefPtr<SegmentReader> reader) {
+  reader_ = std::move(reader);
+  SetPositionState(position_);
+}
+
+size_t SegmentStream::read(void* buffer, size_t size) {
+  if (is_cleared_)
+    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);
+  }
+
+  SetPositionState(position_ + bytes_advanced);
+
+  return bytes_advanced;
+}
+
+size_t SegmentStream::peek(void* buffer, size_t size) const {
+  if (is_cleared_)
+    return 0;
+
+  size = std::min(size, reader_->size() - position_);
+
+  size_t peek_position = position_;

[vmpstr, 2017/07/17 21:28:40]

nit: I don't think you really need this, GetSomeData can just take
total_bytes_peeked + position_?

[cblume, 2017/07/18 10:23:48]

Done.

+  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, peek_position);
+    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;
+    peek_position += bytes_peeked;
+  }
+
+  return total_bytes_peeked;
+}
+
+bool SegmentStream::rewind() {
+  SetPositionState(0);
+  return true;
+}
+
+bool SegmentStream::seek(size_t position) {
+  SetPositionState(position);
+  return true;
+}
+
+bool SegmentStream::move(long offset) {
+  long absolute_position = position_ + offset;
+
+  absolute_position = std::max(absolute_position, 0l);

[vmpstr, 2017/07/17 21:28:40]

Should this just be a DCHECK absolute_position > 0? Do we have users of this
that might move to a negative position?

[cblume, 2017/07/18 10:23:48]

It has been a while. I'll change this to a DCHECK and see if it passes the
tests.

[scroggo_chromium, 2017/07/18 17:52:41]

SkGifCodec won't call move at all (it uses seek instead). 

[cblume, 2017/07/19 23:27:04]

Do you happen to know about the other decoders?
This class should end up being used by them all.

[scroggo_chromium, 2017/07/20 13:59:52]

Oh, oops, SkGifCodec does use it. nvm.

+  SetPositionState(absolute_position);
+  return true;
+}
+
+void SegmentStream::SetPositionState(size_t new_position) {
+  position_ = new_position;
+  if (reader_) {
+    const size_t reader_size = reader_->size();
+    has_read_all_contents_ = position_ >= reader_size;
+    is_cleared_ = position_ > reader_size;

[vmpstr, 2017/07/17 21:28:40]

nit: This seems like a small difference between has_read_all_contents_ and
is_cleared_. Can you comment why there is a off-by-one difference here? On a
related note, can we use has_read_all_contents_ instead of is_cleared_
throughout here or is this off by one important?

[cblume, 2017/07/18 10:23:48]

The off-by-one is indeed important.

Having read the last byte is covered by the >= for has_read_all_contents_.
If we get to the end of the buffer, that doesn't mean the buffer has been
cleared.

When we change the reader's size to be smaller (and thus the old position is now
past the end), the stream has been cleared.
We need to know when this happens so we can exit early when attempting to
decode.

When you said "Can you comment" did you mean here or adding a comment into the
code, since this is not perfectly clear?
FWIW, this is covered by tests.

[vmpstr, 2017/07/18 18:02:48]

Yeah I meant can you leave a comment somewhere in the code? Either here, or in
the header where these variables are declared just so it's obvious that this
distinction is important.

[cblume, 2017/07/19 23:27:04]

I'll put a comment in the header.

+  } else {
+    has_read_all_contents_ = true;
+    is_cleared_ = true;
+  }
+}
+
+}  // namespace blink