Add basic GL functionality to the passthrough command buffer.

Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/851a481ac9a8ef1d0f1d8ccf693d5d735d209b7a
Cr-Commit-Position: refs/heads/master@{#418637}

[Geoff Lang, 2016-09-09 17:53:47]

Description was changed from

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
==========

to

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Geoff Lang, 2016-09-09 18:37:34]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 18:38:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:46:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:46:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Geoff Lang, 2016-09-09 19:54:07]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:55:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Geoff Lang, 2016-09-09 20:36:37]

Description was changed from

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Geoff Lang, 2016-09-09 20:36:37]

geofflang@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[Geoff Lang, 2016-09-09 20:37:30]

PTAL

[commit-bot: I haz the power, 2016-09-09 21:21:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 21:21:26]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-09-09 21:22:11]

mostly looks good, but I don't know mailbox code well enough.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/client_service_map.h (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/client_service_map.h:44: } else {
nit: chrome style asks no else here.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/client_service_map.h:59: } else {
Same here, no else

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:280:
DCHECK(false) << "UNIMPLEMENTED";
Use NOTIMPLEMENTED() instead. here and all other places.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:585: // TODO:
Handle this case in ANGLE by adding a WebGL validation mode.
change all TODO to TODO(geofflang)

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:771: //
glGetBufferParameteri64v(target, pname, params);
command buffer is 32bit, so unnecessary.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2224: }  //
anonymous namespace
Can we move this to the top of the file?

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2230:
resources_->program_id_map.GetServiceIDOrReserve(program);
We can't reserve a program id, can we? Should we use GetServiceID()? and DCHECK
here to make sure service_id is valid?

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2243:
glGetProgramiv(service_program, GL_LINK_STATUS, &link_status);
Should we return early if link_status is 0?

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.h:73: TexturePassthrough(GLuint
service_id);
explicit

[Geoff Lang, 2016-09-12 14:21:20]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[Geoff Lang, 2016-09-12 14:21:29]

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/client_service_map.h (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/client_service_map.h:44: } else {
On 2016/09/09 21:22:10, Zhenyao Mo wrote:
> nit: chrome style asks no else here.

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/client_service_map.h:59: } else {
On 2016/09/09 21:22:10, Zhenyao Mo wrote:
> Same here, no else

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:280:
DCHECK(false) << "UNIMPLEMENTED";
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> Use NOTIMPLEMENTED() instead. here and all other places.

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:585: // TODO:
Handle this case in ANGLE by adding a WebGL validation mode.
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> change all TODO to TODO(geofflang)

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:771: //
glGetBufferParameteri64v(target, pname, params);
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> command buffer is 32bit, so unnecessary.

Ok, I'll have to come back to this one, may require making a scratch buffer.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2224: }  //
anonymous namespace
On 2016/09/09 21:22:10, Zhenyao Mo wrote:
> Can we move this to the top of the file?

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2230:
resources_->program_id_map.GetServiceIDOrReserve(program);
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> We can't reserve a program id, can we? Should we use GetServiceID()? and
DCHECK
> here to make sure service_id is valid?

Done.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2243:
glGetProgramiv(service_program, GL_LINK_STATUS, &link_status);
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> Should we return early if link_status is 0?

I think that it's safe to always do the queries.  The current code will write
out everything as long as the program was successfully linked at some point in
the past.

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.h:73: TexturePassthrough(GLuint
service_id);
On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> explicit

Done.

[commit-bot: I haz the power, 2016-09-12 14:21:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 15:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 15:20:06]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2016-09-12 18:15:35]

lgtm with one question answered

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2243:
glGetProgramiv(service_program, GL_LINK_STATUS, &link_status);
On 2016/09/12 14:21:29, Geoff Lang wrote:
> On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> > Should we return early if link_status is 0?
> 
> I think that it's safe to always do the queries.  The current code will write
> out everything as long as the program was successfully linked at some point in
> the past.

You are saying we will only call this function if the program has been linked
successfully linked? Because otherwise we will get a lot of GL errors from all
the queries.  Just double-check.

