Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1738973004:
Fix EGL configs with GLSurfaceOzoneSurfaceless. (Closed)
DescriptionFix EGL configs with ozone and surfaceless surfaces.
This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling
SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default
implementation in GLSurfaceEGL::GetConfig() would be used to pick an
EGL config. This resulted in "No suitable EGL configs found." being
logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now
calls SurfaceOzoneEGL::GetEGLSurfaceConfig().
For offscreen surfaces SurfacelessEGL is used. This produced the same
error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't
be used with offscren surfaces as there is no window associated with the
offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to
EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces.
The bug doesn't seem to cause problems beyond the logged error messages
as drawing still works under Ozone.
BUG=591064
Committed: https://crrev.com/445089ac3c1b949fb4248f3aef3ce5e7acd66d92
Cr-Commit-Position: refs/heads/master@{#380171}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add back factory method. #
Total comments: 2
Patch Set 3 : Fixes for comments. #Patch Set 4 : Different solution for offscreen. #Patch Set 5 : Fix config map. #Patch Set 6 : Only for surfaceless. #
Total comments: 2
Patch Set 7 : Added format SURFACE_SURFACELESS. #
Messages
Total messages: 37 (15 generated)
Description was changed from ========== Fix EGL configs with GLSurfaceOzoneSurfaceless. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick EGL config. Unable to reproduce any problems on Chromebook Pixel. It looks like the default EGL config worked fine but it wasn't intended behiavour. Test to still work with this patch. BUG= ========== to ========== Fix EGL configs with GLSurfaceOzoneSurfaceless. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. Unable to reproduce any errors on Chromebook Pixel. It looks like the default EGL config worked fine but it wasn't intended behaviour. Tested that EGL/graphics still work with this patch. BUG= ==========
kylechar@chromium.org changed reviewers: + dnicoara@chromium.org
dnicoara@chromium.org changed reviewers: + spang@chromium.org
+ spang@ https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); I'm still worried about this one and the PbufferGLSurfaceEGL implementation since they both inherit GLSurfaceEGL and both of their GetConfig() implementations no longer go through the Ozone path. They'll eventually be used to create a GLContextEGL which will call GetConfig().
Description was changed from ========== Fix EGL configs with GLSurfaceOzoneSurfaceless. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. Unable to reproduce any errors on Chromebook Pixel. It looks like the default EGL config worked fine but it wasn't intended behaviour. Tested that EGL/graphics still work with this patch. BUG= ========== to ========== Fix EGL configs with GLSurfaceOzoneSurfaceless. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. Unable to reproduce any errors on Chromebook Pixel. It looks like the default EGL config worked fine but it wasn't intended behaviour. Tested that EGL/graphics still work with this patch. BUG=591064 ==========
I added back SurfaceFactoryOzone::GetEGLSurfaceProperties(). This should fix any problems but it's not the cleanest solution. There would then be one method on SurfaceFactoryOzone that provides attribs related to getting a config and different method in SurfaceOzoneEGL that handles getting a config. Let me know what you think? https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc File ui/gl/gl_surface_ozone.cc (right): https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); On 2016/02/26 at 16:15:00, dnicoara wrote: > I'm still worried about this one and the PbufferGLSurfaceEGL implementation since they both inherit GLSurfaceEGL and both of their GetConfig() implementations no longer go through the Ozone path. They'll eventually be used to create a GLContextEGL which will call GetConfig(). Oh, right. I've added back GetEGLSurfaceProperties() and associated code to handle this.
lgtm It may be best to provide some extra documentation in SurfaceFactoryOzone. Specifically that each individual surface may override the default configuration by implementing GetEGLSurfaceConfig(). https://codereview.chromium.org/1738973004/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right): https://codereview.chromium.org/1738973004/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:73: EGL_WINDOW_BIT, This should be EGL_DONT_CARE (marcheu@ updated this to better reflect that a window isn't created in: https://codereview.chromium.org/1736883002)
Added documentation back to SurfaceFactoryOzone. Does this work for you spang@? https://codereview.chromium.org/1738973004/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right): https://codereview.chromium.org/1738973004/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:73: EGL_WINDOW_BIT, On 2016/03/01 at 18:15:56, dnicoara wrote: > This should be EGL_DONT_CARE (marcheu@ updated this to better reflect that a window isn't created in: https://codereview.chromium.org/1736883002) Done.
On 2016/02/26 16:15:01, dnicoara wrote: > + spang@ > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc > File ui/gl/gl_surface_ozone.cc (right): > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... > ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); > I'm still worried about this one and the PbufferGLSurfaceEGL implementation > since they both inherit GLSurfaceEGL and both of their GetConfig() > implementations no longer go through the Ozone path. They'll eventually be used > to create a GLContextEGL which will call GetConfig(). The GLSurfaceOzoneSurfaceless change looks good. I am not convinced about bringing back the override for offscreen surfaces. That interface was awful, and it makes no sense to have both the new interface and the old broken one. Does surface configuration even matter for SurfacelessEGL? here is no surface in that case. If there's a regression, we should revert the original change completely. I do not want both interfaces; surface construction is hard enough to follow without two ways to override configuration.
On 2016/03/03 18:29:54, spang wrote: > On 2016/02/26 16:15:01, dnicoara wrote: > > + spang@ > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc > > File ui/gl/gl_surface_ozone.cc (right): > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... > > ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); > > I'm still worried about this one and the PbufferGLSurfaceEGL implementation > > since they both inherit GLSurfaceEGL and both of their GetConfig() > > implementations no longer go through the Ozone path. They'll eventually be > used > > to create a GLContextEGL which will call GetConfig(). > > The GLSurfaceOzoneSurfaceless change looks good. > > I am not convinced about bringing back the override for offscreen surfaces. That > interface was awful, and it makes no sense to have both the new interface and > the old broken one. > > Does surface configuration even matter for SurfacelessEGL? here is no surface in > that case. The config is validated multiple times even for Surfaceless (more than once on some code paths) and generates a LOG(ERROR). An "ignorable error" seems like bad software engineering to me. Either the error matters and we should fix why it happens or it's not really an error and we should stop emitting it. > > If there's a regression, we should revert the original change completely. I do > not want both interfaces; surface construction is hard enough to follow without > two ways to override configuration.
On 2016/03/03 18:57:09, rjkroege wrote: > On 2016/03/03 18:29:54, spang wrote: > > On 2016/02/26 16:15:01, dnicoara wrote: > > > + spang@ > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc > > > File ui/gl/gl_surface_ozone.cc (right): > > > > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... > > > ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); > > > I'm still worried about this one and the PbufferGLSurfaceEGL implementation > > > since they both inherit GLSurfaceEGL and both of their GetConfig() > > > implementations no longer go through the Ozone path. They'll eventually be > > used > > > to create a GLContextEGL which will call GetConfig(). > > > > The GLSurfaceOzoneSurfaceless change looks good. > > > > I am not convinced about bringing back the override for offscreen surfaces. > That > > interface was awful, and it makes no sense to have both the new interface and > > the old broken one. > > > > Does surface configuration even matter for SurfacelessEGL? here is no surface > in > > that case. > > The config is validated multiple times even for Surfaceless (more than once on > some code paths) and generates a LOG(ERROR). An "ignorable error" seems like bad > software engineering to me. Either the error matters and we should fix why it > happens or it's not really an error and we should stop emitting it. > I never said to ignore errors. Can you provide the text of the error? > > > > If there's a regression, we should revert the original change completely. I do > > not want both interfaces; surface construction is hard enough to follow > without > > two ways to override configuration.
On 2016/03/03 at 20:07:44, spang wrote: > On 2016/03/03 18:57:09, rjkroege wrote: > > On 2016/03/03 18:29:54, spang wrote: > > > On 2016/02/26 16:15:01, dnicoara wrote: > > > > + spang@ > > > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc > > > > File ui/gl/gl_surface_ozone.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... > > > > ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); > > > > I'm still worried about this one and the PbufferGLSurfaceEGL implementation > > > > since they both inherit GLSurfaceEGL and both of their GetConfig() > > > > implementations no longer go through the Ozone path. They'll eventually be > > > used > > > > to create a GLContextEGL which will call GetConfig(). > > > > > > The GLSurfaceOzoneSurfaceless change looks good. > > > > > > I am not convinced about bringing back the override for offscreen surfaces. > > That > > > interface was awful, and it makes no sense to have both the new interface and > > > the old broken one. > > > > > > Does surface configuration even matter for SurfacelessEGL? here is no surface > > in > > > that case. > > > > The config is validated multiple times even for Surfaceless (more than once on > > some code paths) and generates a LOG(ERROR). An "ignorable error" seems like bad > > software engineering to me. Either the error matters and we should fix why it > > happens or it's not really an error and we should stop emitting it. > > > > I never said to ignore errors. > > Can you provide the text of the error? > > > > > > > If there's a regression, we should revert the original change completely. I do > > > not want both interfaces; surface construction is hard enough to follow > > without > > > two ways to override configuration. I was able to reproduce it. So the error looks like the following. [13435:13435:0304/091942:ERROR:gl_surface_egl.cc(248)] No suitable EGL configs found. This occurs with a surface of type SurfacelessEGL where GLContextEGL::Initialize calls GetConfig(). https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_context_e... The specifics of why the surface config matters to the context when no surface exists are a bit lost on me. It definitely works despite the error on a Pixel. There are lots of other hardware configurations though.
On 2016/03/04 14:37:38, kylechar wrote: > On 2016/03/03 at 20:07:44, spang wrote: > > On 2016/03/03 18:57:09, rjkroege wrote: > > > On 2016/03/03 18:29:54, spang wrote: > > > > On 2016/02/26 16:15:01, dnicoara wrote: > > > > > + spang@ > > > > > > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc > > > > > File ui/gl/gl_surface_ozone.cc (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1738973004/diff/1/ui/gl/gl_surface_ozone.cc#n... > > > > > ui/gl/gl_surface_ozone.cc:755: surface = new SurfacelessEGL(size); > > > > > I'm still worried about this one and the PbufferGLSurfaceEGL > implementation > > > > > since they both inherit GLSurfaceEGL and both of their GetConfig() > > > > > implementations no longer go through the Ozone path. They'll eventually > be > > > > used > > > > > to create a GLContextEGL which will call GetConfig(). > > > > > > > > The GLSurfaceOzoneSurfaceless change looks good. > > > > > > > > I am not convinced about bringing back the override for offscreen > surfaces. > > > That > > > > interface was awful, and it makes no sense to have both the new interface > and > > > > the old broken one. > > > > > > > > Does surface configuration even matter for SurfacelessEGL? here is no > surface > > > in > > > > that case. > > > > > > The config is validated multiple times even for Surfaceless (more than once > on > > > some code paths) and generates a LOG(ERROR). An "ignorable error" seems like > bad > > > software engineering to me. Either the error matters and we should fix why > it > > > happens or it's not really an error and we should stop emitting it. > > > > > > > I never said to ignore errors. > > > > Can you provide the text of the error? > > > > > > > > > > If there's a regression, we should revert the original change completely. > I do > > > > not want both interfaces; surface construction is hard enough to follow > > > without > > > > two ways to override configuration. > > I was able to reproduce it. So the error looks like the following. > > [13435:13435:0304/091942:ERROR:gl_surface_egl.cc(248)] No suitable EGL configs > found. > > This occurs with a surface of type SurfacelessEGL where GLContextEGL::Initialize > calls GetConfig(). > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_context_e... > > The specifics of why the surface config matters to the context when no surface > exists are a bit lost on me. It definitely works despite the error on a Pixel. > There are lots of other hardware configurations though. This seems like a generic offscreen surfaceless issue. The out of box behavior for SurfacelessEGL is to pass in garbage to eglChooseConfig, so we get an error. For example EGL_SURFACE_TYPE == EGL_WINDOW_BIT | EGL_PBUFFER_BIT. It's bogus to ask for that when you do not actually want to create a surface.
Here is a different solution for the offscreen surfaces and ELG errors. The only problematic attrib was EGL_SURFACE_TYPE so for offscreen set that to EGL_DONT_CARE. Looks much cleaner.
Ugh. The config_map part of ChooseConfig will have to be changed to track onscreen/offscreen too.
Description was changed from ========== Fix EGL configs with GLSurfaceOzoneSurfaceless. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. Unable to reproduce any errors on Chromebook Pixel. It looks like the default EGL config worked fine but it wasn't intended behaviour. Tested that EGL/graphics still work with this patch. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. BUG=591064 ==========
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. BUG=591064 ==========
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. The bug doesn't seem to cause problems beyond the error messages but should it it should likely be fixed. BUG=591064 ==========
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. The bug doesn't seem to cause problems beyond the error messages but should it it should likely be fixed. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. The bug doesn't seem to cause problems beyond the error messages but then something happened? BUG=591064 ==========
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() if it is surfaceless. The bug doesn't seem to cause problems beyond the error messages but then something happened? BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces. The bug doesn't seem to cause problems beyond the logged error messages as drawing still works under Ozone. BUG=591064 ==========
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces. The bug doesn't seem to cause problems beyond the logged error messages as drawing still works under Ozone. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces. The bug doesn't seem to cause problems beyond the logged error messages as drawing still works under Ozone. BUG=591064 ==========
alexst@chromium.org changed reviewers: + alexst@chromium.org
This looks reasonable to me.
lgtm
kylechar@chromium.org changed reviewers: + sievers@chromium.org
Hi sievers@, can you look at gl_surface_egl.cc? It fixes the error message that was occurring on DRM and seems to pass on all the bots. It also sort of makes sense that surfaceless contexts shouldn't care about EGL_SURFACE_TYPE.
https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.c... ui/gl/gl_surface_egl.cc:255: static std::map<std::pair<GLSurface::Format, bool>, EGLConfig> config_map; It seems odd that surfaceless even cares about |Format|. We could also add a FORMAT_SURFACELESS or something for Format instead. wdyt?
https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.c... ui/gl/gl_surface_egl.cc:255: static std::map<std::pair<GLSurface::Format, bool>, EGLConfig> config_map; On 2016/03/08 at 22:02:00, sievers wrote: > It seems odd that surfaceless even cares about |Format|. > We could also add a FORMAT_SURFACELESS or something for Format instead. wdyt? This is all sort of black magic to me (and documentation is very sparse). The EGL config for surfaceless GLSurfaces gets used when creating a context. It would sort of make sense that EGL config ARGB should match the match whatever framebuffer object the context is going to be rendering into, so maybe GLSurface::Format does matter? On the other hand maybe I'm just totally misunderstanding things here? The only place SURFACE_RGB565 is actually ever set is with PbufferGLSurfaceEGL. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_e... That means it can't matter for surfaceless (or it does matter but all surfaceless contexts are drawing into ARGB8888 framebuffers?) I guess adding a new format SURFACE_SURFACELESS should work.
Trybots seem happy with SURFACE_SURFACELESS.
On 2016/03/09 15:07:30, kylechar wrote: > https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.cc > File ui/gl/gl_surface_egl.cc (right): > > https://codereview.chromium.org/1738973004/diff/100001/ui/gl/gl_surface_egl.c... > ui/gl/gl_surface_egl.cc:255: static std::map<std::pair<GLSurface::Format, bool>, > EGLConfig> config_map; > On 2016/03/08 at 22:02:00, sievers wrote: > > It seems odd that surfaceless even cares about |Format|. > > We could also add a FORMAT_SURFACELESS or something for Format instead. wdyt? > > This is all sort of black magic to me (and documentation is very sparse). The > EGL config for surfaceless GLSurfaces gets used when creating a context. It > would sort of make sense that EGL config ARGB should match the match whatever > framebuffer object the context is going to be rendering into, so maybe > GLSurface::Format does matter? > > On the other hand maybe I'm just totally misunderstanding things here? The only > place SURFACE_RGB565 is actually ever set is with PbufferGLSurfaceEGL. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_e... > > That means it can't matter for surfaceless (or it does matter but all > surfaceless contexts are drawing into ARGB8888 framebuffers?) I guess adding a > new format SURFACE_SURFACELESS should work. lgtm. The spec says that the config used to create the context has to match the one used to create the surface wrt certain attributes (such as channels and bit depth). But since there is no actual surface it probably doesn't make sense to talk about RGBA8 vs. RGB565 and so forth. (i.e. just use the default RGBA8 config for surfaceless like we used to).
The CQ bit was checked by kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dnicoara@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1738973004/#ps120001 (title: "Added format SURFACE_SURFACELESS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738973004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738973004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces. The bug doesn't seem to cause problems beyond the logged error messages as drawing still works under Ozone. BUG=591064 ========== to ========== Fix EGL configs with ozone and surfaceless surfaces. This CL fixes a bug where GLSurfaceOzoneSurfaceless was not calling SurfaceOzoneEGL::GetEGLSurfaceConfig(). Instead, the default implementation in GLSurfaceEGL::GetConfig() would be used to pick an EGL config. This resulted in "No suitable EGL configs found." being logged everytime a GLSurfaceOzoneSurfaceless was created. Instead it now calls SurfaceOzoneEGL::GetEGLSurfaceConfig(). For offscreen surfaces SurfacelessEGL is used. This produced the same error message on creation. SurfaceOzoneEGL::GetEGLSurfaceConfig() can't be used with offscren surfaces as there is no window associated with the offscreen surface. Instead, the EGL_SURFACE_TYPE attribute is set to EGL_DONT_CARE in GLSurfaceEGL::GetConfig() for surfaceless surfaces. The bug doesn't seem to cause problems beyond the logged error messages as drawing still works under Ozone. BUG=591064 Committed: https://crrev.com/445089ac3c1b949fb4248f3aef3ce5e7acd66d92 Cr-Commit-Position: refs/heads/master@{#380171} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/445089ac3c1b949fb4248f3aef3ce5e7acd66d92 Cr-Commit-Position: refs/heads/master@{#380171} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
