Proposed changes for Pepper 3D API.

Proposed changes for Pepper 3D API.

[alokp, 2011-08-25 18:07:55]

Proposed changes for API review

[brettw, 2011-08-25 19:25:31]

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

http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:154: // - PP_GRAPHICS3DERROR_CONTEXT_LOST
I'm OK just calling this PP_ERROR_CONTEXT_LOST. We've had some discussions about
wanting the 2D graphics backing store to be able to be thrown away, so could
re-use this error there as well.

[piman, 2011-08-25 19:58:09]

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

http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev....
ppapi/c/dev/pp_graphics_3d_dev.h:36: PP_GRAPHICS3DATTRIB_BIND_RGB = 0x3039,
I think we want more flexibility than this here. At least:
- pre-multiplied alpha (what is the default today)
- non-premul alpha (what many games would typically do, also available on WebGL)
- ignore alpha

See bug http://code.google.com/p/chromium/issues/detail?id=74209

I think we'd want specifying the complete blending function, as 2 enum values
for source and destination, providing the most flexibility, without adding
complexity (since we can do it all in the compositor).

[alokp, 2011-08-29 18:01:21]

On 2011/08/25 19:58:09, piman wrote:
> http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev.h
> File ppapi/c/dev/pp_graphics_3d_dev.h (right):
> 
>
http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev....
> ppapi/c/dev/pp_graphics_3d_dev.h:36: PP_GRAPHICS3DATTRIB_BIND_RGB = 0x3039,
> I think we want more flexibility than this here. At least:
> - pre-multiplied alpha (what is the default today)
> - non-premul alpha (what many games would typically do, also available on
WebGL)
> - ignore alpha
> 
> See bug http://code.google.com/p/chromium/issues/detail?id=74209
> 
> I think we'd want specifying the complete blending function, as 2 enum values
> for source and destination, providing the most flexibility, without adding
> complexity (since we can do it all in the compositor).

Would specifying complete blending function conflict with any of the CSS
settings? opacity perhaps? Would PPB_LayerCompositor_Dev expose something
similar?

[piman, 2011-08-29 18:06:14]

On Mon, Aug 29, 2011 at 11:01 AM, <alokp@chromium.org> wrote:

> On 2011/08/25 19:58:09, piman wrote:
>
>> http://codereview.chromium.**org/7745026/diff/1/ppapi/c/**
>>
dev/pp_graphics_3d_dev.h<http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev.h>
>> File ppapi/c/dev/pp_graphics_3d_**dev.h (right):
>>
>
>
> http://codereview.chromium.**org/7745026/diff/1/ppapi/c/**
>
dev/pp_graphics_3d_dev.h#**newcode36<http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev.h#newcode36>
>
>> ppapi/c/dev/pp_graphics_3d_**dev.h:36: PP_GRAPHICS3DATTRIB_BIND_RGB =
>> 0x3039,
>> I think we want more flexibility than this here. At least:
>> - pre-multiplied alpha (what is the default today)
>> - non-premul alpha (what many games would typically do, also available on
>>
> WebGL)
>
>> - ignore alpha
>>
>
>  See bug
http://code.google.com/p/**chromium/issues/detail?id=**74209<http://code.goog...
>>
>
>  I think we'd want specifying the complete blending function, as 2 enum
>> values
>> for source and destination, providing the most flexibility, without adding
>> complexity (since we can do it all in the compositor).
>>
>
> Would specifying complete blending function conflict with any of the CSS
> settings?


I'm not sure, you'd have to ask a CSS guru. However, given that
traditionally plugins didn't participate in the page compositing, there's
nothing we can really break here...


> opacity perhaps?


I think opacity is mostly orthogonal


> Would PPB_LayerCompositor_Dev expose something
> similar?


It will have to expose this for the individual layers.


>
>
>
http://codereview.chromium.**org/7745026/<http://codereview.chromium.org/7745...
>

[alokp, 2011-08-29 18:32:49]

It seems like all graphical APIs (graphics 2d/3d, video, ???) should expose such
a compositing function. In which case would it make sense to move the
compositing API to a common place - PPB_LayerCompositor_Dev. Maintaining the
same API at multiple places may be a pain.

[alokp, 2011-08-30 19:59:09]

After thinking about it a bit and discussing with dspringer@, I think a better
alternative would be to have a struct consisting of compositing settings. This
struct can be shared by all graphical APIs and passed in
PPB_Instance::BindGraphics() and PPB_LayerCompositor::AddLayer(). Thoughts?

[piman, 2011-08-30 20:06:13]

On Tue, Aug 30, 2011 at 12:59 PM, <alokp@chromium.org> wrote:

> After thinking about it a bit and discussing with dspringer@, I think a
> better
> alternative would be to have a struct consisting of compositing settings.
> This
> struct can be shared by all graphical APIs and passed in
> PPB_Instance::BindGraphics() and PPB_LayerCompositor::AddLayer(**).
> Thoughts?


That seems reasonable. We'll need to add a separate function to PPB_Instance
and rev the interface for backwards compatibility, but I think that should
be ok.

Antoine


>
>
>
http://codereview.chromium.**org/7745026/<http://codereview.chromium.org/7745...
>

[brettw, 2011-08-30 20:11:26]