[piman, 2016-09-13 01:07:32]

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:16:
PassthroughResources::~PassthroughResources() {}
If there are still resources in here, they should be deleted (assuming the
context is current), otherwise they will leak.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:23: if
(id_map->GetServiceID(client_ids[ii], nullptr)) {
use client_id instead of client_ids[ii], to avoid a double-read.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:24: return
error::kInvalidArguments;
Should we delete the service_ids created on l.20?
Should we clean up the mappings that were created in previous iterations of the
loop?

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:104:
glAttachShader(resources_->program_id_map.GetServiceIDOrReserve(program),
Here and other places, I'm not sure about the GetServiceIDOrReserve pattern. I
assume this expects to generate the proper error in ANGLE
(GL_INVALID_OPERATION/GL_INVALID_VALUE) if the client_id is invalid, because of
the made up service id (assuming we don't collide with driver-generated service
ids), but we'll still add tracking info in the map, and then a further attempt
to generate that particular client id will result in a kInvalidArguments
(therefore, lost context), whereas it is expected to succeed. For example:

glCreateProgram();  // clients picks 1. Adds CreateProgram(1) command. Returns 1
glAttachShader(1, 2);  // Adds AttachShader(1, 2) command. Getting ahead of
ourselves here, should raise GL_INVALID_OPERATION
glCreateShader(GL_VERTEX_SHADER);  // client picks 2. Adds
CreateShader(GL_VERTEX_SHADER, 2). Should return 2. However this will cause
kInvalidArguments in DoCreateShader

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:120:
glBindBuffer(target, resources_->buffer_id_map.GetServiceIDOrReserve(buffer));
Note, here in particular (and other glBind*), we expect to fail
(GL_INVALID_OPERATION) if the context was created with bind_generates_resource =
false.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1308:
glReadPixels(x, y, width, height, format, type, pixels);
We need to validate the length against bufsize before doing this, otherwise this
would lead to an out-of-bounds write.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2444:
TexturePassthrough* texture_passthrough = nullptr;
nit: scoped_refptr<TexturePassthrough>

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2469:
TexturePassthrough* texture = nullptr;
nit: scoped_refptr<TexturePassthrough>

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2626: //
CHECK(false) << "UNIMPLEMENTED";
nit: remove commented code

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2662:
CHECK(false) << "UNIMPLEMENTED";
nit: (here and below) just use NOTIMPLEMENTED() - no need to abort.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2684: //
NOTIMPLEMENTED();
nit: remove commented code.

[Geoff Lang, 2016-09-13 16:43:15]

On 2016/09/12 18:15:35, Zhenyao Mo wrote:
> lgtm with one question answered
> 
>
https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
(right):
> 
>
https://codereview.chromium.org/2317363005/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2243:
> glGetProgramiv(service_program, GL_LINK_STATUS, &link_status);
> On 2016/09/12 14:21:29, Geoff Lang wrote:
> > On 2016/09/09 21:22:11, Zhenyao Mo wrote:
> > > Should we return early if link_status is 0?
> > 
> > I think that it's safe to always do the queries.  The current code will
write
> > out everything as long as the program was successfully linked at some point
in
> > the past.
> 
> You are saying we will only call this function if the program has been linked
> successfully linked? Because otherwise we will get a lot of GL errors from all
> the queries.  Just double-check.

I double checked, with the early-exit when the program isn't valid, no GL errors
would be generated due to invalid programs.  As for the queries, querying the
number of attributes and uniforms on an unlinked program is valid, it will
return 0.

[Geoff Lang, 2016-09-13 16:43:30]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 16:43:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Geoff Lang, 2016-09-13 16:51:28]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 16:51:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Geoff Lang, 2016-09-13 17:49:57]

Will upload another patch once I add some unit tests for the ClientServiceMap

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc (right):

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc:16:
PassthroughResources::~PassthroughResources() {}
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> If there are still resources in here, they should be deleted (assuming the
> context is current), otherwise they will leak.

