Bug 68905

Summary: [chromium] Add a setVisibility method to WebGraphicsContext3D.
Product: WebKit Reporter: Michal Mocny <mmocny>
Component: New BugsAssignee: Michal Mocny <mmocny>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, dglazkov, fishd, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Michal Mocny 2011-09-27 08:41:58 PDT
[chromium][Not for review] Add a setVisibility method to WebGraphicsContext3D.
Comment 1 Michal Mocny 2011-09-27 08:45:23 PDT
Created attachment 108852 [details]
Patch
Comment 2 Michal Mocny 2011-10-14 10:07:30 PDT
Created attachment 111025 [details]
Patch
Comment 3 WebKit Review Bot 2011-10-14 10:11:01 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 WebKit Review Bot 2011-10-14 10:12:32 PDT
Comment on attachment 111025 [details]
Patch

Attachment 111025 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10066573
Comment 5 Michal Mocny 2011-10-14 10:36:21 PDT
Kenneth, it looked like you were a good reviewer for this patch, but if someone else is more appropriate, please let me know (and sorry to bother).

This patch will give WebGraphicsContext3D implementations the opportunity to release some needless graphics resources.
Comment 6 Kenneth Russell 2011-10-14 11:17:09 PDT
Comment on attachment 111025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111025&action=review

> Source/WebKit/chromium/public/WebGraphicsContext3D.h:144
> +    virtual void setVisibility(bool visible) = 0;

It seems clear that this is only useful for the compositor's context. What resources are released when called with argument "false"? Would it make more sense to just destroy the context and recreate it from scratch when visibility becomes true again? Without knowing more, it seems to me that any releasing of resources would be better done at the WebKit level.

Also, you would need to either temporarily provide an empty body for this or land all of the Chromium side implementations first (and roll Source/WebKit/chromium/DEPS) in order to fix the pure virtual compilation problem on the EWS bots.
Comment 7 Jonathan Backer 2011-10-14 11:28:12 PDT
I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?).
Comment 8 Darin Fisher (:fishd, Google) 2011-10-14 12:37:57 PDT
(In reply to comment #3)
> Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

I defer to Ken Russell for this change.  I don't need to review it further.
Comment 9 Kenneth Russell 2011-10-14 12:39:39 PDT
(In reply to comment #7)
> I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?).

The signaling in the current patch is compositor specific so the intent wasn't clear.

The direction WebGL is going is to move the back buffer handling up to WebKit. See bug 53201. This means that any discarding of secondary buffers could be done in common code without plumbing anything through the WebGraphicsContext3D interface. The issue of secondary buffers doesn't really become relevant for WebGL until threaded compositing is in place though.

I'd still like to see the Chromium side patch that uses this signal.
Comment 10 Michal Mocny 2011-10-14 12:56:53 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > I'm not sure that I understand the comment about being compositor specific. One could imagine other contexts (like Pepper3D or WebGL) wanting to release some resources if they aren't visible. E.g. AFAIK for WebGL, we double buffer; we could release one of those buffers in the GLES2Decoder if we had the appropriate signals plumbed (perhaps we'd only want to do that when the compositor context is not visible?).
> 
> The signaling in the current patch is compositor specific so the intent wasn't clear.
> 
> The direction WebGL is going is to move the back buffer handling up to WebKit. See bug 53201. This means that any discarding of secondary buffers could be done in common code without plumbing anything through the WebGraphicsContext3D interface. The issue of secondary buffers doesn't really become relevant for WebGL until threaded compositing is in place though.
> 
> I'd still like to see the Chromium side patch that uses this signal.

The Chromium patch is: http://codereview.chromium.org/7890046
Comment 11 Kenneth Russell 2011-10-18 13:22:46 PDT
Comment on attachment 111025 [details]
Patch

OK, this seems fine at least as an interim solution. Source/WebKit/chromium/DEPS has since been rolled forward, so I think this will pass the commit queue now. r=me
Comment 12 WebKit Review Bot 2011-10-18 14:27:35 PDT
Comment on attachment 111025 [details]
Patch

Rejecting attachment 111025 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pport/Source/WebKit/chromium/webkit/support/test_webkit_platform_support.o] Error 1
make: *** Waiting for unfinished jobs....

Failed to run "['Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
c/WebGraphicsContext3D.h:144: note: 	virtual void WebKit::WebGraphicsContext3D::setVisibility(bool)
make: *** [out/Debug/obj.target/webkit_support/Source/WebKit/chromium/webkit/support/test_webkit_platform_support.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10142031
Comment 13 Kenneth Russell 2011-10-18 14:36:38 PDT
Michal, you'll have to add the new virtual to src/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc / .h downstream in Chromium, and once that's committed, roll forward chromium_rev in Source/WebKit/chromium/DEPS.
Comment 14 Michal Mocny 2011-10-20 11:40:17 PDT
Created attachment 111812 [details]
Patch
Comment 15 Kenneth Russell 2011-10-20 13:09:11 PDT
Comment on attachment 111812 [details]
Patch

Looks good to me.
Comment 16 WebKit Review Bot 2011-10-20 16:22:19 PDT
Comment on attachment 111812 [details]
Patch

Clearing flags on attachment: 111812

Committed r98036: <http://trac.webkit.org/changeset/98036>
Comment 17 WebKit Review Bot 2011-10-20 16:22:24 PDT
All reviewed patches have been landed.  Closing bug.