Added PPB_Graphics3D_Dev::Resize to let plugins resize the backing surface.

Added PPB_Graphics3D_Dev::Resize to let plugins resize the backing surface.
BUG=62383
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94930

[alokp, 2011-07-28 21:30:40]

Also updated demos to exercise the new API here:
http://codereview.chromium.org/7529009/

[apatrick_chromium, 2011-07-28 22:11:26]

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
File ppapi/shared_impl/graphics_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
ppapi/shared_impl/graphics_3d_impl.cc:41: // TODO(alokp): Check if resize
succeeded and return appropriate error code.
You might want to Flush after ResizeCHROMIUM so that subsequent IPCs don't race
ahead of it. I'm not familiar with this code though... piman will know.

[alokp, 2011-07-28 22:40:12]

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
File ppapi/shared_impl/graphics_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
ppapi/shared_impl/graphics_3d_impl.cc:41: // TODO(alokp): Check if resize
succeeded and return appropriate error code.
On 2011/07/28 22:11:26, apatrick_chromium wrote:
> You might want to Flush after ResizeCHROMIUM so that subsequent IPCs don't
race
> ahead of it. I'm not familiar with this code though... piman will know.

Glad that you mentioned. All demos except stencil-test is working fine with this
patch. But stencil test is only producing a green quad that spans the whole
viewport. It is supposed to draw four quads. The exe version of stencil test
works. Commenting out the resize calls in pepper.cc works (although stretched).
I have tried adding flush everywhere without much success. It also randomly
crashes in ANGLE. It may be just specific to the stencil buffer.

[piman, 2011-07-28 23:35:06]

http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platfor...
File content/renderer/pepper_platform_context_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platfor...
content/renderer/pepper_platform_context_3d_impl.cc:79: };
You could move this to PPB_Context3D_Impl

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

http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:34: #define PPB_GRAPHICS_3D_DEV_INTERFACE_0_5
"PPB_Graphics3D(Dev);0.5"
You need to rev the interface.

http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:206: //   insufficient resources.
You won't know about this case without doing a sync round-trip to the gpu
process. I think it would be better in general to treat this as a GL command
(after all it's just glResizeCHROMIUM), returning no error here, but setting the
GL error. If the plugin cares, it can call glGetError which will do the round
trip.

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
File ppapi/shared_impl/graphics_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
ppapi/shared_impl/graphics_3d_impl.cc:40: gles2_impl()->ResizeCHROMIUM(width,
height);
You won't have a gles2_impl in the out-of-process case

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
ppapi/shared_impl/graphics_3d_impl.cc:41: // TODO(alokp): Check if resize
succeeded and return appropriate error code.
On 2011/07/28 22:11:26, apatrick_chromium wrote:
> You might want to Flush after ResizeCHROMIUM so that subsequent IPCs don't
race
> ahead of it. I'm not familiar with this code though... piman will know.

I don't think a Flush is necessary since ResizeChromium goes through command
buffers.

[alokp, 2011-07-29 16:13:12]

I also renamed Resize() to ResizeBuffers() to make it consistent with
SwapBuffers() and also make it future-proof in case we would need to resize
something else.

http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platfor...
File content/renderer/pepper_platform_context_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platfor...
content/renderer/pepper_platform_context_3d_impl.cc:79: };
On 2011/07/28 23:35:06, piman wrote:
> You could move this to PPB_Context3D_Impl

Good idea. Done.

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

http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:34: #define PPB_GRAPHICS_3D_DEV_INTERFACE_0_5
"PPB_Graphics3D(Dev);0.5"
On 2011/07/28 23:35:06, piman wrote:
> You need to rev the interface.

Done.

