Implemented offscreen rendering path for GLES2CmdDecoder on Linux....

Implemented offscreen rendering path for GLES2CmdDecoder on Linux.
With these changes, WebGL runs in the sandbox on Linux.

BUG=29120
TEST=ran WebGL demos in sandbox on Linux


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

[Ken Russell (switch to Gerrit), 2010-03-30 02:04:16]

Please review. Thanks.

[piman, 2010-03-30 02:17:23]

LGTM. Awesome !

[apatrick, 2010-03-30 17:36:18]

lgtm

http://codereview.chromium.org/1540004/diff/1/5
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/1/5#newcode55
gpu/command_buffer/service/x_utils.cc:55: static bool InitializeGLEW() {
I already did this for windows in gles2_cmd_decoder.cc. Maybe they can share the
logic for one-time initialization of glew?

http://codereview.chromium.org/1540004/diff/1/5#newcode201
gpu/command_buffer/service/x_utils.cc:201: return
GLXContextWrapper::Initialize();
If this call fails, what destroys the pbuffer and the context? It might be safer
just to always call Destroy() when something fails and make Destroy robust to
partial initialization.

http://codereview.chromium.org/1540004/diff/1/6
File gpu/command_buffer/service/x_utils.h (right):

http://codereview.chromium.org/1540004/diff/1/6#newcode51
gpu/command_buffer/service/x_utils.h:51: Display *display_;
protected member variables are only allowed in unit test fixtures.

[Ken Russell (switch to Gerrit), 2010-03-30 18:34:09]

http://codereview.chromium.org/1540004/diff/1/5
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/1/5#newcode55
gpu/command_buffer/service/x_utils.cc:55: static bool InitializeGLEW() {
On 2010/03/30 17:36:19, apatrick wrote:
> I already did this for windows in gles2_cmd_decoder.cc. Maybe they can share
the
> logic for one-time initialization of glew?

Looking at the code this refactoring will be non-trivial. I've filed
http://crbug.com/39864 to track it.

http://codereview.chromium.org/1540004/diff/1/5#newcode201
gpu/command_buffer/service/x_utils.cc:201: return
GLXContextWrapper::Initialize();
On 2010/03/30 17:36:19, apatrick wrote:
> If this call fails, what destroys the pbuffer and the context? It might be
safer
> just to always call Destroy() when something fails and make Destroy robust to
> partial initialization.

Good point and suggestion; done.

http://codereview.chromium.org/1540004/diff/1/6
File gpu/command_buffer/service/x_utils.h (right):

http://codereview.chromium.org/1540004/diff/1/6#newcode51
gpu/command_buffer/service/x_utils.h:51: Display *display_;
On 2010/03/30 17:36:19, apatrick wrote:
> protected member variables are only allowed in unit test fixtures.

Made private and added necessary accessors.

[brettw, 2010-03-30 19:24:03]

http://codereview.chromium.org/1540004/diff/20001/21001
File gpu/command_buffer/service/gpu_processor_linux.cc (right):

http://codereview.chromium.org/1540004/diff/20001/21001#newcode38
gpu/command_buffer/service/gpu_processor_linux.cc:38: // Offscreen code path
Check that your two comments here end with periods.

http://codereview.chromium.org/1540004/diff/20001/21001#newcode61
gpu/command_buffer/service/gpu_processor_linux.cc:61: GLXContextWrapper *context
= decoder_->context();
Can the decoder manage deleting the context wrapper via a scoped_ptr? Can the
decoder's destructor automatically call Destroy()? Ideally this could be
expressed as decoder_.reset() and everything would be torn down properly.

http://codereview.chromium.org/1540004/diff/20001/21004
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/20001/21004#newcode176
gpu/command_buffer/service/x_utils.cc:176: GLXFBConfig* config =
glXChooseFBConfig(GetDisplay(), 0, config_attributes,
chrome/gpu/x_util.h has a nice ScopedPtrXFree which I think would be very
helpful here. Ideally, this would be in a base or something where you could use
it from src/gpu. I'm fine just copying it to this file for now (it's only one
line). Then you can delete all of the XFree's later in this function.

[Ken Russell (switch to Gerrit), 2010-03-30 21:28:32]

http://codereview.chromium.org/1540004/diff/20001/21001
File gpu/command_buffer/service/gpu_processor_linux.cc (right):

http://codereview.chromium.org/1540004/diff/20001/21001#newcode38
gpu/command_buffer/service/gpu_processor_linux.cc:38: // Offscreen code path
On 2010/03/30 19:24:03, brettw wrote:
> Check that your two comments here end with periods.

Fixed.

http://codereview.chromium.org/1540004/diff/20001/21001#newcode61
gpu/command_buffer/service/gpu_processor_linux.cc:61: GLXContextWrapper *context
= decoder_->context();
On 2010/03/30 19:24:03, brettw wrote:
> Can the decoder manage deleting the context wrapper via a scoped_ptr? Can the
> decoder's destructor automatically call Destroy()? Ideally this could be
> expressed as decoder_.reset() and everything would be torn down properly.

This would require a large refactoring of the service side command buffer code
on all platforms. It's beyond the scope of this CL.

http://codereview.chromium.org/1540004/diff/20001/21004
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/20001/21004#newcode176
gpu/command_buffer/service/x_utils.cc:176: GLXFBConfig* config =
glXChooseFBConfig(GetDisplay(), 0, config_attributes,
On 2010/03/30 19:24:03, brettw wrote:
> chrome/gpu/x_util.h has a nice ScopedPtrXFree which I think would be very
> helpful here. Ideally, this would be in a base or something where you could
use
> it from src/gpu. I'm fine just copying it to this file for now (it's only one
> line). Then you can delete all of the XFree's later in this function.

Thanks, this is a good suggestion. Done.

[brettw, 2010-03-30 21:33:52]

LGTM

Can we file a bug on cleaning up the memory management, and can you put a TODO
in your code referencing the bug? The current state seems pretty silly to me.

http://codereview.chromium.org/1540004/diff/29001/30004
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/29001/30004#newcode132
gpu/command_buffer/service/x_utils.cc:132: if (GetContext()) break;
Chrome basically never uses this single-line form. Can you put this on two
lines?

[Ken Russell (switch to Gerrit), 2010-03-30 21:43:38]

On 2010/03/30 21:33:52, brettw wrote:
> Can we file a bug on cleaning up the memory management, and can you put a TODO
> in your code referencing the bug? The current state seems pretty silly to me.

Done. http://crbug.com/39895

http://codereview.chromium.org/1540004/diff/29001/30004
File gpu/command_buffer/service/x_utils.cc (right):

http://codereview.chromium.org/1540004/diff/29001/30004#newcode132
gpu/command_buffer/service/x_utils.cc:132: if (GetContext()) break;
On 2010/03/30 21:33:52, brettw wrote:
> Chrome basically never uses this single-line form. Can you put this on two
> lines?

Done.