Removed config management from Graphics3D API. It will be better handled in the EGL helper library.

Removed config management from Graphics3D API. It will be better handled in the EGL helper library.
Also removed redundant enums from the API.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97019

[alokp, 2011-08-04 19:02:51]

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:110: // -
PP_GRAPHICS3DATTRIB_MAX_SURFACE_HEIGHT
Querying for the maximum surface size creates a chicken-and-egg problem: You
need to create a context to query the max size, but to create one, you need to
specify a size. I think this can be implemented in the EGL layer as well and
hence can be removed from Graphics3d API. What do you think?

[alokp, 2011-08-15 19:37:29]

piman: ping!

[piman, 2011-08-15 19:44:02]

LGTM

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:110: // -
PP_GRAPHICS3DATTRIB_MAX_SURFACE_HEIGHT
On 2011/08/04 19:02:51, Alok Priyadarshi wrote:
> Querying for the maximum surface size creates a chicken-and-egg problem: You
> need to create a context to query the max size, but to create one, you need to
> specify a size. I think this can be implemented in the EGL layer as well and
> hence can be removed from Graphics3d API. What do you think?

I don't disagree with the chicken-and-egg problem. You'll still need somehow a
way to query the information. I'm fine if it's in a separate interface though.

[alokp, 2011-08-15 20:59:31]

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:110: // -
PP_GRAPHICS3DATTRIB_MAX_SURFACE_HEIGHT
On 2011/08/15 19:44:02, piman wrote:
> On 2011/08/04 19:02:51, Alok Priyadarshi wrote:
> > Querying for the maximum surface size creates a chicken-and-egg problem: You
> > need to create a context to query the max size, but to create one, you need
to
> > specify a size. I think this can be implemented in the EGL layer as well and
> > hence can be removed from Graphics3d API. What do you think?
> 
> I don't disagree with the chicken-and-egg problem. You'll still need somehow a
> way to query the information. I'm fine if it's in a separate interface though.

I can leave it here, but max-surface-size is the minimum of max-render-buffer
and max-texture-size, which can be queried in the EGL implementation.

[piman, 2011-08-15 21:17:51]

On Mon, Aug 15, 2011 at 1:59 PM, <alokp@chromium.org> wrote:

>
> http://codereview.chromium.**org/7576012/diff/1/ppapi/c/**
>
dev/ppb_graphics_3d_dev.h<http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h>
> File ppapi/c/dev/ppb_graphics_3d_**dev.h (right):
>
> http://codereview.chromium.**org/7576012/diff/1/ppapi/c/**
>
dev/ppb_graphics_3d_dev.h#**newcode110<http://codereview.chromium.org/7576012/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h#newcode110>
> ppapi/c/dev/ppb_graphics_3d_**dev.h:110: // -
> PP_GRAPHICS3DATTRIB_MAX_**SURFACE_HEIGHT
> On 2011/08/15 19:44:02, piman wrote:
>
>> On 2011/08/04 19:02:51, Alok Priyadarshi wrote:
>> > Querying for the maximum surface size creates a chicken-and-egg
>>
> problem: You
>
>> > need to create a context to query the max size, but to create one,
>>
> you need to
>
>> > specify a size. I think this can be implemented in the EGL layer as
>>
> well and
>
>> > hence can be removed from Graphics3d API. What do you think?
>>
>
>  I don't disagree with the chicken-and-egg problem. You'll still need
>>
> somehow a
>
>> way to query the information. I'm fine if it's in a separate interface
>>
> though.
>
> I can leave it here, but max-surface-size is the minimum of
> max-render-buffer and max-texture-size, which can be queried in the EGL
> implementation.


You're talking about the EGL implementation as if it was standalone... EGL
has to be written on top of pepper APIs, so you'll still need some sort of
API to query max-render-buffer and max-texture-size.

Antoine


>
>
>
http://codereview.chromium.**org/7576012/<http://codereview.chromium.org/7576...
>

[alokp, 2011-08-15 21:24:25]

> >
> > I can leave it here, but max-surface-size is the minimum of
> > max-render-buffer and max-texture-size, which can be queried in the EGL
> > implementation.
> 
> 
> You're talking about the EGL implementation as if it was standalone... EGL
> has to be written on top of pepper APIs, so you'll still need some sort of
> API to query max-render-buffer and max-texture-size.
> 

I was thinking of creating an off-screen 1x1 graphics-3d instance when EGL is
initialized. Once you have an OpenGL context, you could query max-render-buffer
and max-texture-size. I would need to create a graphics-3d instance on
initialization to query for supported configs anyway. Do you see any issues with
that?

[piman, 2011-08-15 21:46:38]

On Mon, Aug 15, 2011 at 2:24 PM, <alokp@chromium.org> wrote:

>
>  >
>> > I can leave it here, but max-surface-size is the minimum of
>> > max-render-buffer and max-texture-size, which can be queried in the EGL
>> > implementation.
>>
>
>
>  You're talking about the EGL implementation as if it was standalone... EGL
>> has to be written on top of pepper APIs, so you'll still need some sort of
>> API to query max-render-buffer and max-texture-size.
>>
>
>
> I was thinking of creating an off-screen 1x1 graphics-3d instance when EGL
> is
> initialized. Once you have an OpenGL context, you could query
> max-render-buffer
> and max-texture-size. I would need to create a graphics-3d instance on
> initialization to query for supported configs anyway. Do you see any issues
> with
> that?


The main issue is that it's a heavyweight operation. Creating a context
today involves many round-trip IPCs (that need to hop 2 processes),
allocating a bunch of shared memory, as well as taking GPU resources. Plus
each query would be another round-trip. I could easily see this take 100s of
ms or more.
All you really need is a list of values, that you could get with a single
round-trip (that you could even further optimize by passing along when
creating the Pepper Instance).


