ui: Enable virtual contexts for linux

ui: Enable virtual contexts for linux

Use windows instead of pbuffers on linux to force
compatibility for virtual contexts.

BUG=457431
Committed: https://crrev.com/55e588cad550a65f2999255dfccea7db9b696a73
Cr-Commit-Position: refs/heads/master@{#327845}

[hendrikw, 2015-04-17 17:02:26]

hendrikw@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[hendrikw, 2015-04-17 17:08:34]

hendrikw@chromium.org changed reviewers:
+ vmiura@chromium.org

[hendrikw, 2015-04-17 17:08:35]

Hi, here's the virtual context change, using John's idea of replacing the
pbuffers with unmapped windows.  This should cause compatibility in all cases. 
Not being a linux guys,I'm not completely sure of the implications.

As I mentioned in the bug, this does improve performance the few pages I tried
with it.

vmiura@, could you try it out on your machine?  You mentioned that the previous
patch, you have to change 32->24 to make it work on your system, so I know
you're window manager differs from mine.

I have a few TODO's in the code, if any of you can provide answers for them,
that would help.

piman@, do you know what window managers I should try in order to test the
slowdown you mentioned you saw?

[piman, 2015-04-17 18:01:27]

OK, I buy this, LGTM.

Let's keep an eye on bugs, I could see making a context current on a window that
was never mapped as possibly iffy in some drivers (though I'm pretty sure spec
allows it).

https://codereview.chromium.org/1081533003/diff/20001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/1081533003/diff/20001/ui/gl/gl_surface_glx.cc...
ui/gl/gl_surface_glx.cc:602: // See https://crbug.com/326995.
nit: that comment doesn't really apply there (we don't swap). I guess we can say
we create a window with CopyFromParent visual so that we have the same visual as
for NativeViewGLSurfaceGLX (i.e. same GLXFBConfig), to ensure contexts are
compatible and can be made current with either.

https://codereview.chromium.org/1081533003/diff/20001/ui/gl/gl_surface_glx.cc...
ui/gl/gl_surface_glx.cc:605: // is using CopylFromParent still the best setting?
 If so, why?
yes, see above :)

[hendrikw, 2015-04-17 18:59:46]

https://codereview.chromium.org/1081533003/diff/20001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/1081533003/diff/20001/ui/gl/gl_surface_glx.cc...
ui/gl/gl_surface_glx.cc:602: // See https://crbug.com/326995.
On 2015/04/17 18:01:27, piman (Very slow to review) wrote:
> nit: that comment doesn't really apply there (we don't swap). I guess we can
say
> we create a window with CopyFromParent visual so that we have the same visual
as
> for NativeViewGLSurfaceGLX (i.e. same GLXFBConfig), to ensure contexts are
> compatible and can be made current with either.

Exactly what I needed, thanks.

https://codereview.chromium.org/1081533003/diff/80001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/1081533003/diff/80001/ui/gl/gl_surface_glx.cc...
ui/gl/gl_surface_glx.cc:589: if (size_.GetArea() == 0)
I think it's safer to leave this in, but it would be nice to know if it's really
needed?

[piman, 2015-04-17 19:37:29]

lgtm

https://codereview.chromium.org/1081533003/diff/80001/ui/gl/gl_surface_glx.cc
File ui/gl/gl_surface_glx.cc (right):

https://codereview.chromium.org/1081533003/diff/80001/ui/gl/gl_surface_glx.cc...
ui/gl/gl_surface_glx.cc:589: if (size_.GetArea() == 0)
On 2015/04/17 18:59:46, hendrikw wrote:
> I think it's safer to leave this in, but it would be nice to know if it's
really
> needed?

AIUI yes it's needed. XCreatWindow(3) isn't clear about it, but
XConfigureWindow(3) says width/height must be !0.

[vmiura, 2015-04-17 20:02:03]

https://codereview.chromium.org/1081533003/diff/80001/gpu/config/gpu_driver_b...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/1081533003/diff/80001/gpu/config/gpu_driver_b...
gpu/config/gpu_driver_bug_list_json.cc:1230: "disable_ext_occlusion_query"
Bug... missing ].

[hendrikw, 2015-04-17 20:03:23]

On 2015/04/17 20:02:03, vmiura wrote:
>
https://codereview.chromium.org/1081533003/diff/80001/gpu/config/gpu_driver_b...
> File gpu/config/gpu_driver_bug_list_json.cc (right):
> 
>
https://codereview.chromium.org/1081533003/diff/80001/gpu/config/gpu_driver_b...
> gpu/config/gpu_driver_bug_list_json.cc:1230: "disable_ext_occlusion_query"
> Bug... missing ].

Fixed, merge error

[jbauman, 2015-04-17 23:34:38]

lgtm, let's hope this works everywhere.

[mkollaro, 2015-04-28 10:33:11]

I tried out the patch and looked into apitrace. It showed me that the offscreen
framebuffer (for the virtual context) is of size 960x1055, just like the
onscreen framebuffer. Without the patch, only a 1x1 framebuffer is used. This is
quite a high overhead if I understand it correctly (maybe I don't, I'm a bit new
around this).

[hendrikw, 2015-04-28 21:07:18]

On 2015/04/28 10:33:11, mkollaro wrote:
> I tried out the patch and looked into apitrace. It showed me that the
offscreen
> framebuffer (for the virtual context) is of size 960x1055, just like the
> onscreen framebuffer. Without the patch, only a 1x1 framebuffer is used. This
is
> quite a high overhead if I understand it correctly (maybe I don't, I'm a bit
new
> around this).

mkollaro,

I've waded through a diff for a before and after apitrace, but I'm not sure what
to look for to see the offscreen buffer you mention.  Any hints?

[mkollaro, 2015-04-29 11:48:05]

> I've waded through a diff for a before and after apitrace, but I'm not sure
what
> to look for to see the offscreen buffer you mention.  Any hints?

Nevermind, seems that the extra context with the 1x1 surface isn't even created,
to this should be actually more efficient.

[hendrikw, 2015-05-01 00:19:36]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-01 00:19:36]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1081533003/#ps200001
(title: "yet another merge")

[commit-bot: I haz the power, 2015-05-01 00:20:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1081533003/200001

[commit-bot: I haz the power, 2015-05-01 00:32:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-01 00:34:49]

Patchset 11 (id:??) landed as
https://crrev.com/55e588cad550a65f2999255dfccea7db9b696a73
Cr-Commit-Position: refs/heads/master@{#327845}