I just proposed to Alok that we punt this unless there's a strong use case. 3D
is already a bit on the edge for M15 already, so I'd prefer not to jam more
stuff in.

Perhaps we can come up with a nice compositor API where we can set the configs
on the different layers, or set the config for "one and only bound graphics" for
the no-layer case. This would help keep our compositing modes in one place which
I think makes more sense and keeps us from having to rev instance.

[piman, 2011-08-30 20:14:46]

On Tue, Aug 30, 2011 at 1:11 PM, <brettw@chromium.org> wrote:

> I just proposed to Alok that we punt this unless there's a strong use case.
> 3D
> is already a bit on the edge for M15 already, so I'd prefer not to jam more
> stuff in.
>

For the RGBA vs RGB, there is a workaround, though it's a bit costly on
embedded platforms (clear the alpha to 1 before swapping).

Antoine

Perhaps we can come up with a nice compositor API where we can set the
> configs
> on the different layers, or set the config for "one and only bound
> graphics" for
> the no-layer case. This would help keep our compositing modes in one place
> which
> I think makes more sense and keeps us from having to rev instance.
>
>
>
http://codereview.chromium.**org/7745026/<http://codereview.chromium.org/7745...
>

[alokp1, 2011-08-30 20:29:21]

> 
> For the RGBA vs RGB, there is a workaround, though it's a bit costly on
> embedded platforms (clear the alpha to 1 before swapping).
> 
> Antoine
> 

For M15, can we also assume that the buffer has colors with premultiplied-alpha?
I would not want to add a setting for premultiplied-alpha which will become
redundant with the compositor API.

[piman, 2011-08-30 20:40:59]

On Tue, Aug 30, 2011 at 1:29 PM, <alokp@google.com> wrote:

>
>  For the RGBA vs RGB, there is a workaround, though it's a bit costly on
>> embedded platforms (clear the alpha to 1 before swapping).
>>
>
>  Antoine
>>
>
>
> For M15, can we also assume that the buffer has colors with
> premultiplied-alpha?
>

That's the current assumption, yes (but it may be contrary to the
expectation of, say, games).


> I would not want to add a setting for premultiplied-alpha which will become
> redundant with the compositor API.
>
>
>
http://codereview.chromium.**org/7745026/<http://codereview.chromium.org/7745...
>

[alokp, 2011-09-01 20:22:36]

Changes in the new patch:
1. Punted on RGB/RGBA as discussed in favor of a compositor API
2. Replaces GetAttribsMaxValue that takes an attrib_list with GetAttribMaxValue
that queries a single attribute. This function does not need to take an
attrib_list because the query would not involve a round trip to the gpu process.
3. Replaced PP_GRAPHICS3DERROR_CONTEXT_LOST with PP_ERROR_CONTEXT_LOST

PTAL

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

http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/ppb_graphics_3d_dev...
ppapi/c/dev/ppb_graphics_3d_dev.h:154: // - PP_GRAPHICS3DERROR_CONTEXT_LOST
On 2011/08/25 19:25:31, brettw wrote:
> I'm OK just calling this PP_ERROR_CONTEXT_LOST. We've had some discussions
about
> wanting the 2D graphics backing store to be able to be thrown away, so could
> re-use this error there as well.

Done.

[nfullagar, 2011-09-01 22:16:55]

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
ppapi/c/dev/ppb_graphics_3d_dev.h:175: // rest of the html page. While you are
waiting for a Flush callback,
Flush callback -> SwapBuffers callback

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
ppapi/c/dev/ppb_graphics_3d_dev.h:176: // additional calls to Flush will fail.
Flush -> SwapBuffers

[piman, 2011-09-01 22:21:49]

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
ppapi/c/dev/ppb_graphics_3d_dev.h:31: #define PPB_GRAPHICS_3D_DEV_INTERFACE_0_8
"PPB_Graphics3D(Dev);0.8"
You need to rev the API version

[alokp, 2011-09-02 15:10:39]

I am going to combine this patch with required source file changes and send
another request for CR.

Please let me know if further changes are needed in the API.

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
File ppapi/c/dev/ppb_graphics_3d_dev.h (right):

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
ppapi/c/dev/ppb_graphics_3d_dev.h:175: // rest of the html page. While you are
waiting for a Flush callback,
On 2011/09/01 22:16:55, nfullagar wrote:
> Flush callback -> SwapBuffers callback

Done.

http://codereview.chromium.org/7745026/diff/13001/ppapi/c/dev/ppb_graphics_3d...
ppapi/c/dev/ppb_graphics_3d_dev.h:176: // additional calls to Flush will fail.
On 2011/09/01 22:16:55, nfullagar wrote:
> Flush -> SwapBuffers

Done.

[greggman, 2011-09-22 21:01:25]

I don't know why this didn't this comment didn't get published but it still
seems relevant so publishing it now.

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

http://codereview.chromium.org/7745026/diff/1/ppapi/c/dev/pp_graphics_3d_dev....
ppapi/c/dev/pp_graphics_3d_dev.h:36: PP_GRAPHICS3DATTRIB_BIND_RGB = 0x3039,
Do we need that functionality here?

I'm pretty sure that's all being added to css so if you want it blended
different you just set the css.

Adding it here would then add complexity since you'd have to combine them with
the css blending modes to get the correct result.