Done, fixed the destruction logic.  asserts are now firing when the
PassthroughTexture object is destroyed when there is no GL context.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:23: if
(id_map->GetServiceID(client_ids[ii], nullptr)) {
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> use client_id instead of client_ids[ii], to avoid a double-read.

Done.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:24: return
error::kInvalidArguments;
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> Should we delete the service_ids created on l.20?
> Should we clean up the mappings that were created in previous iterations of
the
> loop?

I grouped the error check in with the ID binding to avoid copying the whole list
of client ids but it looks like it only complicates things.  Updated to make a
copy, perform the error check and then gen the service ids.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:104:
glAttachShader(resources_->program_id_map.GetServiceIDOrReserve(program),
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> Here and other places, I'm not sure about the GetServiceIDOrReserve pattern. I
> assume this expects to generate the proper error in ANGLE
> (GL_INVALID_OPERATION/GL_INVALID_VALUE) if the client_id is invalid, because
of
> the made up service id (assuming we don't collide with driver-generated
service
> ids), but we'll still add tracking info in the map, and then a further attempt
> to generate that particular client id will result in a kInvalidArguments
> (therefore, lost context), whereas it is expected to succeed. For example:
> 
> glCreateProgram();  // clients picks 1. Adds CreateProgram(1) command. Returns
1
> glAttachShader(1, 2);  // Adds AttachShader(1, 2) command. Getting ahead of
> ourselves here, should raise GL_INVALID_OPERATION
> glCreateShader(GL_VERTEX_SHADER);  // client picks 2. Adds
> CreateShader(GL_VERTEX_SHADER, 2). Should return 2. However this will cause
> kInvalidArguments in DoCreateShader

Good point.  You're right that the goal is to have ANGLE generate all the errors
for invalid IDs passed in but it looks like I need some extra logic to track
when an ID was reserved and have the ability to update it to a real value later.

I ended up keeping two separate lists of ID mappings in ClientServiceMap. 
GetServiceID will never return a reserved ID, they will only be returned when
calling GetServiceIDOrReserve.  When an ID is gen'd it will overwrite the
reserved ID when SetIDMapping is called.

Going to add some testing to verify the behaviour.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:120:
glBindBuffer(target, resources_->buffer_id_map.GetServiceIDOrReserve(buffer));
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> Note, here in particular (and other glBind*), we expect to fail
> (GL_INVALID_OPERATION) if the context was created with bind_generates_resource
=
> false.

Going to add an extension to ANGLE to generate errors when binding would
generate a resource, filed http://anglebug.com/1518

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2444:
TexturePassthrough* texture_passthrough = nullptr;
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> nit: scoped_refptr<TexturePassthrough>

Done.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2469:
TexturePassthrough* texture = nullptr;
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> nit: scoped_refptr<TexturePassthrough>

Done.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2626: //
CHECK(false) << "UNIMPLEMENTED";
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> nit: remove commented code

Done.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2662:
CHECK(false) << "UNIMPLEMENTED";
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> nit: (here and below) just use NOTIMPLEMENTED() - no need to abort.

Done.

https://codereview.chromium.org/2317363005/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:2684: //
NOTIMPLEMENTED();
On 2016/09/13 01:07:32, piman OOO back 2016-09-12 wrote:
> nit: remove commented code.

Done.

[commit-bot: I haz the power, 2016-09-13 18:18:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 18:18:11]

Dry run: This issue passed the CQ dry run.

[piman, 2016-09-13 21:16:50]

https://codereview.chromium.org/2317363005/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:24: }
There's one other condition that you may want to consider, that client_ids might
contain the same id twice, which could cause trouble (in particular it will
trigger the DCHECK in SetIDMapping).

See what we do in e.g. GLES2DecoderImpl::HandleGenTexturesImmediate, in
particular CheckUniqueAndNonNullIds (we also check against 0, but in your case
that's already covered by GetServiceID).

[Geoff Lang, 2016-09-14 17:38:18]

The CQ bit was checked by geofflang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:38:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Geoff Lang, 2016-09-14 17:43:37]

I updated the logic for handling binding of non-gen'd objects.  For objects
capable of being created on bind, a new service ID will be gen'd on demand. 
Otherwise, the same invalid object ID will be returned.

To fully support this, I have to add an extension to ANGLE for disabling
creation on bind for these object types and make sure that this 'invalid' object
ID will never be gen'd by ANGLE.

https://codereview.chromium.org/2317363005/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right):

https://codereview.chromium.org/2317363005/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:24: }
On 2016/09/13 21:16:50, piman OOO back 2016-09-12 wrote:
> There's one other condition that you may want to consider, that client_ids
might
> contain the same id twice, which could cause trouble (in particular it will
> trigger the DCHECK in SetIDMapping).
> 
> See what we do in e.g. GLES2DecoderImpl::HandleGenTexturesImmediate, in
> particular CheckUniqueAndNonNullIds (we also check against 0, but in your case
> that's already covered by GetServiceID).

Done.

[piman, 2016-09-14 18:22:45]

LGTM, thanks for working through this!

[commit-bot: I haz the power, 2016-09-14 18:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 18:44:51]

Dry run: This issue passed the CQ dry run.

[Geoff Lang, 2016-09-14 19:12:13]

On 2016/09/14 18:22:45, piman OOO back 2016-09-12 wrote:
> LGTM, thanks for working through this!

Thanks for the review and thinking through the bind/gen issues with me

[Geoff Lang, 2016-09-14 19:12:23]

The CQ bit was checked by geofflang@chromium.org

[Geoff Lang, 2016-09-14 19:12:24]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2317363005/#ps100001
(title: "Handle bind_generates_resource")

[commit-bot: I haz the power, 2016-09-14 19:14:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 19:21:00]

Description was changed from

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-14 19:21:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-14 19:22:23]

Description was changed from

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add basic GL functionality to the passthrough command buffer.

With this patch, the passthrough command buffer is capable of rendering chrome
and basic WebGL content.

BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/851a481ac9a8ef1d0f1d8ccf693d5d735d209b7a
Cr-Commit-Position: refs/heads/master@{#418637}
==========

[commit-bot: I haz the power, 2016-09-14 19:22:24]

Patchset 6 (id:??) landed as
https://crrev.com/851a481ac9a8ef1d0f1d8ccf693d5d735d209b7a
Cr-Commit-Position: refs/heads/master@{#418637}