Android: Use virtualized context only for those with compatible config

Android: Use virtualized context only for those with compatible config

This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/556e80df92feedd754453f9c98afbe40cff60335
Cr-Commit-Position: refs/heads/master@{#377161}

[Jinsuk Kim, 2016-02-01 08:02:12]

jinsukkim@chromium.org changed reviewers:
+ sievers@chromium.org

[Jinsuk Kim, 2016-02-01 08:02:12]

This CL associates the virtual context to a real context  of a config compatible
with it. Please see crbug.com/272429#c24 (d) for the details.

[Jinsuk Kim, 2016-02-12 13:50:34]

Description was changed from

==========
Multiple shared contexts in GLShareGroup

Manages multiple shared contexts to allow virtual GL
context initialization when the context switches to
a new one with incompatible configuration.

BUG=272429
==========

to

==========
Multiple shared contexts in GLShareGroup

Manages multiple shared contexts to allow virtual GL
context initialization when the context switches to
a new one with incompatible configuration.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Jinsuk Kim, 2016-02-12 20:30:14]

All the builds are green now.

[no sievers, 2016-02-17 00:56:26]

https://codereview.chromium.org/1652873002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:564: if
(use_virtualized_gl_context_ && share_group) {
Is it simpler to just treat |is_onscreen && is_lowend && is_32bit_requested| as
a non-virtual context?

And then we don't have to track multiple share contexts and we don't need
UpdateSharedContext().

I think Mac at least used to work in a similar way that it only uses context
virtualization for some of the contexts (onscreen I think).

But we do need to force all offscreen contexts into 16bit then also on low-end,
which is PbufferGLSurfaceEGL::Initialize(). We could just default the config to
16bit on lowend.

wdyt?

[Jinsuk Kim, 2016-02-17 07:23:32]

https://codereview.chromium.org/1652873002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:564: if
(use_virtualized_gl_context_ && share_group) {
On 2016/02/17 00:56:26, sievers wrote:
> Is it simpler to just treat |is_onscreen && is_lowend && is_32bit_requested|
as
> a non-virtual context?
> 
> And then we don't have to track multiple share contexts and we don't need
> UpdateSharedContext().
> 
> I think Mac at least used to work in a similar way that it only uses context
> virtualization for some of the contexts (onscreen I think).
> 
> But we do need to force all offscreen contexts into 16bit then also on
low-end,
> which is PbufferGLSurfaceEGL::Initialize(). We could just default the config
to
> 16bit on lowend.
> 
> wdyt?

Good to know that virtualization can be done for some of the contexts (was not
aware of it). Then the change can be much simpler. Done.

Note that multiple shared contexts in shared group has a benefit in that it
saves you from creating real context every time the request comes in. It also
works as a context cache reusing the previously created real context for new
requests. 


The default config for low-end device is set to 16bit. Please see
CreateOffscreenGLSurface(size) in gl_surface.h.

[no sievers, 2016-02-17 23:38:01]

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:166: bool
use_virtualized_gl_context) {
nit: this argument is unused now

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:566: // On-screen 16-bit surfaces
on low-end device won't use virtualized context.
You mean 32-bit surfaces, right?

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:568:
!base::SysInfo::IsLowEndDevice() ||
You could actually generalize this and just say that if the default offscreen
surface's format doesn't match the requested format (and the requested surface
is onscreen), then don't allow virtualization.

Or even more generally: if the share context's config is not compatible with the
requested config here.

https://codereview.chromium.org/1652873002/diff/40001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/40001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:214: return base::SysInfo::IsLowEndDevice() ? SURFACE_RGB565
: SURFACE_ARGB8888;
Can you make this logic/format override specific to GLSurfaceEGL (on- and
offscreen) and also Android?

[Jinsuk Kim, 2016-02-18 02:56:42]

Description was changed from

==========
Multiple shared contexts in GLShareGroup

Manages multiple shared contexts to allow virtual GL
context initialization when the context switches to
a new one with incompatible configuration.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Jinsuk Kim, 2016-02-18 02:57:11]

Patchset #4 (id:60001) has been deleted

[Jinsuk Kim, 2016-02-18 02:57:24]

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:166: bool
use_virtualized_gl_context) {
On 2016/02/17 23:38:01, sievers wrote:
> nit: this argument is unused now

Done.

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:566: // On-screen 16-bit surfaces
on low-end device won't use virtualized context.
On 2016/02/17 23:38:01, sievers wrote:
> You mean 32-bit surfaces, right?

My bad.. removed the comment as this part is being reverted.

https://codereview.chromium.org/1652873002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:568:
!base::SysInfo::IsLowEndDevice() ||
On 2016/02/17 23:38:01, sievers wrote:
> You could actually generalize this and just say that if the default offscreen
> surface's format doesn't match the requested format (and the requested surface
> is onscreen), then don't allow virtualization.
> 
> Or even more generally: if the share context's config is not compatible with
the
> requested config here.

Done.

https://codereview.chromium.org/1652873002/diff/40001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/40001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:214: return base::SysInfo::IsLowEndDevice() ? SURFACE_RGB565
: SURFACE_ARGB8888;
On 2016/02/17 23:38:01, sievers wrote:
> Can you make this logic/format override specific to GLSurfaceEGL (on- and
> offscreen) and also Android?

Done.

[no sievers, 2016-02-18 20:19:45]

https://codereview.chromium.org/1652873002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:581: if
(!context->Initialize(surface_.get(), gpu_preference_)) {
Unfortunately Initialize() is not reliable to determine if the context is
compatible, because of the IsCurrent(compatible_surface) check in there and
GLContextVirtual::IsCurrent() lying about this for offscreen surfaces, and
therefore depending on current state (in other words it might succeed now but
still fail MakeCurrent() at a later time).

How about simply toggling |use_virtualized_gl_context_| off in the constructor
above if
channel_->gpu_channel_manager()->GetDefaultOffscreenSurface()->GetFormat()
doesn't match |surface_format_|?

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:155: virtual GLSurface::Format GetDefaultFormat() const;
This is not needed anymore, right? Because how we use this is specific to
Android, it'd be nice not to expose too much of it everywhere.

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

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:544: return base::SysInfo::IsLowEndDevice() ?
SURFACE_RGB565 : SURFACE_ARGB8888;
wherever we override this, it needs #ifdef ANDROID since these EGL classes +
context virtualization are also used on ChromeOS+ARM.

[Jinsuk Kim, 2016-02-19 01:04:40]

Comments addressed. Please take another look.

https://codereview.chromium.org/1652873002/diff/80001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1652873002/diff/80001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:581: if
(!context->Initialize(surface_.get(), gpu_preference_)) {
On 2016/02/18 20:19:45, sievers wrote:
> Unfortunately Initialize() is not reliable to determine if the context is
> compatible, because of the IsCurrent(compatible_surface) check in there and
> GLContextVirtual::IsCurrent() lying about this for offscreen surfaces, and
> therefore depending on current state (in other words it might succeed now but
> still fail MakeCurrent() at a later time).
> 
> How about simply toggling |use_virtualized_gl_context_| off in the constructor
> above if
> channel_->gpu_channel_manager()->GetDefaultOffscreenSurface()->GetFormat()
> doesn't match |surface_format_|?

Got it. I guarded it with #if for it to take effect only on android. Let me know
if it looks okay.

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface.h#newc...
ui/gl/gl_surface.h:155: virtual GLSurface::Format GetDefaultFormat() const;
On 2016/02/18 20:19:45, sievers wrote:
> This is not needed anymore, right? Because how we use this is specific to
> Android, it'd be nice not to expose too much of it everywhere.

Removed.

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

https://codereview.chromium.org/1652873002/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:544: return base::SysInfo::IsLowEndDevice() ?
SURFACE_RGB565 : SURFACE_ARGB8888;
On 2016/02/18 20:19:45, sievers wrote:
> wherever we override this, it needs #ifdef ANDROID since these EGL classes +
> context virtualization are also used on ChromeOS+ARM.

Done. Moved it to PbufferGLSurafceEGL::Initialize().

[no sievers, 2016-02-19 20:25:18]

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface.h#new...
ui/gl/gl_surface.h:232: GLSurface::Format format_;
Can you initialize this? Actually, do we really need this here, since
GetFormat() is already virtual and can return FORMAT_DEFAULT in the base class
and something different for EGL and mesa, wdyt?

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

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:440: GLSurface(),
nit: not needed

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_osmes...
File ui/gl/gl_surface_osmesa.cc (right):

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_osmes...
ui/gl/gl_surface_osmesa.cc:23: format_ =
static_cast<GLSurface::Format>(OSMESA_BGRA);
hmm this seems like a hack. how about just adding OSMESA_BGRA and _RGBA to the
GLSurface enum?

And then in GLContextOSMesa where this is used you can just have a little switch
statement that converts it to a mesa GLenum. (This is actually a bit hacky in
mesa itself since it mixes actual GL formats with arbitrary mesa defines.)

[Jinsuk Kim, 2016-02-20 13:56:57]

Many things to revert!

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (right):

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface.h#new...
ui/gl/gl_surface.h:232: GLSurface::Format format_;
On 2016/02/19 20:25:18, sievers wrote:
> Can you initialize this? Actually, do we really need this here, since
> GetFormat() is already virtual and can return FORMAT_DEFAULT in the base class
> and something different for EGL and mesa, wdyt?

Removed.

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

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:440: GLSurface(),
On 2016/02/19 20:25:18, sievers wrote:
> nit: not needed

Done.

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_osmes...
File ui/gl/gl_surface_osmesa.cc (right):

https://codereview.chromium.org/1652873002/diff/100001/ui/gl/gl_surface_osmes...
ui/gl/gl_surface_osmesa.cc:23: format_ =
static_cast<GLSurface::Format>(OSMESA_BGRA);
On 2016/02/19 20:25:18, sievers wrote:
> hmm this seems like a hack. how about just adding OSMESA_BGRA and _RGBA to the
> GLSurface enum?
> 
> And then in GLContextOSMesa where this is used you can just have a little
switch
> statement that converts it to a mesa GLenum. (This is actually a bit hacky in
> mesa itself since it mixes actual GL formats with arbitrary mesa defines.)

Had meant to do it but didn't. This is certainly better. Done.

[no sievers, 2016-02-22 22:50:04]

lgtm w/nits

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_context_osmes...
File ui/gl/gl_context_osmesa.cc (right):

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_context_osmes...
ui/gl/gl_context_osmesa.cc:37: default:
nit: NOTREACHED() (and remove line 40). Maybe we should even return 'false' here
since the further behavior seems undefined (it should fail below, but what do
you end up if it doesn't?).

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

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:936: #if defined(OS_ANDROID)
nit: Can you put a comment that we do this for allowing context virtualization
which requires on- and offscreen to use a compatible config. We expect the
client to request RGB565 onscreen surfaces also for this to work (with the
exception of fullscreen video).

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.h...
ui/gl/gl_surface_egl.h:87: using GLSurfaceEGL::Initialize;
why this? GLSurfaceEGL also doesn't have Initialize().

[no sievers, 2016-02-22 22:50:25]

Description was changed from

==========
Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Android: Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Jinsuk Kim, 2016-02-23 01:27:39]

Thanks a lot for reviewing!

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_context_osmes...
File ui/gl/gl_context_osmesa.cc (right):

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_context_osmes...
ui/gl/gl_context_osmesa.cc:37: default:
On 2016/02/22 22:50:03, sievers wrote:
> nit: NOTREACHED() (and remove line 40). Maybe we should even return 'false'
here
> since the further behavior seems undefined (it should fail below, but what do
> you end up if it doesn't?).

Done.

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

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:936: #if defined(OS_ANDROID)
On 2016/02/22 22:50:03, sievers wrote:
> nit: Can you put a comment that we do this for allowing context virtualization
> which requires on- and offscreen to use a compatible config. We expect the
> client to request RGB565 onscreen surfaces also for this to work (with the
> exception of fullscreen video).

Done.

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1652873002/diff/120001/ui/gl/gl_surface_egl.h...
ui/gl/gl_surface_egl.h:87: using GLSurfaceEGL::Initialize;
On 2016/02/22 22:50:04, sievers wrote:
> why this? GLSurfaceEGL also doesn't have Initialize().

Used it to avoid following build error:

error: 'gfx::NativeViewGLSurfaceEGL::Initialize' hides overloaded virtual
function [-Werror,-Woverloaded-virtual]
  virtual bool Initialize(scoped_ptr<VSyncProvider> sync_provider);
               ^
../../ui/gl/gl_surface.h:52:16: note: hidden overloaded virtual function
'gfx::GLSurface::Initialize' declared here: different number of parameters (0 vs
1)
  virtual bool Initialize();
               ^

[Jinsuk Kim, 2016-02-24 00:03:12]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-02-24 00:03:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1652873002/#ps140001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-02-24 00:11:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1652873002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1652873002/140001

[commit-bot: I haz the power, 2016-02-24 01:01:39]

Description was changed from

==========
Android: Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Android: Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-02-24 01:01:40]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-02-24 01:02:47]

Description was changed from

==========
Android: Use virtualized context only for those with compatible config
    
This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Android: Use virtualized context only for those with compatible config

This allows low-end devices to have 16-bit surface by
default to save memory, using 32-bit surface only for
fullscreen video.

BUG=272429
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/556e80df92feedd754453f9c98afbe40cff60335
Cr-Commit-Position: refs/heads/master@{#377161}
==========

[commit-bot: I haz the power, 2016-02-24 01:02:48]

Patchset 7 (id:??) landed as
https://crrev.com/556e80df92feedd754453f9c98afbe40cff60335
Cr-Commit-Position: refs/heads/master@{#377161}