Pepper 3D API changes:

Pepper 3D API changes:
1. Added GetAttribMaxValue() and GetError()
2. Fixed the documentation for SwapBuffers()
3. Replaced PP_GRAPHICS3DERROR_CONTEXT_LOST with PP_ERROR_CONTEXT_LOST

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99532

[alokp, 2011-09-02 16:15:26]

The changes in header files have already been reviewed in
http://codereview.chromium.org/7745026/

This patch contains required changes in source files as well.

[nfullagar, 2011-09-02 18:34:54]

one Q otherwise lgtm

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
ppapi/thunk/ppb_graphics_3d_thunk.cc:23: // TODO(alokp): Implement me.
What's the plan on impl vs moving out of dev here?

[alokp, 2011-09-02 19:57:40]

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
ppapi/thunk/ppb_graphics_3d_thunk.cc:23: // TODO(alokp): Implement me.
On 2011/09/02 18:34:54, nfullagar wrote:
> What's the plan on impl vs moving out of dev here?

My understanding from API review meeting was that we wanted to firm up the API
before the M15 branch. We will definitely need to complete the implementation
before M15 beta.

[brettw, 2011-09-02 20:21:45]

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
ppapi/c/dev/ppb_graphics_3d_dev.h:145: // rendering. Note that the same error
code is also returned in the swap
It's not clear to me when I'm supposed to call this function.

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/pp_errors.h
File ppapi/c/pp_errors.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/pp_errors.h#newcode91
ppapi/c/pp_errors.h:91: * power management event.
"power management event"? I'd expect that to be like "you're on battery so we're
doing something different. Does this cause a context lost? Aren't there other
ways you can get a context lost? Maybe you can rephrase this?

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc
(right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:47: //
TODO(nfullagar): Implement me.
2 space indent.

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:116: //
TODO(nfullagar): Implement me.
Ditto

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
ppapi/thunk/ppb_graphics_3d_thunk.cc:23: // TODO(alokp): Implement me.
2 space indent, same below.

[alokp, 2011-09-02 21:41:02]

I personally think it will be fine to release V1 without GetError(). We can
always add it if required.

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
ppapi/c/dev/ppb_graphics_3d_dev.h:145: // rendering. Note that the same error
code is also returned in the swap
On 2011/09/02 20:21:45, brettw wrote:
> It's not clear to me when I'm supposed to call this function.

The context may get lost anytime. It may go out of memory when attempting to
resize the backing surface. An application may call this function after resize.
Although not recommended, but an application may even call it at the beginning
of a frame to make sure it can be rendered to.

If your question is "why is it part of the API when the error code is already
returned by SwapBuffers?", then please see the discussion here -
http://codereview.chromium.org/7530010. Antoine wanted to support one particular
use case - "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".

I think that most applications will not call this function ever. They will just
check the error code in SwapBuffers callback.

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/pp_errors.h
File ppapi/c/pp_errors.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/pp_errors.h#newcode91
ppapi/c/pp_errors.h:91: * power management event.
On 2011/09/02 20:21:45, brettw wrote:
> "power management event"? I'd expect that to be like "you're on battery so
we're
> doing something different. Does this cause a context lost? Aren't there other
> ways you can get a context lost? Maybe you can rephrase this?

I am not quite sure of the definition of "power management event". I think it is
implementation dependent and the EGL spec is pretty vague. I will ask around in
the gpu team and rephrase the documentation.

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc
(right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:47: //
TODO(nfullagar): Implement me.
On 2011/09/02 20:21:45, brettw wrote:
> 2 space indent.

Done.

http://codereview.chromium.org/7824040/diff/2002/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_graphics_3d.cc:116: //
TODO(nfullagar): Implement me.
On 2011/09/02 20:21:45, brettw wrote:
> Ditto

Done.

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
File ppapi/thunk/ppb_graphics_3d_thunk.cc (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/thunk/ppb_graphics_3d_...
ppapi/thunk/ppb_graphics_3d_thunk.cc:23: // TODO(alokp): Implement me.
On 2011/09/02 20:21:45, brettw wrote:
> 2 space indent, same below.

Done.

[brettw, 2011-09-02 22:14:24]

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
ppapi/c/dev/ppb_graphics_3d_dev.h:145: // rendering. Note that the same error
code is also returned in the swap
Then I'd like to add to the comment that this returns the same error code
provided by the SwapBuffers callback. If this function can return some data
before the callback occurs, can we mention this?

My problem is that as somebody thinking about using this API, it's not clear if
I should be calling this function, and when I should be calling it. I might be
tempted to call it all the time to make sure there's no error, which would be
incorrect. We at least need to have enough info here so that the people who
don't need this function know they don't need it.

[alokp, 2011-09-02 23:45:06]

PTAL at the documentation changes for GetError and SwapBuffers.

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7824040/diff/2002/ppapi/c/dev/ppb_graphics_3d_...
ppapi/c/dev/ppb_graphics_3d_dev.h:145: // rendering. Note that the same error
code is also returned in the swap
On 2011/09/02 22:14:24, brettw wrote:
> Then I'd like to add to the comment that this returns the same error code
> provided by the SwapBuffers callback.

I already have this in the comment - "Note that the same error code is also
returned in the swap buffers callback". Is that what you were looking for?
 
> My problem is that as somebody thinking about using this API, it's not clear
if
> I should be calling this function, and when I should be calling it. I might be
> tempted to call it all the time to make sure there's no error, which would be
> incorrect. We at least need to have enough info here so that the people who
> don't need this function know they don't need it.

I agree it is confusing and could lead to many applications calling a
synchronous function unnecessarily. I have tried to make it clearer by adding
more documentation to GetError and SwapBuffers, but still think we should remove
this from V1. SwapBuffers callback covers most use cases.

[brettw, 2011-09-03 00:11:47]

LGTM, new explanation is great.

[commit-bot: I haz the power, 2011-09-03 03:19:42]

Change committed as 99532