Added ImagePipe

Added ImagePipe

ImagePipe is a Mojo interface designed to provide synchronized transport of
graphics buffers residing on the GPU (though in principle it could be used for
the transport of any type media buffer that can be accessed via a mojo handle).
The semantics of this interface are designed mainly to support the creation of
accelerated rendering contexts (e.g. OpenGL contexts) whose output is consumed
by a different Mojo application than the one doing the rendering.

This interface can be used directly, but we also provide endpoint classes which
provide state tracking and state validation functionality, which should
generally be used unless there is a compeeling reason not to.

Change-Id: I389ec3519314da2623a65051ef9786282a28e3ac

BUG=
R=jeffbrown@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/c905841e49ec58a05806eaf5294c942f151a574f

[Forrest Reiling, 2016-01-19 21:26:14]

freiling@chromium.org changed reviewers:
+ cstout@chromium.org, jamesr@chromium.org, jeffbrown@google.com

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

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:20: class
ImagePipeProducerEndpoint : private ImagePipeEndpoint,
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.

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:25: bool is_checked
= true);
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.

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:35: bool
AcquireImage(uint32_t& id, bool blocking = false);
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]

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:20: class
ImagePipeProducerEndpoint : private ImagePipeEndpoint,
On 2016/02/09 21:07:20, jamesr wrote:
> 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.

Acknowledged.

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:25: bool is_checked
= true);
On 2016/02/09 21:07:20, jamesr wrote:
> 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.

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)

https://codereview.chromium.org/1595773002/diff/20001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:35: bool
AcquireImage(uint32_t& id, bool blocking = false);
On 2016/02/09 21:07:20, jamesr wrote:
> 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.

Acknowledged.

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

