gpu: Remove map_image field from gpu::Capabilities.

gpu: Remove map_image field from gpu::Capabilities.

As far as our current usage of this extension cares, it's
now supported by all command buffer implementations.

BUG=354833

[reveman, 2014-03-22 13:35:20]

https://codereview.chromium.org/205193006/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gpu_control_service.cc (right):

https://codereview.chromium.org/205193006/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gpu_control_service.cc:35:
DCHECK(gpu_memory_buffer_factory_);
Note: I change this to a DCHECK for consistency and removed the message as I
don't think it adds anything.

[danakj, 2014-03-24 16:42:07]

lgtm

https://codereview.chromium.org/205193006/diff/30001/mojo/gles2/gles2_context.cc
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/205193006/diff/30001/mojo/gles2/gles2_context...
mojo/gles2/gles2_context.cc:49: gpu::Capabilities()));
Can we get at the decoder caps here? Or transfer the TODO(piman) here?

[reveman, 2014-03-24 17:00:16]

https://codereview.chromium.org/205193006/diff/30001/mojo/gles2/gles2_context.cc
File mojo/gles2/gles2_context.cc (right):

https://codereview.chromium.org/205193006/diff/30001/mojo/gles2/gles2_context...
mojo/gles2/gles2_context.cc:49: gpu::Capabilities()));
On 2014/03/24 16:42:08, danakj wrote:
> Can we get at the decoder caps here? Or transfer the TODO(piman) here?

transferred the TODO

[piman, 2014-03-24 19:14:26]

So, map_image isn't (currently) supported by mojo or pepper. Should we really do
this?

[reveman, 2014-03-24 19:55:30]

On 2014/03/24 19:14:26, piman wrote:
> So, map_image isn't (currently) supported by mojo or pepper. Should we really
do
> this?

"Supported" doesn't mean much in this case. I guess it currently means we have a
framework in place to produce some type of buffer that can be used with MapImage
API. We can't make the decision whether it should be used or not based on it as
that will have to be a high level decision that depends on platform, drivers,
phase of the moon...

The capabilities field is confusing as it makes it look like we can decide if
0-copy should be used based on it. I'd rather say that this is always supported
and one valid implementation is to always fail when trying to allocate a gpu
memory buffer. Makes it less confusing and removes some unnecessary code.

[piman, 2014-03-24 20:09:42]

On Mon, Mar 24, 2014 at 12:55 PM, <reveman@chromium.org> wrote:

> On 2014/03/24 19:14:26, piman wrote:
>
>> So, map_image isn't (currently) supported by mojo or pepper. Should we
>> really
>>
> do
>
>> this?
>>
>
> "Supported" doesn't mean much in this case. I guess it currently means we
> have a
> framework in place to produce some type of buffer that can be used with
> MapImage
> API. We can't make the decision whether it should be used or not based on
> it as
> that will have to be a high level decision that depends on platform,
> drivers,
> phase of the moon...
>
> The capabilities field is confusing as it makes it look like we can decide
> if
> 0-copy should be used based on it. I'd rather say that this is always
> supported
> and one valid implementation is to always fail when trying to allocate a
> gpu
> memory buffer. Makes it less confusing and removes some unnecessary code.
>

The idea of the Capabilities struct was to shortcut the testing for
extensions: avoid round trips and string parsing. Each one of the fields
refers directly to an optional extension. I agree that the struct may be
misnamed (though we may want to extend it to have other things that would
be nice to avoid round trips for, such as maximum texture size).

In the case of map_image, the extension is effectively not (currently)
present on pepper and mojo, because we haven't yet wired the proper IPCs.
So I feel like pretending the extension is there anyway but always fails is
counter-productive.


> https://codereview.chromium.org/205193006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-03-24 21:37:03]

On 2014/03/24 20:09:42, piman wrote:
> On Mon, Mar 24, 2014 at 12:55 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2014/03/24 19:14:26, piman wrote:
> >
> >> So, map_image isn't (currently) supported by mojo or pepper. Should we
> >> really
> >>
> > do
> >
> >> this?
> >>
> >
> > "Supported" doesn't mean much in this case. I guess it currently means we
> > have a
> > framework in place to produce some type of buffer that can be used with
> > MapImage
> > API. We can't make the decision whether it should be used or not based on
> > it as
> > that will have to be a high level decision that depends on platform,
> > drivers,
> > phase of the moon...
> >
> > The capabilities field is confusing as it makes it look like we can decide
> > if
> > 0-copy should be used based on it. I'd rather say that this is always
> > supported
> > and one valid implementation is to always fail when trying to allocate a
> > gpu
> > memory buffer. Makes it less confusing and removes some unnecessary code.
> >
> 
> The idea of the Capabilities struct was to shortcut the testing for
> extensions: avoid round trips and string parsing. Each one of the fields
> refers directly to an optional extension. I agree that the struct may be
> misnamed (though we may want to extend it to have other things that would
> be nice to avoid round trips for, such as maximum texture size).
> 
> In the case of map_image, the extension is effectively not (currently)
> present on pepper and mojo, because we haven't yet wired the proper IPCs.
> So I feel like pretending the extension is there anyway but always fails is
> counter-productive.

