Add support for Khr_surfaceless_context.

Add support for Khr_surfaceless_context.

http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_surfaceless_context.txt

This patch adds support in GLSurfaceEGL to be able to use
surfaceless context when supported by the drivers. This avoids
the creation of a dummy offscreen surface. This would also enable
support for offscreen rendering on platforms (i.e Ozone-Wayland) which
donot support pbuffer surfaces.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232615

[piman, 2013-10-31 20:07:53]

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:57: bool g_egl_surfaceless_context = false;
nit: g_egl_surfaceless_context_supported for consistency

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:637: NOTREACHED() << "Attempted to call SwapBuffers with
SurfacelessEGL.";
note: technically it can be reached if the (untrusted) client call SwapBuffers
on the offscreen context. Maybe LOG(ERROR) instead.

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:751: surface = new SurfacelessEGL(size);
So, what if the client really meant to have an offscreen backbuffer? Could we
make this choice based on, say, size being (0x0), and fix the callers? We pass
1x1 everywhere for historical reasons, but we should clean that up...

[kalyank, 2013-10-31 22:15:27]

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:57: bool g_egl_surfaceless_context = false;
On 2013/10/31 20:07:54, piman wrote:
> nit: g_egl_surfaceless_context_supported for consistency

fixed.

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:637: NOTREACHED() << "Attempted to call SwapBuffers with
SurfacelessEGL.";
On 2013/10/31 20:07:54, piman wrote:
> note: technically it can be reached if the (untrusted) client call SwapBuffers
> on the offscreen context. Maybe LOG(ERROR) instead.

fixed.

https://codereview.chromium.org/49533003/diff/1/ui/gl/gl_surface_egl.cc#newco...
ui/gl/gl_surface_egl.cc:751: surface = new SurfacelessEGL(size);
On 2013/10/31 20:07:54, piman wrote:
> So, what if the client really meant to have an offscreen backbuffer? Could we
> make this choice based on, say, size being (0x0), and fix the callers? We pass
> 1x1 everywhere for historical reasons, but we should clean that up...

I for now added the checks with 1,1 size. If no one has started working on it, I
can fix the callers and change the size to 0x0 in a separate CL?

[piman, 2013-10-31 22:51:24]

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:752: (size.width() == 1 && size.height() == 1)) {
How about checking with 0,0 so that we don't regress?

[kalyank, 2013-10-31 23:18:07]

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:752: (size.width() == 1 && size.height() == 1)) {
On 2013/10/31 22:51:24, piman wrote:
> How about checking with 0,0 so that we don't regress?

Ya, I am k with that. I see that GPUChannerManager creates default offscreen
surface with a size of 1,1. The offscreen surface is used by texture
imagetransportsurface, commnadbuffer stub etc. so, is this something which needs
to be changed ? In other places we construct PBufferSurface directly i.e. in
imagetransportsurfacewin etc

I was thinking it's bit safe as it's used only with ozone egl backend but
realized the code is used by Android too.

[kalyank, 2013-10-31 23:49:11]

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:188: // Check if SurfacelessEGL is supported.
On a second thought another possible solution to avoid regression on other
platforms :
Guard extension checks for ozone. So, SurfacelessEGL support is only enabled for
ozone with this CL.

Than we can create another CL which would make appropriate changes to enable
this on other supported platforms?

[piman, 2013-11-01 01:23:37]

The right thing to do is:
1- introduce the concept (that we don't really have right now) that we want
to create surfaces that we won't use, just there to make context current.
We can key that off size = 0x0 (or something else, but I'm ok with this)
2- make sure we have reasonable defaults for these surfaces on all platforms
3- where available, use the surfaceless extension, if available, for those.

This CL is step 3.
We need steps 1 and 2 though. I'm ok with 3 going in first as long as it
doesn't regress. So one way is to key the decision off of 0x0 and work on
steps 1 and 2, another way is to do 1 and 2 first, a third way is to do it
all at once. But I don't think introducing #ifdef OZONE is going into the
right direction.



On Thu, Oct 31, 2013 at 4:49 PM, <kalyan.kondapally@intel.com> wrote:

>
> https://codereview.chromium.**org/49533003/diff/60001/ui/gl/**
>
gl_surface_egl.cc<https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc>
> File ui/gl/gl_surface_egl.cc (right):
>
> https://codereview.chromium.**org/49533003/diff/60001/ui/gl/**
>
gl_surface_egl.cc#newcode188<https://codereview.chromium.org/49533003/diff/60001/ui/gl/gl_surface_egl.cc#newcode188>
> ui/gl/gl_surface_egl.cc:188: // Check if SurfacelessEGL is supported.
> On a second thought another possible solution to avoid regression on
> other platforms :
> Guard extension checks for ozone. So, SurfacelessEGL support is only
> enabled for ozone with this CL.
>
> Than we can create another CL which would make appropriate changes to
> enable this on other supported platforms?
>
>
https://codereview.chromium.**org/49533003/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kalyank, 2013-11-01 02:02:13]

On 2013/11/01 01:23:37, piman wrote:
> The right thing to do is:
> 1- introduce the concept (that we don't really have right now) that we want
> to create surfaces that we won't use, just there to make context current.
> We can key that off size = 0x0 (or something else, but I'm ok with this)
> 2- make sure we have reasonable defaults for these surfaces on all platforms
> 3- where available, use the surfaceless extension, if available, for those.
> 
> This CL is step 3.
> We need steps 1 and 2 though. I'm ok with 3 going in first as long as it
> doesn't regress. So one way is to key the decision off of 0x0 and work on
> steps 1 and 2, another way is to do 1 and 2 first, a third way is to do it
> all at once. But I don't think introducing #ifdef OZONE is going into the
> right direction.
> 

I have now changed the decision key to 0 x 0. I am ok with holding on to these
changes till 1 and 2 are done. I can start looking at those tasks.

[piman, 2013-11-01 02:52:45]

LGTM

[kalyank, 2013-11-01 15:15:06]

@rjkroege can you please take a look ?

[rjkroege, 2013-11-01 15:20:20]

lgtm

[commit-bot: I haz the power, 2013-11-01 18:14:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalyan.kondapally@intel.com/49533003/1...

[commit-bot: I haz the power, 2013-11-01 20:12:21]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-02 12:17:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalyan.kondapally@intel.com/49533003/1...

[commit-bot: I haz the power, 2013-11-02 14:00:40]

Change committed as 232615