Issue 6400007: Implement proxy for 3d-related interfaces

Issue 6400007: Implement proxy for 3d-related interfaces (Closed)

Created:
9 years, 10 months ago by piman

Modified:
9 years, 7 months ago

Reviewers:
brettw

CC:
chromium-reviews, apatrick_chromium

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Implement proxy for 3d-related interfaces BUG=none TEST=Pepper Flash Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73472

Patch Set 2 : separate ParamTraits<gpu::CommandBuffer::State> into own library #

Total comments: 24

Patch Set 4 : rebase #

Created: 9 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+2501 lines, -47 lines)			Patch
M	chrome/chrome_common.gypi	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/gpu_messages.cc	View	1	1 chunk	+0 lines, -30 lines	0 comments	Download
M	chrome/common/gpu_param_traits.h	View	1	2 chunks	+1 line, -8 lines	0 comments	Download
M	gpu/command_buffer/build_gles2_cmd_buffer.py	View	1 2 3	5 chunks	+88 lines, -4 lines	0 comments	Download
M	gpu/gpu.gyp	View	1 2 3	1 chunk	+15 lines, -0 lines	0 comments	Download
A	gpu/ipc/DEPS	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
A	gpu/ipc/gpu_command_buffer_traits.h	View	1	1 chunk	+23 lines, -0 lines	0 comments	Download
A	gpu/ipc/gpu_command_buffer_traits.cc	View	1	1 chunk	+39 lines, -0 lines	0 comments	Download
M	ppapi/ppapi_shared_proxy.gypi	View	1	3 chunks	+9 lines, -0 lines	0 comments	Download
M	ppapi/proxy/DEPS	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/proxy/dispatcher.cc	View		5 chunks	+19 lines, -3 lines	0 comments	Download
M	ppapi/proxy/interface_id.h	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	ppapi/proxy/interface_proxy.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ppapi/proxy/plugin_dispatcher.h	View	1 2	4 chunks	+19 lines, -0 lines	0 comments	Download
M	ppapi/proxy/plugin_dispatcher.cc	View		1 chunk	+15 lines, -0 lines	0 comments	Download
M	ppapi/proxy/plugin_resource.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	ppapi/proxy/ppapi_messages_internal.h	View	1	5 chunks	+65 lines, -1 line	0 comments	Download
A	ppapi/proxy/ppb_context_3d_proxy.h	View	1 2	1 chunk	+120 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_context_3d_proxy.cc	View	1 2	1 chunk	+584 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_gles_chromium_texture_mapping_proxy.h	View	1 2	1 chunk	+36 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_gles_chromium_texture_mapping_proxy.cc	View		1 chunk	+70 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_opengles2_proxy.h	View		1 chunk	+35 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_opengles2_proxy.cc	View		1 chunk	+1037 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_surface_3d_proxy.h	View	1 2	1 chunk	+100 lines, -0 lines	0 comments	Download
A	ppapi/proxy/ppb_surface_3d_proxy.cc	View	1 2 3	1 chunk	+196 lines, -0 lines	0 comments	Download
M	ppapi/proxy/ppp_instance_proxy.cc	View		3 chunks	+18 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

brettw

http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/plugin_dispatcher.h File ppapi/proxy/plugin_dispatcher.h (right): http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/plugin_dispatcher.h#newcode70 ppapi/proxy/plugin_dispatcher.h:70: void DidCreateInstance(PP_Instance); Normally we would name the parameter here, ...

9 years, 10 months ago (2011-01-31 18:51:29 UTC) #2

piman

9 years, 10 months ago (2011-01-31 19:34:28 UTC) #3

piman

After our discussion, LGTY ? Antoine On Mon, Jan 31, 2011 at 11:34 AM, <piman@chromium.org> ...

9 years, 10 months ago (2011-02-02 02:30:54 UTC) #4

After our discussion, LGTY ?

Antoine

