Added ImagePipe

mojo/services/gfx/images/cpp/image_pipe_endpoint.cc

Index: mojo/services/gfx/images/cpp/image_pipe_endpoint.cc
diff --git a/mojo/services/gfx/images/cpp/image_pipe_endpoint.cc b/mojo/services/gfx/images/cpp/image_pipe_endpoint.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0ed27d65f8203a6de9dea1a1489e1f99017ef2fd
--- /dev/null
+++ b/mojo/services/gfx/images/cpp/image_pipe_endpoint.cc
@@ -0,0 +1,235 @@
+// Copyright 2015 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 "image_pipe_endpoint.h"
+
+namespace image_pipe {
+
+void ImagePipeEndpoint::ImagePipeLogError(const char* entity,
+                                          const char* message,
+                                          uint32_t id) {
+  MOJO_LOG(ERROR) << "ImagePipe " << entity << " Error on Image ID " << id
+                  << ": " << message;
+}
+
+void ImagePipeEndpoint::ProducerFatalError(const char* message, uint32_t id) {
+  if (is_checked_) {
+    ImagePipeLogError("Producer", message, id);
+    if (is_producer_) {
+      MOJO_CHECK(false);
+    } else {
+      fatal_error_handler_();
+    }
+  }
+}
+
+void ImagePipeEndpoint::ConsumerFatalError(const char* message, uint32_t id) {
+  ImagePipeLogError("Consumer", message, id);
+  if (is_producer_) {
+    fatal_error_handler_();
+  } else {
+    if (is_checked_) {
+      MOJO_CHECK(false);
+    } else {
+      fatal_error_handler_();
+    }
+  }
+}
+
+ImagePipeEndpoint::ImagePipeEndpoint(bool is_producer,
+                                     std::function<void()> fatal_error_handler)
+    : is_producer_(is_producer),
+      is_checked_(true),
+      fatal_error_handler_(fatal_error_handler) {}
+
+ImagePipeEndpoint::~ImagePipeEndpoint() {}
+
+void ImagePipeEndpoint::ProducerAdd(uint32_t id) {
+  if (IsInPool(id)) {
+    ProducerFatalError("Attempting to add an image that is already in the pool",
+                       id);
+  } else {
+    image_pool_ids_.push_back(id);
+    if (is_producer_) {
+      producer_acquirable_ids_.push_back(id);
+    } else {
+      producer_owned_ids_.push_back(id);
+    }
+  }
+}
+
+bool ImagePipeEndpoint::AcquireNextImage(uint32_t& id_out) {
+  auto acquirable_ids_ =
+      is_producer_ ? &producer_acquirable_ids_ : &consumer_acquirable_ids_;
+  auto owned_ids_ = is_producer_ ? &producer_owned_ids_ : &consumer_owned_ids_;
+
+  if (acquirable_ids_->empty()) {
+    return false;
+  }
+
+  int id = acquirable_ids_->front();
+  acquirable_ids_->pop_front();
+  owned_ids_->push_back(id);
+  id_out = id;
+  return true;
+}
+
+void ImagePipeEndpoint::ProducerRemove(uint32_t id) {
+  if (IsInPool(id)) {
+    if ((IsConsumerOwned(id) || IsConsumerAcquirable(id))) {
+      ProducerFatalError(
+          "Attempting to remove an image that has been presented "
+          "but has not been released by the consumer",
+          id);
+    } else {
+      auto image_pool_ids_iter =
+          std::find(image_pool_ids_.begin(), image_pool_ids_.end(), id);
+      image_pool_ids_.erase(image_pool_ids_iter);

[jamesr, 2016/02/18 20:40:59]

you should check that |image_pool_ids_iter| is not i_p_ids_.end() before calling
erase, right()? or is that enforced by something else?

[Forrest Reiling, 2016/02/25 00:35:14]

Thats enforced by IsInPool(id) in the outer if statement

+      MOJO_DCHECK(!is_checked_ || IsProducerOwned(id) ||
+                  IsProducerAcquirable(id));
+      if (IsProducerOwned(id)) {
+        MOJO_DCHECK(!is_checked_ || !IsProducerAcquirable(id));
+        auto producer_owned_ids_iter = std::find(producer_owned_ids_.begin(),
+                                                 producer_owned_ids_.end(), id);
+        producer_owned_ids_.erase(producer_owned_ids_iter);
+        MOJO_DCHECK(!is_checked_ || !IsProducerOwned(id));
+      } else {
+        MOJO_DCHECK(!is_checked_ || IsProducerAcquirable(id));
+        auto producer_acquirable_ids_iter =
+            std::find(producer_acquirable_ids_.begin(),
+                      producer_acquirable_ids_.end(), id);
+        producer_acquirable_ids_.erase(producer_acquirable_ids_iter);
+        MOJO_DCHECK(!is_checked_ || !IsProducerAcquirable(id));
+      }
+    }
+  } else {
+    ProducerFatalError(
+        "Attempting to remove an image that is not in the image pool", id);
+  }
+}
+
+// private method to ensure that produce/release logic is symmetric between

[jamesr, 2016/02/18 20:40:58]

start all comments with uppercase letters and end with periods as if it was a
sentence

[Forrest Reiling, 2016/02/25 00:35:14]

Done.

+// producer and consumer since they represent the same action from a state
+// tracking perspective
+void ImagePipeEndpoint::ReleaseInternal(uint32_t id,
+                                        bool released_by_producer) {
+  auto releaser_owned_ids =

[jamesr, 2016/02/18 20:40:59]

i can't figure out what type |releaser_owned_ids| is after the ternary and
reference decay happens. i think it'd be marginally clearer to have this be a
ref instead of a pointer since that's in the type, i.e.

auto& releaser_owned_ids = relea... ? producer_owned_ids_ : consumer_owned_ids_

but it's still pretty obscure (imo) and probably deserves typing the name out or
refactoring this

[Forrest Reiling, 2016/02/25 00:35:14]

I like the reference syntax, Im definitely on board with that.

I agree that this method is pretty unclear (since it is one of the only places
here that has shared code for producer and consumer state tracking. The obvious
alternative here is to not share the code for the two (and duplicate this logic
with more meaningful names in ProducerPresent and ConsumerRelease). That would
probably make the code easier to read and understand, but I though the
duplication of the logic was pretty nasty so I decided to go with this.
Basically with the other approaches youd be looking at lines 130-143 duplicated
with different variable names but identical logic in ProducerPresent and
ConsumerRelease. Honestly I still prefer this but I could probably be persuaded
if you think theres a compelling reason why this is worse.

For now I'm going to go with the reference syntax, and I'll add some comments to
try and clear stuff up. 

+      released_by_producer ? &producer_owned_ids_ : &consumer_owned_ids_;
+  auto releasee_owned_ids =
+      !released_by_producer ? &producer_owned_ids_ : &consumer_owned_ids_;
+
+  auto releasee_acquirable_ids = !released_by_producer
+                                     ? &producer_acquirable_ids_
+                                     : &consumer_acquirable_ids_;
+
+  auto IsReleaserOwned = released_by_producer
+                             ? &ImagePipeEndpoint::IsProducerOwned
+                             : &ImagePipeEndpoint::IsConsumerOwned;
+
+  MOJO_DCHECK(!is_checked_ || (this->*IsReleaserOwned)(id));
+  auto releaser_owned_ids_iter =
+      std::find(releaser_owned_ids->begin(), releaser_owned_ids->end(), id);
+  releaser_owned_ids->erase(releaser_owned_ids_iter);
+  MOJO_DCHECK(!is_checked_ || !(this->*IsReleaserOwned)(id));
+
+  // if the release action is coming from our side of the pipe, we wont see the
+  // acquire events, so we just pretend the other side immediately aquires
+  // everything to simplify state tracking
+  if (released_by_producer == is_producer_) {
+    releasee_owned_ids->push_back(id);
+  } else {
+    releasee_acquirable_ids->push_back(id);
+  }
+}
+
+void ImagePipeEndpoint::ProducerPresent(
+    uint32_t id,
+    mojo::gfx::ImagePipe::PresentImageCallback callback) {
+  if (IsProducerOwned(id)) {
+    MOJO_DCHECK(!is_checked_ || (IsInPool(id) && !IsConsumerOwned(id) &&
+                                 !IsConsumerAcquirable(id)));
+    ReleaseInternal(id, true);
+    present_callback_map_[id] = callback;
+  } else if (!IsInPool(id)) {
+    ProducerFatalError(
+        "Attempting to present an image that is not in the image pool", id);
+  } else if (IsProducerAcquirable(id)) {
+    ProducerFatalError(
+        "Attempting to present an image that has not been acquired", id);
+  } else if (IsConsumerOwned(id) || IsConsumerAcquirable(id)) {
+    ProducerFatalError(
+        "Attempting to present an image that has already been presented", id);
+  }
+}
+
+void ImagePipeEndpoint::ConsumerRelease(uint32_t id,
+                                        mojo::gfx::PresentationStatus status) {
+  if (IsConsumerOwned(id)) {
+    MOJO_DCHECK(!is_checked_ || (IsInPool(id) && !IsProducerOwned(id) &&
+                                 !IsProducerAcquirable(id)));
+    ReleaseInternal(id, false);
+    CallPresentCallback(id, status);
+  } else if (!IsInPool(id)) {
+    ConsumerFatalError(
+        "Attempting to release an image that is not in the image pool", id);
+  } else if (IsConsumerAcquirable(id)) {
+    ConsumerFatalError(
+        "Attempting to release an image that has not been acquired", id);
+  } else if (IsProducerOwned(id) || IsProducerAcquirable(id)) {
+    ConsumerFatalError(
+        "Attempting to release an image that has not been presented", id);
+  }
+}
+
+void ImagePipeEndpoint::ProducerFlush() {
+  if (!is_producer_) {
+    for (auto id : consumer_acquirable_ids_) {
+      MOJO_DCHECK(!is_checked_ || (IsInPool(id) && !IsConsumerOwned(id) &&
+                                   !IsProducerOwned(id)));
+      CallPresentCallback(id,
+                          mojo::gfx::PresentationStatus::NOT_PRESENTED_FLUSHED);
+    }
+    consumer_acquirable_ids_.clear();
+  }
+}
+
+void ImagePipeEndpoint::CallPresentCallback(
+    uint32_t id,
+    mojo::gfx::PresentationStatus status) {
+  auto present_callback_iter = present_callback_map_.find(id);
+  MOJO_DCHECK(present_callback_iter != present_callback_map_.end());
+  auto present_callback = present_callback_iter->second;
+  present_callback_map_.erase(present_callback_iter);
+  present_callback.Run(id, status);
+}
+
+bool ImagePipeEndpoint::IsInPool(uint32_t id) const {
+  auto container = image_pool_ids_;

[jamesr, 2016/02/18 20:40:58]

did you mean to make a copy of the entire vector here? that's what this line
(and the ones below) do

[Forrest Reiling, 2016/02/25 00:35:14]

No this is just me C++'ing myself in the foot. So does this need to be 'auto&
container = image_pool_ids_' then? 

+  return std::find(container.begin(), container.end(), id) != container.end();
+}
+
+bool ImagePipeEndpoint::IsConsumerOwned(uint32_t id) const {
+  auto container = consumer_owned_ids_;
+  return std::find(container.begin(), container.end(), id) != container.end();
+}
+
+bool ImagePipeEndpoint::IsConsumerAcquirable(uint32_t id) const {
+  auto container = consumer_acquirable_ids_;
+  return std::find(container.begin(), container.end(), id) != container.end();
+}
+
+bool ImagePipeEndpoint::IsProducerOwned(uint32_t id) const {
+  auto container = producer_owned_ids_;
+  return std::find(container.begin(), container.end(), id) != container.end();
+}
+
+bool ImagePipeEndpoint::IsProducerAcquirable(uint32_t id) const {
+  auto container = producer_acquirable_ids_;
+  return std::find(container.begin(), container.end(), id) != container.end();
+}
+
+void ImagePipeEndpoint::DisableFatalErrors() {
+  is_checked_ = false;
+}
+}

[jamesr, 2016/02/18 20:40:58]

newline before bracket, add a comment indicating what it's closing (i think it's
probably a namespace here)

[Forrest Reiling, 2016/02/25 00:35:14]

Done.