I haven't looked closely yet at the interface or the test logic, as I don't
really understand the interface, but have enough comments on code stuff that I
think it's worth sending out.  Do you want me to look at the API too?  Is there
anywhere to read about the design/goals/use cases other than this patch?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/BUILD.gn (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/BUILD.gn:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
2016 (and elsewhere)

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/BUILD.gn:41:
"$mojo_sdk_root/mojo/public/cpp/utility:utility",
just ".../cpp/utility"

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_apptest.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:16: using namespace
image_pipe;
this is banned by the style guide: 
http://google.github.io/styleguide/cppguide.html#Namespaces.  you can have using
statements for specific types you want to bring into this namespace

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:18: class
ImagePipeApplicationTest : public test::ApplicationTestBase {
what's this class for? it doesn't seem to do anything

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:27: class TestImagePipe :
private ImagePipeConsumerDelegate {
private inheritance is banned by the style guide (and generally not a good idea)

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:33:
ImagePipeProducerEndpoint* producer_endpoint;
if it's a class members are private

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:35:
mojo::gfx::SupportedImagePropertiesPtr supported_properties_;
this is a public member with private naming, double confusing

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:39:
std::function<void(mojo::gfx::ImagePtr image, uint32_t id)>
no public members

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:45: // Inherited from
ImagePipeConsumerDelegate //
odd trailing //

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:67: present_image_override =
[](uint32_t id) {};
seems silly - wouldn't it be cleaner to leave it default constructed (aka empty)
and just test if the function is empty before calling?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:102: delete
producer_endpoint;
std::unique_ptr<> instead?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.cc:52: /*
mojo::gfx::ImagePipe implementation */
prefer // style comments

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:29: bool
AcquireNextImage(uint32_t& id);
google c++ style is parameters are either pointers, values, const references, or
rvalue references. non-const lvalue refs are banned:
http://google.github.io/styleguide/cppguide.html#Reference_Arguments

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:54:
ImagePipeEndpoint state_tacker_;
"tracker"?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:88:
image_pool_ids_.erase(image_pool_ids_iter);
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?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:112: // private method to
ensure that produce/release logic is symmetric between
start all comments with uppercase letters and end with periods as if it was a
sentence

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:117: auto releaser_owned_ids
=
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

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:208: auto container =
image_pool_ids_;
did you mean to make a copy of the entire vector here? that's what this line
(and the ones below) do

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:235: }
newline before bracket, add a comment indicating what it's closing (i think it's
probably a namespace here)

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:23: void
ProducerFatalError(const char* message, uint32_t id);
why const char*? it's very rare in c++ code to really want to use c-style
strings

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:26: bool
AcquireNextImage(uint32_t& id);
ditto re non-const ref param

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:39: void
DisableFatalErrors();
make "for testing" part of the function name - you can't see this comment at
callsites. in other code we suffix "ForTesting" on functions that should only be
used in tests

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:50: static void
ImagePipeLogError(const char* entity,
why const char*?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:55: std::map<uint32_t,
mojo::gfx::ImagePipe::PresentImageCallback>
unordered_map?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:59: std::vector<uint32_t>
image_pool_ids_;
does the order of these matter? if it's just keeping track of a set then it
seems like std::set<> or std::unordered_set<> would be better

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:77: }
say what this bracket is closing (i.e. "}  // namespace image_pipe")

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:27: // ask the
endpoint for an available image to draw into
capitalize

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:30: bool
AcquireImage(uint32_t& id);
pointer for out-param

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:45: // returns true
iff the message pipe underlying the image pipe has enountered
we don't normally say "iff" even when we mean it

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:65:
ImagePipeEndpoint state_tacker_;
tacker -> tracker? or is this something else? i'm not sure what a "tacker" is

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:69: }
newline, comment

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/interfaces/image.mojom (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/interfaces/image.mojom:9: //This should probably be a
system wide list (maybe in exists already?) since
space after "//"

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/interfaces/image.mojom:13: BGRA_8888 = 1,
do we actually support both of these? can we just go with one color format for
now?

[Forrest Reiling, 2016-02-22 19:11:15]

On 2016/02/18 20:40:59, jamesr wrote:
> I haven't looked closely yet at the interface or the test logic, as I don't
> really understand the interface, but have enough comments on code stuff that I
> think it's worth sending out.  Do you want me to look at the API too?  Is
there
> anywhere to read about the design/goals/use cases other than this patch?
> 

So there is a design doc for Image Pipe
(https://docs.google.com/document/d/17AnUMAZ4yI7AU5nvcUuUz-dgN49Dv9jct6i77BFP4...)
which has comments from you on it, as well as notes from the meeting we had
about it (which you attended). Theres also some discussion on the tq-internal
code review I did for the interface design itself
(https://turquoise-internal-review.git.corp.google.com/#/c/1062/) which you were
invited to comment on 3 months ago, before I started implementing stuff around
the interface.

I would of course like your feedback on the interface, just please keep in mind
that changes to the interface at this stage (especially significant changes)
will require adapting not just the interface code, but also the state tracking
wrappers, DisplayManager, the GL on ImagePipe implementation, and the Vulkan
swapchain implementation, so please request changes sparingly?

[Forrest Reiling, 2016-02-25 00:35:15]

Most recent changes are in, PTAL

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/BUILD.gn (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/BUILD.gn:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/02/18 20:40:58, jamesr wrote:
> 2016 (and elsewhere)

Ok I think I got them all

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/BUILD.gn:41:
"$mojo_sdk_root/mojo/public/cpp/utility:utility",
On 2016/02/18 20:40:58, jamesr wrote:
> just ".../cpp/utility"

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_apptest.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:16: using namespace
image_pipe;
On 2016/02/18 20:40:58, jamesr wrote:
> this is banned by the style guide: 
> http://google.github.io/styleguide/cppguide.html#Namespaces.  you can have
using
> statements for specific types you want to bring into this namespace

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:18: class
ImagePipeApplicationTest : public test::ApplicationTestBase {
On 2016/02/18 20:40:58, jamesr wrote:
> what's this class for? it doesn't seem to do anything

Its used to do some kind of wicked crazy test macro magic in the TEST_F calls
below. I'm not going to pretend how it works, but it wont build without it and
all the other app tests have it, so unless you can tell me how to make app tests
work without it I'm gonna leave it there.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:27: class TestImagePipe :
private ImagePipeConsumerDelegate {
On 2016/02/18 20:40:58, jamesr wrote:
> private inheritance is banned by the style guide (and generally not a good
idea)

Acknowledged.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:33:
ImagePipeProducerEndpoint* producer_endpoint;
On 2016/02/18 20:40:58, jamesr wrote:
> if it's a class members are private

Acknowledged.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:35:
mojo::gfx::SupportedImagePropertiesPtr supported_properties_;
On 2016/02/18 20:40:58, jamesr wrote:
> this is a public member with private naming, double confusing

Acknowledged.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:39:
std::function<void(mojo::gfx::ImagePtr image, uint32_t id)>
On 2016/02/18 20:40:58, jamesr wrote:
> no public members

Acknowledged.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:45: // Inherited from
ImagePipeConsumerDelegate //
On 2016/02/18 20:40:58, jamesr wrote:
> odd trailing //

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:67: present_image_override =
[](uint32_t id) {};
On 2016/02/18 20:40:58, jamesr wrote:
> seems silly - wouldn't it be cleaner to leave it default constructed (aka
empty)
> and just test if the function is empty before calling?

I mean in general I prefer initializing the functions in the constructor so that
way you dont have to check them at every single call site. In this case theres
only one call site so sure, I guess that makes sense, but in a more complex case
I would definitely favor having the functions be initialized empty so if theres
an unchecked call in some untested edge case it wont just bring the whole
program down.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_apptest.cc:102: delete
producer_endpoint;
On 2016/02/18 20:40:58, jamesr wrote:
> std::unique_ptr<> instead?

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.cc:52: /*
mojo::gfx::ImagePipe implementation */
On 2016/02/18 20:40:58, jamesr wrote:
> prefer // style comments

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:29: bool
AcquireNextImage(uint32_t& id);
On 2016/02/18 20:40:58, jamesr wrote:
> google c++ style is parameters are either pointers, values, const references,
or
> rvalue references. non-const lvalue refs are banned:
> http://google.github.io/styleguide/cppguide.html#Reference_Arguments

Good to know. Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:54:
ImagePipeEndpoint state_tacker_;
On 2016/02/18 20:40:58, jamesr wrote:
> "tracker"?

Its amazing how many times I copy and pasted that without noticing lol

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.cc (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:88:
image_pool_ids_.erase(image_pool_ids_iter);
On 2016/02/18 20:40:59, jamesr wrote:
> 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?

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

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:112: // private method to
ensure that produce/release logic is symmetric between
On 2016/02/18 20:40:58, jamesr wrote:
> start all comments with uppercase letters and end with periods as if it was a
> sentence

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:117: auto releaser_owned_ids
=
On 2016/02/18 20:40:59, jamesr wrote:
> 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

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.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:208: auto container =
image_pool_ids_;
On 2016/02/18 20:40:58, jamesr wrote:
> did you mean to make a copy of the entire vector here? that's what this line
> (and the ones below) do

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

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.cc:235: }
On 2016/02/18 20:40:58, jamesr wrote:
> newline before bracket, add a comment indicating what it's closing (i think
it's
> probably a namespace here)

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:23: void
ProducerFatalError(const char* message, uint32_t id);
On 2016/02/18 20:40:59, jamesr wrote:
> why const char*? it's very rare in c++ code to really want to use c-style
> strings

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:26: bool
AcquireNextImage(uint32_t& id);
On 2016/02/18 20:40:59, jamesr wrote:
> ditto re non-const ref param

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:39: void
DisableFatalErrors();
On 2016/02/18 20:40:59, jamesr wrote:
> make "for testing" part of the function name - you can't see this comment at
> callsites. in other code we suffix "ForTesting" on functions that should only
be
> used in tests

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:50: static void
ImagePipeLogError(const char* entity,
On 2016/02/18 20:40:59, jamesr wrote:
> why const char*?

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:55: std::map<uint32_t,
mojo::gfx::ImagePipe::PresentImageCallback>
On 2016/02/18 20:40:59, jamesr wrote:
> unordered_map?

I mean theres not likely going to be more than a few elements in this map at a
time, so i doubt it will matter.. But sure, why not?

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:59: std::vector<uint32_t>
image_pool_ids_;
yeah i guess. The ones that are deques I think should probably stay deques since
the ordering is important on those in some cases, but I guess for this guy it
doesnt matter. For the consumer_owned_ids_ and producer_owned_ids_ its a little
trickier because the consumer/producer can release the ids it owns in any order,
though some orders will be more likely in practice. For the common use case
these containers will probably only have a few elements each, so hash table
based containers might actually perform worse than array based containers, but I
guess I agree with this on the grounds that unordered_set more accurately
represents the intended use of the container.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:77: }
On 2016/02/18 20:40:59, jamesr wrote:
> say what this bracket is closing (i.e. "}  // namespace image_pipe")

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:27: // ask the
endpoint for an available image to draw into
On 2016/02/18 20:40:59, jamesr wrote:
> capitalize

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:30: bool
AcquireImage(uint32_t& id);
On 2016/02/18 20:40:59, jamesr wrote:
> pointer for out-param

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:45: // returns true
iff the message pipe underlying the image pipe has enountered
On 2016/02/18 20:40:59, jamesr wrote:
> we don't normally say "iff" even when we mean it

Sure. For the record though I'm pretty sure if you're reading header comments
you probably know what iff means and can infer from context that its not a typo.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:65:
ImagePipeEndpoint state_tacker_;
On 2016/02/18 20:40:59, jamesr wrote:
> tacker -> tracker? or is this something else? i'm not sure what a "tacker" is

'tacker' is typo-copy-pasta cancer. Fixed

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:69: }
On 2016/02/18 20:40:59, jamesr wrote:
> newline, comment

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
File mojo/services/gfx/images/interfaces/image.mojom (right):

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/interfaces/image.mojom:9: //This should probably be a
system wide list (maybe in exists already?) since
On 2016/02/18 20:40:59, jamesr wrote:
> space after "//"

Done.

https://codereview.chromium.org/1595773002/diff/60001/mojo/services/gfx/image...
mojo/services/gfx/images/interfaces/image.mojom:13: BGRA_8888 = 1,
On 2016/02/18 20:40:59, jamesr wrote:
> do we actually support both of these? can we just go with one color format for
> now?

I think these come from differences in GL vs Vulkan texture allocation. I'll
drop the RGBA one and see what happens.

Also, as per the comment, is there a color format enum somewhere else I should
be using?

[jamesr, 2016-02-25 23:16:17]

I remember the meeting and the doc but I still don't think I understand what
ImagePipe in general is supposed to be used for well enough to give a
high-quality review.  It seems like Jeff and Craig have some strong ideas about
how it's supposed to be used, which is fine, but that means one of them will
have to do the review on that portion to verify that it actually does what they
want it to do.

If one of those folks does that then I'm happy to review that the implementation
(especially of the GL bindings) does what it should.

[jeffbrown, 2016-03-03 23:09:58]

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image.mojom:43: struct
SupportedImageProperties {
move to image_pipe.mojom

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image_pipe.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:13: //Image properties are
not supported by the consumer (this is not neccessarily
style nit: space after //

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:15:
NOT_PRESENTED_INVALID_PROPERTIES,
assign explicit values to each one

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:20: //ImagePipe is a
collection of semantics for streaming shared images between two
nit: ImagePipe is a mechanism for streaming...

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:23: //It operates
conceptually on a pool of image descriptors that the pipe knows
Conceptually, it operates on a poll of |Image| objects...

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:31: //are both errors and
will cause the connection to terminate
instead of terminate, say closed

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:32: AddImage(Image image,
uint32 id);
AddImage(uint32 id, Image image);

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:38: //Mark the image as
available for consumption by the consumer
period at the end of each sentence (here and elsewhere)

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:52: //The reply to this
method will only be sent when the values change compared
So what's interesting is that this is the Producer asking the Consumer what
kinds of images it wants.  I think perhaps that if the Consumer wants to start
receiving Images with a new size or properties, it should ask the Producer out
of band to do that.

eg. MGLResize / glResizeCHROMIUM

But if the Consumer is communicating that out of band, then the Producer
shouldn't have to ask for the information through this interface at all.  So
perhaps the only thing we need to push through SupportedImageProperties is the
format (not the size).

We could perhaps even get rid of this method altogether.

[jeffbrown, 2016-03-03 23:10:40]

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image.mojom:43: struct
SupportedImageProperties {
On 2016/03/03 23:09:58, jeffbrown wrote:
> move to image_pipe.mojom

On second thought, don't.

[jeffbrown, 2016-03-03 23:27:49]

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_consumer_delegate.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_delegate.h:12: // pure virtual
class for platform dependent handling of
nit: Capitalize start of sentence.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:15: class
ImagePipeConsumerEndpoint : private mojo::gfx::ImagePipe {
public

nit: add docs

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:19:
mojo::gfx::SupportedImagePropertiesPtr supported_properties,
remove this

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:26: // id into their
id argument and return true
dead? delete this comment

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:29: bool
AcquireNextImage(uint32_t* id);
nit: out_id

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:31: // Releases an
image back to the producer
nit: period at end of sentences

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:34: void
SetSupportedImageProperties(
delete this

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:41: void
DisableFatalErrorsForTesting();
Consider removing this ForTesting() stuff if you can in future patches.  (Don't
worry about it right now.)

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:28: bool
AcquireNextImage(uint32_t* id);
nit: out_id

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:30: bool
AcquireImage(uint32_t* id);
out_id

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image_pipe.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:43: PresentImage(uint32 id)
=> (uint32 id, PresentationStatus status);
remove the returned id, the caller should just bind the id into the callback on
its side

[Forrest Reiling, 2016-03-04 21:04:28]

PTAL

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_consumer_delegate.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_delegate.h:12: // pure virtual
class for platform dependent handling of
On 2016/03/03 23:27:49, jeffbrown wrote:
> nit: Capitalize start of sentence.

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:15: class
ImagePipeConsumerEndpoint : private mojo::gfx::ImagePipe {
On 2016/03/03 23:27:49, jeffbrown wrote:
> public
> 
> nit: add docs

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:19:
mojo::gfx::SupportedImagePropertiesPtr supported_properties,
On 2016/03/03 23:27:49, jeffbrown wrote:
> remove this

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:26: // id into their
id argument and return true
On 2016/03/03 23:27:49, jeffbrown wrote:
> dead? delete this comment

merged with below comment

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:29: bool
AcquireNextImage(uint32_t* id);
On 2016/03/03 23:27:49, jeffbrown wrote:
> nit: out_id

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:31: // Releases an
image back to the producer
On 2016/03/03 23:27:49, jeffbrown wrote:
> nit: period at end of sentences

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:34: void
SetSupportedImageProperties(
On 2016/03/03 23:27:49, jeffbrown wrote:
> delete this

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_consumer_endpoint.h:41: void
DisableFatalErrorsForTesting();
On 2016/03/03 23:27:49, jeffbrown wrote:
> Consider removing this ForTesting() stuff if you can in future patches. 
(Don't
> worry about it right now.)

Acknowledged.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_endpoint.h:28: bool
AcquireNextImage(uint32_t* id);
On 2016/03/03 23:27:49, jeffbrown wrote:
> nit: out_id

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/cpp/image_pipe_producer_endpoint.h:30: bool
AcquireImage(uint32_t* id);
On 2016/03/03 23:27:49, jeffbrown wrote:
> out_id

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image.mojom:43: struct
SupportedImageProperties {
On 2016/03/03 23:10:40, jeffbrown wrote:
> On 2016/03/03 23:09:58, jeffbrown wrote:
> > move to image_pipe.mojom
> 
> On second thought, don't.

Acknowledged.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image.mojom:43: struct
SupportedImageProperties {
On 2016/03/03 23:09:58, jeffbrown wrote:
> move to image_pipe.mojom

Acknowledged.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
File mojo/services/gfx/images/interfaces/image_pipe.mojom (right):

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:13: //Image properties are
not supported by the consumer (this is not neccessarily
On 2016/03/03 23:09:58, jeffbrown wrote:
> style nit: space after //

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:15:
NOT_PRESENTED_INVALID_PROPERTIES,
On 2016/03/03 23:09:58, jeffbrown wrote:
> assign explicit values to each one

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:20: //ImagePipe is a
collection of semantics for streaming shared images between two
On 2016/03/03 23:09:58, jeffbrown wrote:
> nit: ImagePipe is a mechanism for streaming...

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:23: //It operates
conceptually on a pool of image descriptors that the pipe knows
On 2016/03/03 23:09:58, jeffbrown wrote:
> Conceptually, it operates on a poll of |Image| objects...

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:31: //are both errors and
will cause the connection to terminate
On 2016/03/03 23:09:58, jeffbrown wrote:
> instead of terminate, say closed

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:32: AddImage(Image image,
uint32 id);
On 2016/03/03 23:09:58, jeffbrown wrote:
> AddImage(uint32 id, Image image);

I'm skipping this one because I don't think it has any effect on the objective
quality of the code and it would take significant work to implement. Also, from
a completely subjective perspective, I think putting the Image first makes more
sense anyway because the image is the thing you're adding. Either way, this
function only has two arguments and they have different types, so even if you do
get it backwards the compiler will yell at you about it.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:38: //Mark the image as
available for consumption by the consumer
On 2016/03/03 23:09:58, jeffbrown wrote:
> period at the end of each sentence (here and elsewhere)

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:43: PresentImage(uint32 id)
=> (uint32 id, PresentationStatus status);
On 2016/03/03 23:27:49, jeffbrown wrote:
> remove the returned id, the caller should just bind the id into the callback
on
> its side

Done.

https://codereview.chromium.org/1595773002/diff/100001/mojo/services/gfx/imag...
mojo/services/gfx/images/interfaces/image_pipe.mojom:52: //The reply to this
method will only be sent when the values change compared
On 2016/03/03 23:09:58, jeffbrown wrote:
> So what's interesting is that this is the Producer asking the Consumer what
> kinds of images it wants.  I think perhaps that if the Consumer wants to start
> receiving Images with a new size or properties, it should ask the Producer out
> of band to do that.
> 
> eg. MGLResize / glResizeCHROMIUM
> 
> But if the Consumer is communicating that out of band, then the Producer
> shouldn't have to ask for the information through this interface at all.  So
> perhaps the only thing we need to push through SupportedImageProperties is the
> format (not the size).
> 
> We could perhaps even get rid of this method altogether.

Removing as per our discussion

[jeffbrown, 2016-03-05 00:01:22]

lgtm

[Forrest Reiling, 2016-03-08 19:36:50]

Description was changed from

==========
Added ImagePipe

ImagePipe is a Mojo interface designed to provide synchronized transport of
graphics buffers residing on the GPU (though in principle it could be used for
the transport of any type media buffer that can be accessed via a mojo handle).
The semantics of this interface are designed mainly to support the creation of
accelerated rendering contexts (e.g. OpenGL contexts) whose output is consumed
by a different Mojo application than the one doing the rendering.

This interface can be used directly, but we also provide endpoint classes which
provide state tracking and state validation functionality, which should
generally be used unless there is a compeeling reason not to.

Change-Id: I389ec3519314da2623a65051ef9786282a28e3ac

BUG=
==========

to

==========
Added ImagePipe

ImagePipe is a Mojo interface designed to provide synchronized transport of
graphics buffers residing on the GPU (though in principle it could be used for
the transport of any type media buffer that can be accessed via a mojo handle).
The semantics of this interface are designed mainly to support the creation of
accelerated rendering contexts (e.g. OpenGL contexts) whose output is consumed
by a different Mojo application than the one doing the rendering.

This interface can be used directly, but we also provide endpoint classes which
provide state tracking and state validation functionality, which should
generally be used unless there is a compeeling reason not to.

Change-Id: I389ec3519314da2623a65051ef9786282a28e3ac

BUG=
R=jeffbrown@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/c905841e49ec58a05806eaf5294...
==========

[Forrest Reiling, 2016-03-08 19:36:51]

Committed patchset #8 (id:140001) manually as
c905841e49ec58a05806eaf5294c942f151a574f (presubmit successful).