Bug 68905 – [chromium] Add a setVisibility method to WebGraphicsContext3D.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michal Mocny]

[chromium][Not for review] Add a setVisibility method to WebGraphicsContext3D.

[Michal Mocny]

Created attachment 108852 [details]
Patch

[Michal Mocny]

Created attachment 111025 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 111025 [details]
Patch

Attachment 111025 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10066573

[Michal Mocny]

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.

[Kenneth Russell]

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.

[Jonathan Backer]

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?).

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> Please wait for approval from [e-mail] 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.

[Kenneth Russell]

(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.

[Michal Mocny]

(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

[Kenneth Russell]

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

[WebKit Review Bot]

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

[Kenneth Russell]

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.

[Michal Mocny]

Created attachment 111812 [details]
Patch

[Kenneth Russell]

Comment on attachment 111812 [details]
Patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 111812 [details]
Patch

Clearing flags on attachment: 111812

Committed r98036: <http://trac.webkit.org/changeset/98036>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.