>
>
http://codereview.chromium.**org/7576012/<http://codereview.chromium.org/7576...
>

[brettw, 2011-08-15 22:04:51]

On Mon, Aug 15, 2011 at 2:46 PM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Mon, Aug 15, 2011 at 2:24 PM, <alokp@chromium.org> wrote:
>>
>>> >
>>> > I can leave it here, but max-surface-size is the minimum of
>>> > max-render-buffer and max-texture-size, which can be queried in the EGL
>>> > implementation.
>>
>>
>>> You're talking about the EGL implementation as if it was standalone...
>>> EGL
>>> has to be written on top of pepper APIs, so you'll still need some sort
>>> of
>>> API to query max-render-buffer and max-texture-size.
>>
>>
>> I was thinking of creating an off-screen 1x1 graphics-3d instance when EGL
>> is
>> initialized. Once you have an OpenGL context, you could query
>> max-render-buffer
>> and max-texture-size. I would need to create a graphics-3d instance on
>> initialization to query for supported configs anyway. Do you see any
>> issues with
>> that?
>
> The main issue is that it's a heavyweight operation. Creating a context
> today involves many round-trip IPCs (that need to hop 2 processes),
> allocating a bunch of shared memory, as well as taking GPU resources. Plus
> each query would be another round-trip. I could easily see this take 100s of
> ms or more.
> All you really need is a list of values, that you could get with a single
> round-trip (that you could even further optimize by passing along when
> creating the Pepper Instance).

This sounds good to me.

Brett

[alokp, 2011-08-16 15:52:14]

> The main issue is that it's a heavyweight operation. Creating a context
> today involves many round-trip IPCs (that need to hop 2 processes),
> allocating a bunch of shared memory, as well as taking GPU resources. Plus
> each query would be another round-trip. I could easily see this take 100s of
> ms or more.

I agree that creating a context is currently heavyweight. But querying does not
hit the IPC once GLES2Implementation is created. It uses
glGetMultipleIntegervCHROMIUM to query for multiple states in one call and
caches them on the client side.

> All you really need is a list of values, that you could get with a single
> round-trip (that you could even further optimize by passing along when
> creating the Pepper Instance).
>

Sorry I do not understand your proposal. We already have
PPB_Graphics3D_Dev::GetAttribs() which returns a list of values in a single
call. But it can only be called AFTER you have created a PPB_Graphics3D_Dev
instance. The client might need to query for max-surface-size BEFORE creating an
instance. OpenGL API does not have a chicken-and-egg problem. It allows you to
query for max-texture-size before allocating a texture.

[piman, 2011-08-16 17:02:23]

On Tue, Aug 16, 2011 at 8:52 AM, <alokp@chromium.org> wrote:

>
>  The main issue is that it's a heavyweight operation. Creating a context
>> today involves many round-trip IPCs (that need to hop 2 processes),
>> allocating a bunch of shared memory, as well as taking GPU resources. Plus
>> each query would be another round-trip. I could easily see this take 100s
>> of
>> ms or more.
>>
>
> I agree that creating a context is currently heavyweight. But querying does
> not
> hit the IPC once GLES2Implementation is created. It uses
> glGetMultipleIntegervCHROMIUM to query for multiple states in one call and
> caches them on the client side.
>
>
>  All you really need is a list of values, that you could get with a single
>> round-trip (that you could even further optimize by passing along when
>> creating the Pepper Instance).
>>
>
>
> Sorry I do not understand your proposal. We already have
> PPB_Graphics3D_Dev::**GetAttribs() which returns a list of values in a
> single
> call. But it can only be called AFTER you have created a PPB_Graphics3D_Dev
> instance. The client might need to query for max-surface-size BEFORE
> creating an
> instance. OpenGL API does not have a chicken-and-egg problem. It allows you
> to
> query for max-texture-size before allocating a texture.


Right, what I'm saying is you can query in the GPU process those values way
before you create the pepper context. They're static values and won't
change. So you can pass them to the renderer, and along to the pepper
process as you create the plugin instance. That way, the value is available
before you create any pepper Graphics3D, and you don't need any round trip
or heavyweight context creation.

Antoine


>
>
>
http://codereview.chromium.**org/7576012/<http://codereview.chromium.org/7576...
>

[alokp, 2011-08-16 20:25:26]

> 
> Right, what I'm saying is you can query in the GPU process those values way
> before you create the pepper context. They're static values and won't
> change. So you can pass them to the renderer, and along to the pepper
> process as you create the plugin instance. That way, the value is available
> before you create any pepper Graphics3D, and you don't need any round trip
> or heavyweight context creation.
> 
> Antoine
> 

I see. It seems we need something like PPB_Graphics3D_Dev::GetAttribLimits()
that takes PPB_Instance parameter. I will do it in another CL.

[piman, 2011-08-16 20:52:10]

On Tue, Aug 16, 2011 at 1:25 PM, <alokp@chromium.org> wrote:

>
>
>  Right, what I'm saying is you can query in the GPU process those values
>> way
>> before you create the pepper context. They're static values and won't
>> change. So you can pass them to the renderer, and along to the pepper
>> process as you create the plugin instance. That way, the value is
>> available
>> before you create any pepper Graphics3D, and you don't need any round trip
>> or heavyweight context creation.
>>
>
>  Antoine
>>
>
>
> I see. It seems we need something like PPB_Graphics3D_Dev::**
> GetAttribLimits()
> that takes PPB_Instance parameter. I will do it in another CL.


Yes, that's fine. I already gave the LGTM on this one.

Antoine

>
>
>
http://codereview.chromium.**org/7576012/<http://codereview.chromium.org/7576...
>