On Mon, Jan 31, 2011 at 11:34 AM, <piman@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/plugin_dispatcher.h
> File ppapi/proxy/plugin_dispatcher.h (right):
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/plugin_dispatche...
> ppapi/proxy/plugin_dispatcher.h:70: void DidCreateInstance(PP_Instance);
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Normally we would name the parameter here, I think.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/plugin_dispatche...
> ppapi/proxy/plugin_dispatcher.h:72: InstanceData*
> GetInstanceData(PP_Instance);
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Can you document that this will return NULL for invalid instance IDs?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> File ppapi/proxy/ppb_context_3d_proxy.cc (right):
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:104: if (result == PP_OK) {
> On 2011/01/31 18:51:29, brettw wrote:
>
>> No {} since you didn't use them elsewhere.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:337: base::SharedMemory*
> shared_memory = new base::SharedMemory(handle, false);
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Maybe use a scoped_ptr and .release() it when you assign at the bottom
>>
> of the
>
>> function?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:369: } else {
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Chrome style prefers no else after return.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:376: : PluginResource(resource),
> On 2011/01/31 18:51:29, brettw wrote:
>
>> 2 more spaces.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.cc:578: &shm_size)) {
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Indenting.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> File ppapi/proxy/ppb_context_3d_proxy.h (right):
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.h:20: namespace gpu {
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Can you put blank lines above and below the namespace boundaries here?
>>
> The style
>
>> guide says to do that but sometimes we don't for forward declarations
>>
> when
>
>> there's only a few items. This group of stuff is getting hard to
>>
> follow so we
>
>> should follow the rule.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.h:31: class Surface3D;
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Can you put a blank line between these?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_context_3d_p...
> ppapi/proxy/ppb_context_3d_proxy.h:42: void get_surfaces(Surface3D**
> draw, Surface3D** read) const {
> On 2011/01/31 18:51:29, brettw wrote:
>
>> This would be more "normal" to my eye if there were two separate
>>
> getters for
>
>> draw and read. The one place you call this could also get one line
>>
> shorter.
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_gles_chromiu...
> File ppapi/proxy/ppb_gles_chromium_texture_mapping_proxy.h (right):
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_gles_chromiu...
> ppapi/proxy/ppb_gles_chromium_texture_mapping_proxy.h:32: private:
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Maybe just delete this?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_opengles2_pr...
> File ppapi/proxy/ppb_opengles2_proxy.cc (right):
>
>
>
http://codereview.chromium.org/6400007/diff/3001/ppapi/proxy/ppb_opengles2_pr...
> ppapi/proxy/ppb_opengles2_proxy.cc:23: Context3D* context =
> PluginResource::GetAs<Context3D>(context_id);
> On 2011/01/31 18:51:29, brettw wrote:
>
>> Should these functions be checking the validity of the resource? I'm
>>
> concerned
>
>> that we might start using this proxy code in a more trusted context in
>>
> Native
>
>> Client and crashes in this layer could be more privileged.
>>
>
> Unless you really feel strongly about it I'd rather not do it, and leave
> the responsibility to the plugin.
> Security-wise, it's not an issue, this code would run on the untrusted
> side. in NaCl, and can't cause privilege escalation.
>
> The reasons I'd rather not add extra checks are 2-fold:
> - these are all high-frequency calls (expect 10k to 100k per frame in
> high-end graphics applications - e.g. games - and that can't take more
> than a few ms). We need to aim at making them as efficient as possible.
> The map lookup bothers me and I'm planning on caching it on the TLS or
> something. We could adding DCHECKs for invalid usage if you think that's
> useful for debugging.
> - All of this is currently auto-generated for easy maintenance. I would
> hate to lose that for adding custom early return (because some of these
> functions need to return actual parameters).
>
> Once I add TLS caching or something, we could add a stub NOOP
> GLESImplementation for the case where a bad context was passed. Not that
> it would only shift the problem, because we can't return useful values
> to Get() calls and the caller would then easily have invalid behavior.
>
>
> http://codereview.chromium.org/6400007/
>

Yes, sorry. LGTM

Expand Messages | Collapse Messages