Added ImagePipe

mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h

+// 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.
+
+#ifndef MOJO_SERVICES_GFX_IMAGES_INTERFACES_IMAGE_PIPE_PRODUCER_ENDPOINT_H_
+#define MOJO_SERVICES_GFX_IMAGES_INTERFACES_IMAGE_PIPE_PRODUCER_ENDPOINT_H_
+
+#include "image_pipe_endpoint.h"
+#include "mojo/public/c/system/macros.h"
+#include "mojo/services/gfx/images/interfaces/image_pipe.mojom.h"
+
+namespace image_pipe {
+
+// This class wraps the producer end of an ImagePipe and validates the sanity
+// of both the producer actions and the consumer actions coming over the
+// message pipe. It also tracks the state of Images in the pipe's image pool
+// and provides conviennce mechanisms for accessing this state (like the ability
+// to get the ID of an available image without having to manually track the
+// lifecycle of these images.
+class ImagePipeProducerEndpoint : private ImagePipeEndpoint,

[jamesr, 2016/02/09 21:07:20]

i strongly recommend against using implementation inheritance to share
implementation details. first, private inheritance is banned by the style guide.
 second, having this shared code be part of the implementation chain of
ImagePipeProducerEndpoint makes things extremely inflexible especially since as
the shared code has to also work with the other endpoint. it's not possible to
configure or replace any bit of functionality in ImagePipeEndpoint since it is
part of the type itself.

for this, you should use composition. have ImagePipeProducerEndpoint contain an
ImagePipeEndpoint member and call into it when needed. this way you can choose
to make this object configurable down the road, expose it if needed, mutate the
implementation without changing this type's interface, etc etc. it's a bit more
typing up from but well worth it.

[Forrest Reiling, 2016/02/10 20:38:42]

Acknowledged.

+                                  public mojo::gfx::ImagePipe {
+ public:
+  ImagePipeProducerEndpoint(mojo::gfx::ImagePipePtr image_pipe,
+                            std::function<void()> endpoint_closed_callback,
+                            bool is_checked = true);

[jamesr, 2016/02/09 21:07:20]

default parameters are discouraged (to sometimes banned) by the style guide and
pure bool params are pretty hard to read at the callsite, i.e. what does 'false'
here mean (without reading the header):

 auto my_producer(pipe.Pass(), ..., false);

when is the not-checked version useful? if you want to make the failure behavior
generally configurable you could have a way for the user of this type to pass in
an error handler that they can then use to log or check or discard as needed.

[Forrest Reiling, 2016/02/10 20:38:42]

The checked parameter exists solely for testing purposes (i.e. disable state
validation on one end of the image pipe in order to test state validation on the
other end). I can just move this bool to a separate setter and comment it as
test-only. 

I dont really see the purpose of having more generally configurable failure
behavior, since the failures this parameter controls are failures to conform to
the image pipe protocol, which you should be able to avoid deterministically
(i.e. hitting one of these errors at runtime is an indication that you are using
image pipe incorrectly and are therefore fatal)  

+
+  ~ImagePipeProducerEndpoint() override;
+
+  // ask the endpoint for an available image to draw into
+  // returns false if no images are available, otherwise returns true and
+  // sets |id| to the acquired ID. If |blocking| is true, this function will
+  // block until an Image is available. If |blocking| is true, but there are no
+  // images in the pool or the underlying message pipe has errors then false
+  // will be returned
+  bool AcquireImage(uint32_t& id, bool blocking = false);

[jamesr, 2016/02/09 21:07:20]

bool param here is bad - it's really hard to tell at callsites what the
difference between AcquireImage(..., false) and AcquireImage(..., true) are. i
think this would be better as separate functions, especially as the return
values have to be interpreted pretty differently in the two cases. for the
blocking version you'll probably want to add a timeout as well.

[Forrest Reiling, 2016/02/10 20:38:42]

Acknowledged.

+
+  // Inherited from mojo::gfx::ImagePipe, see image_pipe.mojom for comments
+  void AddImage(mojo::gfx::ImagePtr image, uint32_t id) override;
+  void RemoveImage(uint32_t id) override;
+  void PresentImage(uint32_t id, const PresentImageCallback& callback) override;
+  void FlushImages() override;
+  void GetSupportedImageProperties(
+      const GetSupportedImagePropertiesCallback& callback) override;
+
+  void CloseEndpoint() override;
+
+ private:
+  // This exists wrap ImagePipeEndpoint::ConsumerRelease because for some reason
+  // GCC doesnt like us calling protected methods on our base class from inside
+  // a lambda that captures 'this', which breaks the fnl build. Clang handles it
+  // fine, but its unclear whos right here, so we trampoline it as a workaround
+  void ConsumerReleaseInternal(uint32_t id,
+                               mojo::gfx::PresentationStatus status);
+  mojo::gfx::ImagePipePtr image_pipe_ptr_;
+  std::function<void()> endpoint_closed_callback_;
+};
+}
+
+#endif  // MOJO_SERVICES_GFX_IMAGES_INTERFACES_IMAGE_PIPE_PRODUCER_ENDPOINT_H_