Requiring everything that is not implemented by all existing gpu::GpuControl
implementations to be a run-time optional feature is overkill IMO. I'll wire up
pepper if necessary but I think we'd save ourselves some trouble by not
requiring that for everything we add to the gpu::GpuControl interface.

[piman, 2014-03-24 21:44:04]

On Mon, Mar 24, 2014 at 2:37 PM, <reveman@chromium.org> wrote:

> On 2014/03/24 20:09:42, piman wrote:
>
>  On Mon, Mar 24, 2014 at 12:55 PM, <mailto:reveman@chromium.org> wrote:
>>
>
>  > On 2014/03/24 19:14:26, piman wrote:
>> >
>> >> So, map_image isn't (currently) supported by mojo or pepper. Should we
>> >> really
>> >>
>> > do
>> >
>> >> this?
>> >>
>> >
>> > "Supported" doesn't mean much in this case. I guess it currently means
>> we
>> > have a
>> > framework in place to produce some type of buffer that can be used with
>> > MapImage
>> > API. We can't make the decision whether it should be used or not based
>> on
>> > it as
>> > that will have to be a high level decision that depends on platform,
>> > drivers,
>> > phase of the moon...
>> >
>> > The capabilities field is confusing as it makes it look like we can
>> decide
>> > if
>> > 0-copy should be used based on it. I'd rather say that this is always
>> > supported
>> > and one valid implementation is to always fail when trying to allocate a
>> > gpu
>> > memory buffer. Makes it less confusing and removes some unnecessary
>> code.
>> >
>>
>
>  The idea of the Capabilities struct was to shortcut the testing for
>> extensions: avoid round trips and string parsing. Each one of the fields
>> refers directly to an optional extension. I agree that the struct may be
>> misnamed (though we may want to extend it to have other things that would
>> be nice to avoid round trips for, such as maximum texture size).
>>
>
>  In the case of map_image, the extension is effectively not (currently)
>> present on pepper and mojo, because we haven't yet wired the proper IPCs.
>> So I feel like pretending the extension is there anyway but always fails
>> is
>> counter-productive.
>>
>
> Requiring everything that is not implemented by all existing
> gpu::GpuControl
> implementations to be a run-time optional feature is overkill IMO. I'll
> wire up
> pepper if necessary but I think we'd save ourselves some trouble by not
> requiring that for everything we add to the gpu::GpuControl interface.
>

We can make it compile-time by breaking GpuControl into smaller interfaces.


>
> https://codereview.chromium.org/205193006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-03-24 23:31:38]

On 2014/03/24 21:44:04, piman wrote:
> On Mon, Mar 24, 2014 at 2:37 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2014/03/24 20:09:42, piman wrote:
> >
> >  On Mon, Mar 24, 2014 at 12:55 PM, <mailto:reveman@chromium.org> wrote:
> >>
> >
> >  > On 2014/03/24 19:14:26, piman wrote:
> >> >
> >> >> So, map_image isn't (currently) supported by mojo or pepper. Should we
> >> >> really
> >> >>
> >> > do
> >> >
> >> >> this?
> >> >>
> >> >
> >> > "Supported" doesn't mean much in this case. I guess it currently means
> >> we
> >> > have a
> >> > framework in place to produce some type of buffer that can be used with
> >> > MapImage
> >> > API. We can't make the decision whether it should be used or not based
> >> on
> >> > it as
> >> > that will have to be a high level decision that depends on platform,
> >> > drivers,
> >> > phase of the moon...
> >> >
> >> > The capabilities field is confusing as it makes it look like we can
> >> decide
> >> > if
> >> > 0-copy should be used based on it. I'd rather say that this is always
> >> > supported
> >> > and one valid implementation is to always fail when trying to allocate a
> >> > gpu
> >> > memory buffer. Makes it less confusing and removes some unnecessary
> >> code.
> >> >
> >>
> >
> >  The idea of the Capabilities struct was to shortcut the testing for
> >> extensions: avoid round trips and string parsing. Each one of the fields
> >> refers directly to an optional extension. I agree that the struct may be
> >> misnamed (though we may want to extend it to have other things that would
> >> be nice to avoid round trips for, such as maximum texture size).
> >>
> >
> >  In the case of map_image, the extension is effectively not (currently)
> >> present on pepper and mojo, because we haven't yet wired the proper IPCs.
> >> So I feel like pretending the extension is there anyway but always fails
> >> is
> >> counter-productive.
> >>
> >
> > Requiring everything that is not implemented by all existing
> > gpu::GpuControl
> > implementations to be a run-time optional feature is overkill IMO. I'll
> > wire up
> > pepper if necessary but I think we'd save ourselves some trouble by not
> > requiring that for everything we add to the gpu::GpuControl interface.
> >
> 
> We can make it compile-time by breaking GpuControl into smaller interfaces.

