Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5567004:
Proposed changes to Pepper 3D API. (Closed)
|
Created:
10 years ago by alokp Modified:
9 years, 6 months ago CC:
chromium-reviews, apatrick_chromium, gwink Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionProposed changes to Pepper 3D API.
Patch Set 1 #
Total comments: 14
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 16
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 3
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 8
Patch Set 11 : '' #
Total comments: 4
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 1
Patch Set 14 : '' #Patch Set 15 : '' #
Messages
Total messages: 16 (0 generated)
Not ready yet. Sending it out to receive feedback before the API review meeting.
http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:48: uint32_t (*IsGraphics3D)(PP_Resource resource, PP_Bool* value); I think there is a convention that PPAPI interfaces have an IsX method that returns PP_Bool and takes only the resource as argument. This is perhaps the only method that should not return an integer error code. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:55: uint32_t (*ChooseConfig)(const int32_t* attrib_list, Can you add a comment indicating that this method is redundant and could potentially be moved into an EGL-style library? http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:60: // TODO(apatrick): What to do if the browser window is moved to Can you remove this comment? We decided last meeting that since Chrome can move from one display to another and straddle displays that the we are pretty much forced to virtualize multiple displays as a single display. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:67: int32_t (*QueryString)(int32_t name, char* str, int32_t* str_size); How about: uint32_t (*QueryString)(int32_t name, char** str); The strings are constants as far as I know so no neeed to worry about length. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:75: uint32_t (*CreateSurface)(int32_t config, These surface related functions could go in a separate interface? http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:76: PP_Instance instance, Do you want to delete the instance argument here as well? http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:100: uint32_t (*BindContext)(PP_Resource draw, I don't understand how this works but I'm about to get off a bus...
Thanks Al. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev.h File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:48: uint32_t (*IsGraphics3D)(PP_Resource resource, PP_Bool* value); On 2010/12/03 18:36:36, apatrick wrote: > I think there is a convention that PPAPI interfaces have an IsX method that > returns PP_Bool and takes only the resource as argument. This is perhaps the > only method that should not return an integer error code. Done. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:55: uint32_t (*ChooseConfig)(const int32_t* attrib_list, On 2010/12/03 18:36:36, apatrick wrote: > Can you add a comment indicating that this method is redundant and could > potentially be moved into an EGL-style library? Done. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:60: // TODO(apatrick): What to do if the browser window is moved to On 2010/12/03 18:36:36, apatrick wrote: > Can you remove this comment? We decided last meeting that since Chrome can move > from one display to another and straddle displays that the we are pretty much > forced to virtualize multiple displays as a single display. Done. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:67: int32_t (*QueryString)(int32_t name, char* str, int32_t* str_size); On 2010/12/03 18:36:36, apatrick wrote: > How about: > uint32_t (*QueryString)(int32_t name, char** str); > > The strings are constants as far as I know so no neeed to worry about length. Done. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:75: uint32_t (*CreateSurface)(int32_t config, On 2010/12/03 18:36:36, apatrick wrote: > These surface related functions could go in a separate interface? Yes they could go into PPB_Surface3D. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:76: PP_Instance instance, On 2010/12/03 18:36:36, apatrick wrote: > Do you want to delete the instance argument here as well? Actually instance argument is required here. We are creating a surface for this particular instance. http://codereview.chromium.org/5567004/diff/1/ppapi/c/dev/ppb_graphics_3d_dev... ppapi/c/dev/ppb_graphics_3d_dev.h:100: uint32_t (*BindContext)(PP_Resource draw, On 2010/12/03 18:36:36, apatrick wrote: > I don't understand how this works but I'm about to get off a bus... I have a couple of options that we can discuss.
Looks okay. I'm sure more things will pop up when you actually try to implement EGL as a library. http://codereview.chromium.org/5567004/diff/11001/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): http://codereview.chromium.org/5567004/diff/11001/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:5213: file.Write(" // TODO(alokp): Add context argument to all gl functions.\n") I think that's an interesting idea. The way it works currently, all these functions are also exported so there isn't another GL library. If we were to add context argument here, it would mean that we do need an actual GL library that puts the current context in TLS and so forth. I like that, it removes the complexity from our proxy layer. We could also #define all those functions. We should talk about this today. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:67: // TODO(alokp): GetString may be a better name Isn't QueryString the EGL name? http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:74: // TODO(alokp): What about BindGraphics()? Move to PPB_Graphics2D. I don't understand this comment.
http://codereview.chromium.org/5567004/diff/11001/gpu/command_buffer/build_gl... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): http://codereview.chromium.org/5567004/diff/11001/gpu/command_buffer/build_gl... gpu/command_buffer/build_gles2_cmd_buffer.py:5213: file.Write(" // TODO(alokp): Add context argument to all gl functions.\n") On 2010/12/03 19:27:21, neb wrote: > I think that's an interesting idea. The way it works currently, all these > functions are also exported so there isn't another GL library. If we were to add > context argument here, it would mean that we do need an actual GL library that > puts the current context in TLS and so forth. I like that, it removes the > complexity from our proxy layer. We could also #define all those functions. We > should talk about this today. Yes removing complexity is the goal. TLS should be part of the helper library. We could either create two helper libraries - gles and egl, or fold them into one pgl library. I am not a big fan of #define because I ran into nasty issues while porting OpenSceneGraph. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:67: // TODO(alokp): GetString may be a better name On 2010/12/03 19:27:21, neb wrote: > Isn't QueryString the EGL name? Yes and thats why I am using it here but I think it is inconsistent. EGL inconsistently uses Get and Query. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:74: // TODO(alokp): What about BindGraphics()? Move to PPB_Graphics2D. On 2010/12/03 19:27:21, neb wrote: > I don't understand this comment. This renders PPB_Instance::BindGraphics redundant. What should we do about that?
Comments from meeting. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:32: PP_GRAPHICS_3D_ERROR_SUCCESS = 0x3000, We should move these to pp_errors.h and do the EGL mapping in the wrapper library. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:40: enum { This should be a named enum http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:47: struct PPB_Graphics3D_Dev { We should have two different interfaces for Surface and Context. We should defined separate enums for the configs for each of these things. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:54: uint32_t (*GetConfigs)(int32_t* configs, When these return a "real" PP error, these should all be int32_t. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:59: uint32_t (*ChooseConfig)(const int32_t* attrib_list, We're thinking there will be one function to get all configs that will return all the configs and attributes at once, so we can avoid IPC traffic and the extra complexity. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:68: int32_t (*QueryString)(int32_t name, const char** str); As discussed, this will be: PP_Var (*QueryString)(enum SomeEnumName name); and return a VARTYPE_UNDEFINED on an invalid name. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:78: const int32_t* attrib_list, I'd rather be explicit about the size and add a PP_Size argument. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:101: uint32_t (*BindContext)(PP_Resource draw, This should probably be called SetSurfaces http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:104: There was talk about adding a fence callback. http://codereview.chromium.org/5567004/diff/11001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:112: uint32_t (*SwapBuffers)(PP_Resource surface); This should take a completion callback to optionally have a blocking call or an asynchronous call (from the main thread).
Addressed most of the comments from API review.
http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_context_3d_... File ppapi/c/dev/ppb_context_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_context_3d_... ppapi/c/dev/ppb_context_3d_dev.h:17: int32_t (*Create)(PP_Module module, Generally we put this function first and IsXXX second. This function should return a PP_Resource or NULL on failure rather than have an out param to match everything else. Can you also be sure to provide documentation for each function? In particular: what is share_context, and what is the format of attrib_list? http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:40: int32_t (*GetConfigs)(int32_t* configs, Please provide documentation for all of these functions & args. http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_surface_3d_... File ppapi/c/dev/ppb_surface_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/27001/ppapi/c/dev/ppb_surface_3d_... ppapi/c/dev/ppb_surface_3d_dev.h:17: int32_t (*Create)(PP_Module module, Create should be first & return a PP_Resource. Please provide documentation.
Getting close! Added plenty of documentation (mostly copied from EGL spec). Please review. Please pay extra attention to TODOs. They should be read as "ask for suggestions". I am not too happy with the class and enum names. They are quite lengthy and do not tie the whole Graphics3D package together. I have an idea of replacing PP_Graphics3D prefix with PP_GL that I have documented in one of the TODOs.
This is looking very nice, just some minor comments/questions. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... File ppapi/c/dev/pp_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:17: PP_GRAPHICS3DERROR_BAD_CONTEXT = 0x3006, I'm a little confused about how this overlaps with PP_ERROR_BADRESOURCE which seems to be what it means in the places I saw. I guess I would expect that the return value here is a PP_Error with some extra possibilities for 3D. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:110: // PP_GRAPHICS3DATTRIB_CONFIG_CAVEAT values. Normally we would put a blank line before a comment associated with some line. I think this would especially help this enum since you have groups of enum values that would be better separated. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_context_3d_... File ppapi/c/dev/ppb_context_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_context_3d_... ppapi/c/dev/ppb_context_3d_dev.h:62: // - PP_GRAPHICS3DATTRIB_CONTEXT_CLIENT_TYPE: returns the type of client API The documentation here is great. I think it would be better to add the documentation where the enum is defined rather than on these functions. Then on the functions, specify which subset of attributes it can take. This will also save you from describing them twice (set and get attrib on surface 3D have some duplicated descriptions). http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:40: // TODO(alokp): Do these functions need module argument. Actually, I suspect these will need an Instance argument since it needs to talk to the GPU process which will be per-page. Personally, I'm fine waiting to see what the implementation ends up needing.
Thanks Brett. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... File ppapi/c/dev/pp_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:17: PP_GRAPHICS3DERROR_BAD_CONTEXT = 0x3006, On 2010/12/10 00:32:27, brettw wrote: > I'm a little confused about how this overlaps with PP_ERROR_BADRESOURCE which > seems to be what it means in the places I saw. I guess I would expect that the > return value here is a PP_Error with some extra possibilities for 3D. This is equivalent to PP_ERROR_BADRESOURCE except in cases where you have two or more resources. EGL indicates exactly which resource is bad, so these error codes are need to properly implement EGL. But it might be possible to resolve this in EGL layer itself, in which case I will remove these error codes from here. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:110: // PP_GRAPHICS3DATTRIB_CONFIG_CAVEAT values. On 2010/12/10 00:32:27, brettw wrote: > Normally we would put a blank line before a comment associated with some line. I > think this would especially help this enum since you have groups of enum values > that would be better separated. Done. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_context_3d_... File ppapi/c/dev/ppb_context_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_context_3d_... ppapi/c/dev/ppb_context_3d_dev.h:62: // - PP_GRAPHICS3DATTRIB_CONTEXT_CLIENT_TYPE: returns the type of client API On 2010/12/10 00:32:27, brettw wrote: > The documentation here is great. I think it would be better to add the > documentation where the enum is defined rather than on these functions. Then on > the functions, specify which subset of attributes it can take. This will also > save you from describing them twice (set and get attrib on surface 3D have some > duplicated descriptions). Done. http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/50001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:40: // TODO(alokp): Do these functions need module argument. On 2010/12/10 00:32:27, brettw wrote: > Actually, I suspect these will need an Instance argument since it needs to talk > to the GPU process which will be per-page. Personally, I'm fine waiting to see > what the implementation ends up needing. Yeah lets wait until implementation. http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... File ppapi/c/dev/pp_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:8: // TODO(alokp): Using PP_Graphics3D prefix is making these enum names rather Any suggestion on this? http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... File ppapi/c/dev/ppb_surface_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... ppapi/c/dev/ppb_surface_3d_dev.h:34: // TODO(alokp): Need a way to return specific error codes. Would a fourth Any suggestion on this?
LGTM, we can continue to refine this as it's implemented. http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... File ppapi/c/dev/pp_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... ppapi/c/dev/pp_graphics_3d_dev.h:8: // TODO(alokp): Using PP_Graphics3D prefix is making these enum names rather I kind of like the current (long) way since I think overall API consistency is important. http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... File ppapi/c/dev/ppb_surface_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... ppapi/c/dev/ppb_surface_3d_dev.h:34: // TODO(alokp): Need a way to return specific error codes. Would a fourth What types of error codes would you want to return? I'm a bit suspicious of all the different error codes since there's nothing that a plugin can realistically do with the different errors. I think we should focus on better logging (I want to output error messages to the console but this isn't implemented yet) and have a simpler interface.
ACK On 2010/12/10 17:39:11, brettw wrote: > LGTM, we can continue to refine this as it's implemented. > > http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... > File ppapi/c/dev/pp_graphics_3d_dev.h (right): > > http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/pp_graphics_3d_... > ppapi/c/dev/pp_graphics_3d_dev.h:8: // TODO(alokp): Using PP_Graphics3D prefix > is making these enum names rather > I kind of like the current (long) way since I think overall API consistency is > important. > > http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... > File ppapi/c/dev/ppb_surface_3d_dev.h (right): > > http://codereview.chromium.org/5567004/diff/55001/ppapi/c/dev/ppb_surface_3d_... > ppapi/c/dev/ppb_surface_3d_dev.h:34: // TODO(alokp): Need a way to return > specific error codes. Would a fourth > What types of error codes would you want to return? I'm a bit suspicious of all > the different error codes since there's nothing that a plugin can realistically > do with the different errors. I think we should focus on better logging (I want > to output error messages to the console but this isn't implemented yet) and have > a simpler interface.
Used PP_Instance instead of PP_Module in all Create functions.
Still LGTM http://codereview.chromium.org/5567004/diff/71001/ppapi/c/dev/ppb_graphics_3d... File ppapi/c/dev/ppb_graphics_3d_dev.h (right): http://codereview.chromium.org/5567004/diff/71001/ppapi/c/dev/ppb_graphics_3d... ppapi/c/dev/ppb_graphics_3d_dev.h:87: // - PP_GRAPHICS3DSTRING_CLIENT_APIS: describes which client rendering APIs are Line too long :)
The new API has been submitted in chunks: http://codereview.chromium.org/5927002 http://codereview.chromium.org/6062003 http://codereview.chromium.org/6047008 Closing this one. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
