Chromium implementation of discardBackbuffer WebGraphicsContext3D extension.

Chromium implementation of discardBackbuffer WebGraphicsContext3D extension.

Webkit side patch: https://bugs.webkit.org/show_bug.cgi?id=81383

BUG=116049
TEST=Manual

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

[mmocny, 2012-03-16 17:50:52]

This is just a bare minimum patch before webkit side can land.

Implementation of these two methods will be filled in once the gles2 commands
are implemented.

[apatrick_chromium, 2012-03-16 18:07:55]

Rather than a chromium specific extension, why not implement
EXT_discard_framebuffer?

http://www.khronos.org/registry/gles/extensions/EXT/EXT_discard_framebuffer.txt

It could discard the back buffer if the specified FBO is zero and all of
COLOR_EXT, DEPTH_EXT and STENCIL_EXT are specified and do nothing otherwise.

[mmocny, 2012-03-16 18:15:29]

Sure, I can name it that way.

But it would temporarily differ from that extension in that it won't yet
auto-recreate itself upon first use.

I can remove the second explicit call to recreate once that is implemented.

[apatrick_chromium, 2012-03-16 19:37:54]

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:446:
virtual void discardFramebufferEXT();
This one takes some arguments.

https://chromiumcodereview.appspot.com/9699125/diff/3002/gpu/command_buffer/s...
File gpu/command_buffer/service/feature_info.cc (right):

https://chromiumcodereview.appspot.com/9699125/diff/3002/gpu/command_buffer/s...
gpu/command_buffer/service/feature_info.cc:121:
AddExtensionString("GL_EXT_discard_framebuffer");
Best not to advertise it until it's finished. I think Pepper will see the
extension is available if you do this.

[mmocny, 2012-03-16 20:25:20]

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:446:
virtual void discardFramebufferEXT();
How closely should I duplicate the original arguments?

I assume target will simply be "WGC3Denum target", but I am not sure how best to
implement the attachements argument:

The only reference to values "COLOR_EXT" etc in our codebase is defined in
ANGLE, and WebGraphicsContext3D seems to use an Attributes struct of which
contains bools for these concepts.

Should I use an array of enum values and define these values somewhere, or use
something higher level?

On 2012/03/16 19:37:56, apatrick_chromium wrote:
> This one takes some arguments.

[apatrick_chromium, 2012-03-16 21:08:19]

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://chromiumcodereview.appspot.com/9699125/diff/3002/content/common/gpu/c...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:446:
virtual void discardFramebufferEXT();
I think for the purposes of this patch, just make the signature:

void discardFramebufferEXT(WGC3Denum target, WGC3Dsizei numAttachments, const
WGC3Denum *attachments);

The constants are not used anywhere yet. But I think the appropriate place to
put them would be WebKit/Source/WebCore/platform/graphics/Extensions3D.h or
WebKit/Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h. I don't
do a lot with WebKit though. You might want to check with someone more familiar
with it.

[mmocny, 2012-03-21 20:13:52]

This is ready for another round of review, though it will need to wait for
https://bugs.webkit.org/show_bug.cgi?id=81823 to land+roll.

I've tested both GLX and ui-use-gpu-process and thumbnailing in background
finally works fine.

This is a fix tailored for the currently known issues and not for all possible
abuses, though it is a step in the right direction.

I am tracking down an issue with rapid tab switching, though the error likely
lays either in the webkit patch, or is an IPC race.  If anyone spots something
potentially racy, it will help.

I have not yet done vram benchmarks to prove memory usage is back to old
figures.

https://chromiumcodereview.appspot.com/9699125/diff/17001/gpu/command_buffer/...
File gpu/command_buffer/service/feature_info.cc (right):

https://chromiumcodereview.appspot.com/9699125/diff/17001/gpu/command_buffer/...
gpu/command_buffer/service/feature_info.cc:121:
AddExtensionString("GL_EXT_discard_framebuffer");
Since we aren't following the extension precisely as described, should I name
this differently for now?

[apatrick_chromium, 2012-03-22 21:03:49]

LGTM for my part. You will need one of piman or backer for ui/gfx/compositor.

https://chromiumcodereview.appspot.com/9699125/diff/17001/gpu/command_buffer/...
File gpu/command_buffer/service/feature_info.cc (right):

https://chromiumcodereview.appspot.com/9699125/diff/17001/gpu/command_buffer/...
gpu/command_buffer/service/feature_info.cc:121:
AddExtensionString("GL_EXT_discard_framebuffer");
On 2012/03/21 20:13:52, mmocny wrote:
> Since we aren't following the extension precisely as described, should I name
> this differently for now?

Maybe CHROMIUM_discard_framebuffer for now. Also, since gles2_cmd_decoder.cc
does not actually implement this extension, maybe it should be advertised
somewhere higher up the stack? Although I see a bunch of other one's that might
already be in the same boat. I'm happy to go with rename or don't advertise at
all for now if you want.

[jonathan.backer, 2012-03-22 21:05:53]

On 2012/03/22 21:03:49, apatrick_chromium wrote:
> LGTM for my part. You will need one of piman or backer for ui/gfx/compositor.

LGTM for ui/gfx/compositor

[commit-bot: I haz the power, 2012-03-23 14:38:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/9699125/21002

[commit-bot: I haz the power, 2012-03-23 16:17:42]

Change committed as 128501