http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:206: //   insufficient resources.
On 2011/07/28 23:35:06, piman wrote:
> You won't know about this case without doing a sync round-trip to the gpu
> process. I think it would be better in general to treat this as a GL command
> (after all it's just glResizeCHROMIUM), returning no error here, but setting
the
> GL error. If the plugin cares, it can call glGetError which will do the round
> trip.

Yes I was worried about the round-trip, hence the TODO. There are a couple of
options to avoid the round trip:

1. Expose this as a GL function as you suggest. It is certainly practical but
not very clean API. We create and swap buffers in this interface. But having to
jump over to a the GL interface to resize it is confusing. Another problem with
this approach is that we will have to expose this as a GL extension, which by
definition is optional. But this is not optional. It will be impossible to use
Pepper 3D without this extension.

2. Add documentation that while you could issue a resize command here, you need
to call glGetError to catch out-of-memory condition. Again confusing.

3. Make it asynchronous like swap-buffers. One more callback to track may be too
complicated.

4. Make the callback for SwapBuffers accept an error code. This will also get
rid of the PPP_Graphics3D_Dev::Graphics3DContextLost(). After every frame we
could return one of the following errors generated by this interface:
- PP_GRAPHICS3D_ERROR_CONTEXT_LOST
- PP_ERROR_NO_MEMORY

I like the last option the best. It makes the API much simpler.

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
File ppapi/shared_impl/graphics_3d_impl.cc (right):

http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_i...
ppapi/shared_impl/graphics_3d_impl.cc:40: gles2_impl()->ResizeCHROMIUM(width,
height);
On 2011/07/28 23:35:06, piman wrote:
> You won't have a gles2_impl in the out-of-process case

In that case this command will be handled by the proxy which shares this
implementation. We have to check for the availability of gles2_impl only in
swap-buffers because the proxy delegates some of the task to the host. This is
not the case here.

[piman, 2011-07-29 16:31:13]

On Fri, Jul 29, 2011 at 9:13 AM, <alokp@chromium.org> wrote:

> I also renamed Resize() to ResizeBuffers() to make it consistent with
> SwapBuffers() and also make it future-proof in case we would need to resize
> something else.


I don't see the new patch yet. Upload failure ?


>
>
>
> http://codereview.chromium.**org/7530010/diff/1/content/**
>
renderer/pepper_platform_**context_3d_impl.cc<http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platform_context_3d_impl.cc>
> File content/renderer/pepper_**platform_context_3d_impl.cc (right):
>
> http://codereview.chromium.**org/7530010/diff/1/content/**
>
renderer/pepper_platform_**context_3d_impl.cc#newcode79<http://codereview.chromium.org/7530010/diff/1/content/renderer/pepper_platform_context_3d_impl.cc#newcode79>
> content/renderer/pepper_**platform_context_3d_impl.cc:**79: };
> On 2011/07/28 23:35:06, piman wrote:
>
>> You could move this to PPB_Context3D_Impl
>>
>
> Good idea. Done.
>
>
> http://codereview.chromium.**org/7530010/diff/1/ppapi/c/**
>
dev/ppb_graphics_3d_dev.h<http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h>
> File ppapi/c/dev/ppb_graphics_3d_**dev.h (right):
>
> http://codereview.chromium.**org/7530010/diff/1/ppapi/c/**
>
dev/ppb_graphics_3d_dev.h#**newcode34<http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h#newcode34>
> ppapi/c/dev/ppb_graphics_3d_**dev.h:34: #define
> PPB_GRAPHICS_3D_DEV_INTERFACE_**0_5 "PPB_Graphics3D(Dev);0.5"
> On 2011/07/28 23:35:06, piman wrote:
>
>> You need to rev the interface.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7530010/diff/1/ppapi/c/**
>
dev/ppb_graphics_3d_dev.h#**newcode206<http://codereview.chromium.org/7530010/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h#newcode206>
> ppapi/c/dev/ppb_graphics_3d_**dev.h:206: //   insufficient resources.
> On 2011/07/28 23:35:06, piman wrote:
>
>> You won't know about this case without doing a sync round-trip to the
>>
> gpu
>
>> process. I think it would be better in general to treat this as a GL
>>
> command
>
>> (after all it's just glResizeCHROMIUM), returning no error here, but
>>
> setting the
>
>> GL error. If the plugin cares, it can call glGetError which will do
>>
> the round
>
>> trip.
>>
>
> Yes I was worried about the round-trip, hence the TODO. There are a
> couple of options to avoid the round trip:
>
> 1. Expose this as a GL function as you suggest. It is certainly
> practical but not very clean API. We create and swap buffers in this
> interface. But having to jump over to a the GL interface to resize it is
> confusing. Another problem with this approach is that we will have to
> expose this as a GL extension, which by definition is optional. But this
> is not optional. It will be impossible to use Pepper 3D without this
> extension.
>

Yeah, I just meant keep it in this interface, but make it like the GL calls
(return void, set the GL error). I guess it's more or less like option 2.


>
> 2. Add documentation that while you could issue a resize command here,
> you need to call glGetError to catch out-of-memory condition. Again
> confusing.
>
> 3. Make it asynchronous like swap-buffers. One more callback to track
> may be too complicated.
>
> 4. Make the callback for SwapBuffers accept an error code. This will
> also get rid of the PPP_Graphics3D_Dev::**Graphics3DContextLost(). After
> every frame we could return one of the following errors generated by
> this interface:
> - PP_GRAPHICS3D_ERROR_CONTEXT_**LOST
> - PP_ERROR_NO_MEMORY
>
> I like the last option the best. It makes the API much simpler.


That seems like a good idea in general, and I like it, it makes it easy from
the user's POV. Just one thing, in the case that it's CPU-costly to draw a
frame, the user may want to check for the result of resize before starting a
draw cycle, paying for the round trip. To support that case, would it be
possible to also set the GL error ?


>
> http://codereview.chromium.**org/7530010/diff/1/ppapi/**
>
shared_impl/graphics_3d_impl.**cc<http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_impl.cc>
> File ppapi/shared_impl/graphics_3d_**impl.cc (right):
>
> http://codereview.chromium.**org/7530010/diff/1/ppapi/**
>
shared_impl/graphics_3d_impl.**cc#newcode40<http://codereview.chromium.org/7530010/diff/1/ppapi/shared_impl/graphics_3d_impl.cc#newcode40>
> ppapi/shared_impl/graphics_3d_**impl.cc:40:
> gles2_impl()->ResizeCHROMIUM(**width, height);
> On 2011/07/28 23:35:06, piman wrote:
>
>> You won't have a gles2_impl in the out-of-process case
>>
>
> In that case this command will be handled by the proxy which shares this
> implementation. We have to check for the availability of gles2_impl only
> in swap-buffers because the proxy delegates some of the task to the
> host. This is not the case here.


Doh. Yeah.

Antoine


>
>
>
http://codereview.chromium.**org/7530010/<http://codereview.chromium.org/7530...
>

[alokp, 2011-07-29 17:02:56]

> That seems like a good idea in general, and I like it, it makes it easy from
> the user's POV. Just one thing, in the case that it's CPU-costly to draw a
> frame, the user may want to check for the result of resize before starting a
> draw cycle, paying for the round trip. To support that case, would it be
> possible to also set the GL error ?
> 

Command buffer tries to keep GL errors due to internal usage separate. But we
could make an exception in this case. Although in an out-of-memory condition,
the application will get a GL_OUT_OF_MEMORY very soon anyway. Another way to
check for success of resize would be to do a swap-buffers right after resize.

[alokp, 2011-07-29 17:03:34]

Updated documentation for ResizeBuffers() and uploaded patch. PTAL.

[piman, 2011-07-29 17:17:23]

On Fri, Jul 29, 2011 at 10:02 AM, <alokp@chromium.org> wrote:

>  That seems like a good idea in general, and I like it, it makes it easy
>> from
>> the user's POV. Just one thing, in the case that it's CPU-costly to draw a
>> frame, the user may want to check for the result of resize before starting
>> a
>> draw cycle, paying for the round trip. To support that case, would it be
>> possible to also set the GL error ?
>>
>
>
> Command buffer tries to keep GL errors due to internal usage separate. But
> we
> could make an exception in this case. Although in an out-of-memory
> condition,
> the application will get a GL_OUT_OF_MEMORY very soon anyway.


If the user tries to resize to something crazy (e.g. 10k x 10k), it'll just
fail, but won't necessarily cause a GL_OUT_OF_MEMORY.
The command buffer does set errors that don't come directly from the
underlying GL (e.g. invalid enum values), so there's definitely a precedent.


> Another way to
> check for success of resize would be to do a swap-buffers right after
> resize.


No, that'd be broken - you'll post invalid bits and trigger compositing
(causing a bunch side effects) when you just want to check for an error.

Antoine


>
>
http://codereview.chromium.**org/7530010/<http://codereview.chromium.org/7530...
>

[alokp, 2011-07-29 17:43:01]

> If the user tries to resize to something crazy (e.g. 10k x 10k), it'll just
> fail, but won't necessarily cause a GL_OUT_OF_MEMORY.
> The command buffer does set errors that don't come directly from the
> underlying GL (e.g. invalid enum values), so there's definitely a precedent.
> 

AFAIK glRenderbufferStorage generates GL_OUT_OF_MEMORY. If it just fails, what
should be returned by glGetError()?

> > Another way to
> > check for success of resize would be to do a swap-buffers right after
> > resize.
> 
> 
> No, that'd be broken - you'll post invalid bits and trigger compositing
> (causing a bunch side effects) when you just want to check for an error.
> 

Doh. Yes you are right.

I guess the decision we have to make is: Is this use-case important enough to
introduce any confusion/complexity? Issuing a command in one interface and
catching error in another is not very appetizing.

[piman, 2011-07-29 18:16:50]

On Fri, Jul 29, 2011 at 10:43 AM, <alokp@chromium.org> wrote:

>
>  If the user tries to resize to something crazy (e.g. 10k x 10k), it'll
>> just
>> fail, but won't necessarily cause a GL_OUT_OF_MEMORY.
>> The command buffer does set errors that don't come directly from the
>> underlying GL (e.g. invalid enum values), so there's definitely a
>> precedent.
>>
>
>
> AFAIK glRenderbufferStorage generates GL_OUT_OF_MEMORY.


If it's above the max size, it'll generate GL_INVALID_VALUE. If it's out of
memory it'll generate GL_OUT_OF_MEMORY.


> If it just fails, what
> should be returned by glGetError()?


What do you mean, "just fails" ? It'll fail for a reason, that reason can be
encoded in GL errors.

>
>
>  > Another way to
>> > check for success of resize would be to do a swap-buffers right after
>> > resize.
>>
>
>
>  No, that'd be broken - you'll post invalid bits and trigger compositing
>> (causing a bunch side effects) when you just want to check for an error.
>>
>
>
> Doh. Yes you are right.
>
> I guess the decision we have to make is: Is this use-case important enough
> to
> introduce any confusion/complexity? Issuing a command in one interface and
> catching error in another is not very appetizing.


Like I said, you can have both. Have an error value in the swapbuffer
callback, but also set a GL error
in GLES2DecoderImpl::UpdateOffscreenFrameBufferSize
I don't have an issue with multiple interfaces accessing the same state,
it's already the case.

Antoine

>
>
>
>
http://codereview.chromium.**org/7530010/<http://codereview.chromium.org/7530...
>

[alokp, 2011-07-29 20:02:51]

> 
> If it's above the max size, it'll generate GL_INVALID_VALUE. If it's out of
> memory it'll generate GL_OUT_OF_MEMORY.
> 

We can catch the GL_INVALID_VALUE case on the client side. gles2-impl has
max-texture-size and max-renderbuffer-size cached.

> Like I said, you can have both. Have an error value in the swapbuffer
> callback, but also set a GL error
> in GLES2DecoderImpl::UpdateOffscreenFrameBufferSize
> I don't have an issue with multiple interfaces accessing the same state,
> it's already the case.
> 

I do not see how its already the case. Could you point me to a Graphics3D
function that sets a GL error? Generally EGL/WGL/GLX have their own error state.
AFAIK an EGL function does not cause a GL error.

[piman, 2011-07-29 20:24:03]

On Fri, Jul 29, 2011 at 1:02 PM, <alokp@chromium.org> wrote:

>
>
>  If it's above the max size, it'll generate GL_INVALID_VALUE. If it's out
>> of
>> memory it'll generate GL_OUT_OF_MEMORY.
>>
>
>
> We can catch the GL_INVALID_VALUE case on the client side. gles2-impl has
> max-texture-size and max-renderbuffer-size cached.


The code in RenderBuffer::AllocateStorage/Texture::AllocateStorage eats the
GL error. It returns a bool
to GLES2DecoderImpl::UpdateOffscreenFrameBufferSize but it doesn't end up
traslating to a GL error in the case of ResizeCHROMIUM.

>
>
>  Like I said, you can have both. Have an error value in the swapbuffer
>> callback, but also set a GL error
>> in GLES2DecoderImpl::**UpdateOffscreenFrameBufferSize
>> I don't have an issue with multiple interfaces accessing the same state,
>> it's already the case.
>>
>
>
> I do not see how its already the case. Could you point me to a Graphics3D
> function that sets a GL error? Generally EGL/WGL/GLX have their own error
> state.
> AFAIK an EGL function does not cause a GL error.


Oh, I see what you mean. IMO the separation between PPB_Graphics3D_Dev and
 PPB_OpenGLES2_Dev is arbitrary. I really don't see an issue with setting a
GL error.
If you want to get technical, SwapBuffers can generate GL_OUT_OF_MEMORY,
since all the calls in the underlying GL can.

http://codereview.chromium.**org/7530010/<http://codereview.chromium.org/7530...
>

[alokp, 2011-07-29 21:45:49]

> The code in RenderBuffer::AllocateStorage/Texture::AllocateStorage eats the
> GL error. It returns a bool
> to GLES2DecoderImpl::UpdateOffscreenFrameBufferSize but it doesn't end up
> traslating to a GL error in the case of ResizeCHROMIUM.
> 

Yeah I think the command-buffer code is trying to separate the GL errors
generated by the app and those generated by itself. We can fix this depending on
how we want to handle the error for ResizeCHROMIUM.

> 
> Oh, I see what you mean. IMO the separation between PPB_Graphics3D_Dev and
>  PPB_OpenGLES2_Dev is arbitrary. I really don't see an issue with setting a
> GL error.
> If you want to get technical, SwapBuffers can generate GL_OUT_OF_MEMORY,
> since all the calls in the underlying GL can.
> 

I agree that separation is arbitrary. I will think about it more. Other than
this, do you have any other issues with the patch? I would like to submit it so
apatrick can help me debug the crash I am seeing.

[piman, 2011-07-29 22:35:45]

On Fri, Jul 29, 2011 at 2:45 PM, <alokp@chromium.org> wrote:

>
>  The code in RenderBuffer::AllocateStorage/**Texture::AllocateStorage eats
>> the
>> GL error. It returns a bool
>> to GLES2DecoderImpl::**UpdateOffscreenFrameBufferSize but it doesn't end
>> up
>> traslating to a GL error in the case of ResizeCHROMIUM.
>>
>
>
> Yeah I think the command-buffer code is trying to separate the GL errors
> generated by the app and those generated by itself. We can fix this
> depending on
> how we want to handle the error for ResizeCHROMIUM.


Today, it silently fails. It should be fixed.


> Oh, I see what you mean. IMO the separation between PPB_Graphics3D_Dev and
>>  PPB_OpenGLES2_Dev is arbitrary. I really don't see an issue with setting
>> a
>> GL error.
>> If you want to get technical, SwapBuffers can generate GL_OUT_OF_MEMORY,
>> since all the calls in the underlying GL can.
>>
>
>
> I agree that separation is arbitrary. I will think about it more. Other
> than
> this, do you have any other issues with the patch? I would like to submit
> it so
> apatrick can help me debug the crash I am seeing.


Please file a bug for the remaining issue, and then LGTM.

Antoine

>
>
>
>
http://codereview.chromium.**org/7530010/<http://codereview.chromium.org/7530...
>

[alokp, 2011-07-29 22:51:24]

> 
> Please file a bug for the remaining issue, and then LGTM.
> 
> Antoine

http://code.google.com/p/chromium/issues/detail?id=91045

[alokp, 2011-08-01 15:41:15]

brettw: OWNERS approval needed for ppapi.

[brettw, 2011-08-01 16:24:54]

LGTM