Not sure I understand how that helps. Can you explain a bit more what you have
in mind?

[piman, 2014-03-24 23:38:15]

On Mon, Mar 24, 2014 at 4:31 PM, <reveman@chromium.org> wrote:

> On 2014/03/24 21:44:04, piman wrote:
>
>  On Mon, Mar 24, 2014 at 2:37 PM, <mailto:reveman@chromium.org> wrote:
>>
>
>  > On 2014/03/24 20:09:42, piman wrote:
>> >
>> >  On Mon, Mar 24, 2014 at 12:55 PM, <mailto:reveman@chromium.org> wrote:
>> >>
>> >
>> >  > On 2014/03/24 19:14:26, piman wrote:
>> >> >
>> >> >> So, map_image isn't (currently) supported by mojo or pepper. Should
>> we
>> >> >> really
>> >> >>
>> >> > do
>> >> >
>> >> >> this?
>> >> >>
>> >> >
>> >> > "Supported" doesn't mean much in this case. I guess it currently
>> means
>> >> we
>> >> > have a
>> >> > framework in place to produce some type of buffer that can be used
>> with
>> >> > MapImage
>> >> > API. We can't make the decision whether it should be used or not
>> based
>> >> on
>> >> > it as
>> >> > that will have to be a high level decision that depends on platform,
>> >> > drivers,
>> >> > phase of the moon...
>> >> >
>> >> > The capabilities field is confusing as it makes it look like we can
>> >> decide
>> >> > if
>> >> > 0-copy should be used based on it. I'd rather say that this is always
>> >> > supported
>> >> > and one valid implementation is to always fail when trying to
>> allocate a
>> >> > gpu
>> >> > memory buffer. Makes it less confusing and removes some unnecessary
>> >> code.
>> >> >
>> >>
>> >
>> >  The idea of the Capabilities struct was to shortcut the testing for
>> >> extensions: avoid round trips and string parsing. Each one of the
>> fields
>> >> refers directly to an optional extension. I agree that the struct may
>> be
>> >> misnamed (though we may want to extend it to have other things that
>> would
>> >> be nice to avoid round trips for, such as maximum texture size).
>> >>
>> >
>> >  In the case of map_image, the extension is effectively not (currently)
>> >> present on pepper and mojo, because we haven't yet wired the proper
>> IPCs.
>> >> So I feel like pretending the extension is there anyway but always
>> fails
>> >> is
>> >> counter-productive.
>> >>
>> >
>> > Requiring everything that is not implemented by all existing
>> > gpu::GpuControl
>> > implementations to be a run-time optional feature is overkill IMO. I'll
>> > wire up
>> > pepper if necessary but I think we'd save ourselves some trouble by not
>> > requiring that for everything we add to the gpu::GpuControl interface.
>> >
>>
>
>  We can make it compile-time by breaking GpuControl into smaller
>> interfaces.
>>
>
> Not sure I understand how that helps. Can you explain a bit more what you
> have
> in mind?
>

Separate out CreateGpuMemoryBuffer/DestroyGpuMemoryBuffer out of
GpuControl, and make a new interface (GpuMemoryBufferInterface). Have the
places that implement the extension (in-process, CommandBufferProxyImpl)
also implement that interface. Have the places that don't implement the
extension (pepper, mojo) not implement the interface.
Then you can pass that interface explicitly to places that need it (cc?).
At compile time you know whether you're hooking things that will work vs
not.


> https://codereview.chromium.org/205193006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.