Issue 2347383002: X11: Use better visuals for OpenGL

Tom (Use chromium acct)

Description was changed from ========== [WIP] X11: Use a better visual for OpenGL BUG= ========== ...

4 years, 3 months ago (2016-09-18 04:46:21 UTC) #1

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-21 22:51:28 UTC) #2

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/80001

4 years, 3 months ago (2016-09-21 22:52:03 UTC) #3

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-21 23:03:01 UTC) #4

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/272691)

4 years, 3 months ago (2016-09-21 23:03:02 UTC) #5

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-21 23:40:07 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/100001

4 years, 3 months ago (2016-09-21 23:40:34 UTC) #7

Tom (Use chromium acct)

The CQ bit was unchecked by thomasanderson@google.com

4 years, 3 months ago (2016-09-21 23:43:50 UTC) #8

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-22 00:24:57 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/120001

4 years, 3 months ago (2016-09-22 00:25:17 UTC) #11

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-22 00:40:53 UTC) #12

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/132963) linux_chromium_asan_rel_ng on ...

4 years, 3 months ago (2016-09-22 00:40:55 UTC) #13

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-22 01:32:43 UTC) #14

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/140001

4 years, 3 months ago (2016-09-22 01:33:00 UTC) #15

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-22 02:26:11 UTC) #16

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/230537)

4 years, 3 months ago (2016-09-22 02:26:11 UTC) #17

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-22 02:48:07 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/160001

4 years, 3 months ago (2016-09-22 02:48:25 UTC) #19

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-22 04:14:24 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/302130)

4 years, 3 months ago (2016-09-22 04:14:25 UTC) #21

Tom (Use chromium acct)

Patchset #6 (id:120001) has been deleted

4 years, 3 months ago (2016-09-22 19:29:00 UTC) #22

Tom (Use chromium acct)

Patchset #6 (id:140001) has been deleted

4 years, 3 months ago (2016-09-22 19:29:09 UTC) #23

Tom (Use chromium acct)

Patchset #6 (id:160001) has been deleted

4 years, 3 months ago (2016-09-22 19:29:17 UTC) #24

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-22 19:29:44 UTC) #25

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/180001

4 years, 3 months ago (2016-09-22 19:30:07 UTC) #26

Tom (Use chromium acct)

Description was changed from ========== [WIP] X11: Use a better visual for OpenGL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ...

4 years, 3 months ago (2016-09-22 19:42:53 UTC) #27

Tom (Use chromium acct)

thomasanderson@google.com changed reviewers: + calamity@chromium.org, dcheng@chromium.org, j.isorce@samsung.com, piman@chromium.org, sadrul@chromium.org, watk@chromium.org

4 years, 3 months ago (2016-09-22 19:45:23 UTC) #28

Tom (Use chromium acct)

Description was changed from ========== [WIP] X11: Use a better visual for OpenGL BUG=347333,369209 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ...

4 years, 3 months ago (2016-09-22 19:52:04 UTC) #29

Tom (Use chromium acct)

From the git-cl owners: watk: media/gpu/avda_picture_buffer_manager.cc calamity: extensions/browser/api/app_window/app_window_apitest.cc sadrul: ui/base/x/x11_util.cc ui/base/x/x11_util_internal.h ui/base/x/x11_util_unittest.cc ui/gfx/x/x11_switches.cc ui/gfx/x/x11_switches.h ui/gfx/x/x11_types.h ...

4 years, 3 months ago (2016-09-22 19:53:29 UTC) #30

From the git-cl owners:

watk:
  media/gpu/avda_picture_buffer_manager.cc
calamity:
  extensions/browser/api/app_window/app_window_apitest.cc
sadrul:
  ui/base/x/x11_util.cc
  ui/base/x/x11_util_internal.h
  ui/base/x/x11_util_unittest.cc
  ui/gfx/x/x11_switches.cc
  ui/gfx/x/x11_switches.h
  ui/gfx/x/x11_types.h
  ui/gl/BUILD.gn
  ui/gl/gl_context.cc
  ui/gl/gl_context.h
  ui/gl/gl_context_glx.cc
  ui/gl/gl_share_group.cc
  ui/gl/gl_share_group.h
  ui/gl/gl_share_group_glx.cc
  ui/gl/gl_share_group_glx.h
  ui/gl/gl_surface_egl.cc
  ui/gl/gl_surface_glx.cc
  ui/gl/gl_surface_glx.h
  ui/gl/gl_visual_picker_glx.cc
  ui/gl/gl_visual_picker_glx.h
  ui/views/test/views_test_base.cc
  ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
dcheng:
  gpu/ipc/common/gpu_info.mojom
  gpu/ipc/common/gpu_info_struct_traits.cc
  gpu/ipc/common/gpu_info_struct_traits.h
  gpu/ipc/common/gpu_param_traits_macros.h
  gpu/ipc/common/struct_traits_unittest.cc
piman:
  content/browser/browser_main_loop.cc
  content/browser/gpu/gpu_data_manager_impl_private.cc
  content/browser/gpu/gpu_internals_ui.cc
  content/browser/gpu/gpu_process_host.cc
  content/gpu/gpu_main.cc
  content/test/gpu/page_sets/gpu_process_tests.py
  gpu/command_buffer/service/gl_context_virtual_unittest.cc
  gpu/command_buffer/service/in_process_command_buffer.cc
  gpu/config/gpu_driver_bug_list_json.cc
  gpu/config/gpu_driver_bug_workaround_type.h
  gpu/config/gpu_feature_type.h
  gpu/config/gpu_info.cc
  gpu/config/gpu_info.h
  gpu/config/gpu_info_collector.cc
  gpu/ipc/service/gpu_channel_manager.cc
  gpu/ipc/service/gpu_command_buffer_stub.cc
  ui/gl/BUILD.gn
  ui/gl/gl_context.cc
  ui/gl/gl_context.h
  ui/gl/gl_context_glx.cc
  ui/gl/gl_share_group.cc
  ui/gl/gl_share_group.h
  ui/gl/gl_share_group_glx.cc
  ui/gl/gl_share_group_glx.h
  ui/gl/gl_surface_egl.cc
  ui/gl/gl_surface_glx.cc
  ui/gl/gl_surface_glx.h
  ui/gl/gl_visual_picker_glx.cc
  ui/gl/gl_visual_picker_glx.h

Also +j.isorce@ who did some work on transparent visuals recently

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:1179: //
TODO(thomasanderson): figure out what to do with this
I left this here as a placeholder but I'm not quite sure where to actually put
it.  Maybe just make a GpuInfoObserver and stick it in browser_main_loop.cc
where the disable_transparent_visual stuff got removed?

Also, I don't know how to reliably figure out whether we're using software
compositing.  The "gpu_info_.software_rendering || card_blacklisted_" doesn't
work with --single-process.  And GpuAccessAllowed() returns false without
--disable-gpu before we have the GPUInfo.

https://codereview.chromium.org/2347383002/diff/180001/extensions/browser/api...
File extensions/browser/api/app_window/app_window_apitest.cc (left):

https://codereview.chromium.org/2347383002/diff/180001/extensions/browser/api...
extensions/browser/api/app_window/app_window_apitest.cc:155: #if
defined(USE_X11) && !defined(OS_CHROMEOS)
I had to remove this because the GL implementation used is
kGLImplementationOSMesaGL, not kGLImplementationDesktopGL :(

https://codereview.chromium.org/2347383002/diff/180001/gpu/config/gpu_info_co...
File gpu/config/gpu_info_collector.cc (right):

https://codereview.chromium.org/2347383002/diff/180001/gpu/config/gpu_info_co...
gpu/config/gpu_info_collector.cc:181: if (gl::GetGLImplementation() ==
gl::kGLImplementationDesktopGL) {
The glx library is not loaded unless gl::GetGLImplementation() ==
gl::kGLImplementationDesktopGL.  So, I can't make calls to glXGetConfig

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-22 20:48:59 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_tests_rel/builds/3943)

4 years, 3 months ago (2016-09-22 20:49:01 UTC) #33

piman

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1180 content/browser/gpu/gpu_data_manager_impl_private.cc:1180: ui::XVisualManager::GetInstance()->OnGPUInfoChanged( This function is called on the IO thread ...

4 years, 3 months ago (2016-09-22 21:24:55 UTC) #34

Tom (Use chromium acct)

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 3 months ago (2016-09-23 01:53:41 UTC) #35

Tom (Use chromium acct)

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1180 content/browser/gpu/gpu_data_manager_impl_private.cc:1180: ui::XVisualManager::GetInstance()->OnGPUInfoChanged( On 2016/09/22 21:24:54, piman OOO back 2016-09-12 wrote: ...

4 years, 3 months ago (2016-09-23 20:00:38 UTC) #36

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-23 20:00:47 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/200001

4 years, 3 months ago (2016-09-23 20:02:09 UTC) #38

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-23 20:06:34 UTC) #39

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/133869) android_clang_dbg_recipe on ...

4 years, 3 months ago (2016-09-23 20:06:36 UTC) #40

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 3 months ago (2016-09-23 20:10:01 UTC) #41

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/220001

4 years, 3 months ago (2016-09-23 20:10:31 UTC) #42

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-23 21:22:47 UTC) #43

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_gpu_tests_rel/builds/2841)

4 years, 3 months ago (2016-09-23 21:22:48 UTC) #44

piman

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1180 content/browser/gpu/gpu_data_manager_impl_private.cc:1180: ui::XVisualManager::GetInstance()->OnGPUInfoChanged( On 2016/09/23 20:00:37, Tom Anderson wrote: > On ...

4 years, 2 months ago (2016-09-26 21:13:29 UTC) #45

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:1180:
ui::XVisualManager::GetInstance()->OnGPUInfoChanged(
On 2016/09/23 20:00:37, Tom Anderson wrote:
> On 2016/09/22 21:24:54, piman OOO back 2016-09-12 wrote:
> > This function is called on the IO thread (and sometimes on the UI thread).
> IIUC,
> > XVisualManager is intended to be called from the UI thread, so I think this
> > would cause some races. Maybe it would be better to add a
> GpuDataManagerObserver
> > to forward that info? observer_list_ is thread-safe, so the observer would
be
> > called on the same thread it's registered from (the UI thread if added from
> > BrowserMainLoop).
> 
> Done.  Added GpuDataManagerVisualProxy in browser_main_loop.
> 
> Although it still doesn't work with --single-process, and this is because
> CollectGraphicsInfoGL never gets called.
> 
> I suppose we could check for BrowserMainLoop::UsingInProcessGpu(), and in that
> case, just use GLVisualPickerGLX directly since the GL stuff would be loaded? 
> Or is it even worth it to support this in --single-process?

I think I'm ok if we punt on --single-process.
Directly using GLVisualPickerGLX may be a bit iffy because in --single-process,
the GPU main thread is a separate thread from the UI thread. Another way is to
add a call to gpu::CollectContextGraphicsInfo in InProcessGpuThread and pass the
GpuInfo that to the in-process GpuChildThread constructor.

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

https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:39: #include "ui/gl/gl_visual_picker_glx.h"
On 2016/09/23 20:00:37, Tom Anderson wrote:
> On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > When we're using EGL, we are not using GLX, so I don't think this will
work...
> 
> Done.  I opted to use XVisualManager instead.  Unfortunately this means I also
> needed to add a dependency on ui/base/x.  PLMK if this is not right

I think this is ok.

https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
File ui/gl/gl_visual_picker_glx.cc (right):

https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
ui/gl/gl_visual_picker_glx.cc:108: return PickBestGlVisual(filtered_visuals,
IsRgbaVisual(default_visual_info));
On 2016/09/23 20:00:38, Tom Anderson wrote:
> On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > For offscreen and opaque windows, the best visual is the root visual. Any
> other
> > visual will typically involve a copy on present, reducing performance.
> > 
> > Any reason not to go with that?
> 
> On my system there's a bunch of visuals that have identical XVisualInfo
> properties as the default visual (indicated by xdpyinfo), but have different
GL
> properties (indicated by glxinfo).  Would this reduce performance if it's only
> the GL stuff that's different?  If so, and if we don't need any extra things
> like double-buffer in the future, I'm fine with removing this.

It's hard to know without testing, in particular with closed-source drivers.
Some of the things I can think of that could impact things are color space (sRGB
vs linear), buffer formats (float vs int), or the use of a colormap (which is
necessary for different visuals), or internal things such as framebuffer
compression.
The best way to test is with a fill-rate limited example, and see the frame
rate. The WebGL aquarium http://webglsamples.org/aquarium/aquarium.html may be a
good choice at high resolution.

One option is to try the root visual and see if it satisfies requirements, and
if so pick that, otherwise pick the best match?

https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
ui/gl/gl_visual_picker_glx.cc:146: std::remove_if(filtered_visuals.begin(),
filtered_visuals.end(),
On 2016/09/23 20:00:38, Tom Anderson wrote:
> On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > Note: remove_if doesn't remove anything, it just moves it to the end, so I
> don't
> > think you filter anything if you don't erase elements starting at the
iterator
> > returned by remove_if.
> > 
> > 
> > Maybe it would be better to simply not copy it, i.e. by moving the
> IsRgbaVisual
> > check to the lambda inside the copy_if?
> 
> Done.  Nice catch, I think the reason it was returning the right visuals
before
> was because the selection algorithm in PickBestGlVisual happened to pick the
> first visual with the highest score.

But you don't want to check inside the copy_if? It does seem a little simpler
(and efficient fwiw).

https://codereview.chromium.org/2347383002/diff/220001/ui/gl/gl_share_group.cc
File ui/gl/gl_share_group.cc (right):

https://codereview.chromium.org/2347383002/diff/220001/ui/gl/gl_share_group.c...
ui/gl/gl_share_group.cc:61: int fbconfig_id = compatible->GetCompatibilityKey();
nit: compatibility_key instead of fbconfig_id?
Also, use unsigned long instead of int for consistency?

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 2 months ago (2016-09-27 00:19:44 UTC) #46

Tom (Use chromium acct)

On 2016/09/26 21:13:29, piman OOO back 2016-09-26 wrote: > https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gpu_data_manager_impl_private.cc > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > ...

4 years, 2 months ago (2016-09-27 00:20:02 UTC) #47

On 2016/09/26 21:13:29, piman OOO back 2016-09-26 wrote:
>
https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/2347383002/diff/180001/content/browser/gpu/gp...
> content/browser/gpu/gpu_data_manager_impl_private.cc:1180:
> ui::XVisualManager::GetInstance()->OnGPUInfoChanged(
> On 2016/09/23 20:00:37, Tom Anderson wrote:
> > On 2016/09/22 21:24:54, piman OOO back 2016-09-12 wrote:
> > > This function is called on the IO thread (and sometimes on the UI thread).
> > IIUC,
> > > XVisualManager is intended to be called from the UI thread, so I think
this
> > > would cause some races. Maybe it would be better to add a
> > GpuDataManagerObserver
> > > to forward that info? observer_list_ is thread-safe, so the observer would
> be
> > > called on the same thread it's registered from (the UI thread if added
from
> > > BrowserMainLoop).
> > 
> > Done.  Added GpuDataManagerVisualProxy in browser_main_loop.
> > 
> > Although it still doesn't work with --single-process, and this is because
> > CollectGraphicsInfoGL never gets called.
> > 
> > I suppose we could check for BrowserMainLoop::UsingInProcessGpu(), and in
that
> > case, just use GLVisualPickerGLX directly since the GL stuff would be
loaded? 
> > Or is it even worth it to support this in --single-process?
> 
> I think I'm ok if we punt on --single-process.
> Directly using GLVisualPickerGLX may be a bit iffy because in
--single-process,
> the GPU main thread is a separate thread from the UI thread. Another way is to
> add a call to gpu::CollectContextGraphicsInfo in InProcessGpuThread and pass
the
> GpuInfo that to the in-process GpuChildThread constructor.
> 

Unfortunately there's some sort of issue with doing that
[28067:28110:0926/151750:FATAL:gpu_info_collector.cc(108)] Check failed:
gl::GetGLImplementation() != gl::kGLImplementationNone (0 vs. 0)
#0 0x7ff1a5e8c66e base::debug::StackTrace::StackTrace()
#1 0x7ff1a5efa74f logging::LogMessage::~LogMessage()
#2 0x7ff19e7669f2 gpu::CollectGraphicsInfoGL()
#3 0x7ff19e76a36c gpu::CollectContextGraphicsInfo()
#4 0x7ff19f35febb content::InProcessGpuThread::Init()
#5 0x7ff1a606bea4 base::Thread::ThreadMain()
#6 0x7ff1a605307a base::(anonymous namespace)::ThreadFunc()
#7 0x7ff1a62c0184 start_thread
#8 0x7ff1940ba37d clone

> https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_surface_egl.c...
> ui/gl/gl_surface_egl.cc:39: #include "ui/gl/gl_visual_picker_glx.h"
> On 2016/09/23 20:00:37, Tom Anderson wrote:
> > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > When we're using EGL, we are not using GLX, so I don't think this will
> work...
> > 
> > Done.  I opted to use XVisualManager instead.  Unfortunately this means I
also
> > needed to add a dependency on ui/base/x.  PLMK if this is not right
> 
> I think this is ok.
> 
>
https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
> File ui/gl/gl_visual_picker_glx.cc (right):
> 
>
https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
> ui/gl/gl_visual_picker_glx.cc:108: return PickBestGlVisual(filtered_visuals,
> IsRgbaVisual(default_visual_info));
> On 2016/09/23 20:00:38, Tom Anderson wrote:
> > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > For offscreen and opaque windows, the best visual is the root visual. Any
> > other
> > > visual will typically involve a copy on present, reducing performance.
> > > 
> > > Any reason not to go with that?
> > 
> > On my system there's a bunch of visuals that have identical XVisualInfo
> > properties as the default visual (indicated by xdpyinfo), but have different
> GL
> > properties (indicated by glxinfo).  Would this reduce performance if it's
only
> > the GL stuff that's different?  If so, and if we don't need any extra things
> > like double-buffer in the future, I'm fine with removing this.
> 
> It's hard to know without testing, in particular with closed-source drivers.
> Some of the things I can think of that could impact things are color space
(sRGB
> vs linear), buffer formats (float vs int), or the use of a colormap (which is
> necessary for different visuals), or internal things such as framebuffer
> compression.
> The best way to test is with a fill-rate limited example, and see the frame
> rate. The WebGL aquarium http://webglsamples.org/aquarium/aquarium.html may be
a
> good choice at high resolution.
> 

I should point out that right now, DWTHX11 tries to use an alpha visual for
windows that are TYPE_DRAG or TYPE_WINDOW (including browser windows).  This is
because normal windows need transparency for extensions (although it looks like
the transparency API is not actually usable at this point).  Maybe we should
only enable transparency for TYPE_DRAG, and for extension windows where we know
we will be needing transparency?  Maybe in a separate patch set?

I tried the aquarium sample and I was getting about 10fps using the default
visual and the alpha visual (on a debug build).

> One option is to try the root visual and see if it satisfies requirements, and
> if so pick that, otherwise pick the best match?
> 

done.  And I've verified that the check actually returns the root visual on my
system.

>
https://codereview.chromium.org/2347383002/diff/180001/ui/gl/gl_visual_picker...
> ui/gl/gl_visual_picker_glx.cc:146: std::remove_if(filtered_visuals.begin(),
> filtered_visuals.end(),
> On 2016/09/23 20:00:38, Tom Anderson wrote:
> > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > Note: remove_if doesn't remove anything, it just moves it to the end, so I
> > don't
> > > think you filter anything if you don't erase elements starting at the
> iterator
> > > returned by remove_if.
> > > 
> > > 
> > > Maybe it would be better to simply not copy it, i.e. by moving the
> > IsRgbaVisual
> > > check to the lambda inside the copy_if?
> > 
> > Done.  Nice catch, I think the reason it was returning the right visuals
> before
> > was because the selection algorithm in PickBestGlVisual happened to pick the
> > first visual with the highest score.
> 
> But you don't want to check inside the copy_if? It does seem a little simpler
> (and efficient fwiw).
> 

Done.  Sorry, I overlooked the second part of your comment

> https://codereview.chromium.org/2347383002/diff/220001/ui/gl/gl_share_group.cc
> File ui/gl/gl_share_group.cc (right):
> 
>
https://codereview.chromium.org/2347383002/diff/220001/ui/gl/gl_share_group.c...
> ui/gl/gl_share_group.cc:61: int fbconfig_id =
compatible->GetCompatibilityKey();
> nit: compatibility_key instead of fbconfig_id?
> Also, use unsigned long instead of int for consistency?

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/240001

4 years, 2 months ago (2016-09-27 00:20:41 UTC) #48

piman

On Mon, Sep 26, 2016 at 5:20 PM, <thomasanderson@google.com> wrote: > On 2016/09/26 21:13:29, piman ...

4 years, 2 months ago (2016-09-27 01:06:32 UTC) #49

On Mon, Sep 26, 2016 at 5:20 PM, <thomasanderson@google.com> wrote:

> On 2016/09/26 21:13:29, piman OOO back 2016-09-26 wrote:
> >
> https://codereview.chromium.org/2347383002/diff/180001/
> content/browser/gpu/gpu_data_manager_impl_private.cc
> > File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> >
> >
> https://codereview.chromium.org/2347383002/diff/180001/
> content/browser/gpu/gpu_data_manager_impl_private.cc#newcode1180
> > content/browser/gpu/gpu_data_manager_impl_private.cc:1180:
> > ui::XVisualManager::GetInstance()->OnGPUInfoChanged(
> > On 2016/09/23 20:00:37, Tom Anderson wrote:
> > > On 2016/09/22 21:24:54, piman OOO back 2016-09-12 wrote:
> > > > This function is called on the IO thread (and sometimes on the UI
> thread).
> > > IIUC,
> > > > XVisualManager is intended to be called from the UI thread, so I
> think
> this
> > > > would cause some races. Maybe it would be better to add a
> > > GpuDataManagerObserver
> > > > to forward that info? observer_list_ is thread-safe, so the observer
> would
> > be
> > > > called on the same thread it's registered from (the UI thread if
> added
> from
> > > > BrowserMainLoop).
> > >
> > > Done. Added GpuDataManagerVisualProxy in browser_main_loop.
> > >
> > > Although it still doesn't work with --single-process, and this is
> because
> > > CollectGraphicsInfoGL never gets called.
> > >
> > > I suppose we could check for BrowserMainLoop::UsingInProcessGpu(),
> and in
> that
> > > case, just use GLVisualPickerGLX directly since the GL stuff would be
> loaded?
> > > Or is it even worth it to support this in --single-process?
> >
> > I think I'm ok if we punt on --single-process.
> > Directly using GLVisualPickerGLX may be a bit iffy because in
> --single-process,
> > the GPU main thread is a separate thread from the UI thread. Another way
> is to
> > add a call to gpu::CollectContextGraphicsInfo in InProcessGpuThread and
> pass
> the
> > GpuInfo that to the in-process GpuChildThread constructor.
> >
>
> Unfortunately there's some sort of issue with doing that
> [28067:28110:0926/151750:FATAL:gpu_info_collector.cc(108)] Check failed:
> gl::GetGLImplementation() != gl::kGLImplementationNone (0 vs. 0)
> #0 0x7ff1a5e8c66e base::debug::StackTrace::StackTrace()
> #1 0x7ff1a5efa74f logging::LogMessage::~LogMessage()
> #2 0x7ff19e7669f2 gpu::CollectGraphicsInfoGL()
> #3 0x7ff19e76a36c gpu::CollectContextGraphicsInfo()
> #4 0x7ff19f35febb content::InProcessGpuThread::Init()
> #5 0x7ff1a606bea4 base::Thread::ThreadMain()
> #6 0x7ff1a605307a base::(anonymous namespace)::ThreadFunc()
> #7 0x7ff1a62c0184 start_thread
> #8 0x7ff1940ba37d clone
>

It's probably a matter of making sure GL is initialized early enough.
 gl::init::InitializeGLOneOff() is called in the in-process GpuChildThread
constructor, but it could move to InProcessGpuThread, before this query (or
vice versa, but I think this would be more consistent with the
out-of-process path).


>
> > https://codereview.chromium.org/2347383002/diff/180001/ui/
> gl/gl_surface_egl.cc
> > File ui/gl/gl_surface_egl.cc (right):
> >
> >
> https://codereview.chromium.org/2347383002/diff/180001/ui/
> gl/gl_surface_egl.cc#newcode39
> > ui/gl/gl_surface_egl.cc:39: #include "ui/gl/gl_visual_picker_glx.h"
> > On 2016/09/23 20:00:37, Tom Anderson wrote:
> > > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > > When we're using EGL, we are not using GLX, so I don't think this
> will
> > work...
> > >
> > > Done. I opted to use XVisualManager instead. Unfortunately this means I
> also
> > > needed to add a dependency on ui/base/x. PLMK if this is not right
> >
> > I think this is ok.
> >
> >
> https://codereview.chromium.org/2347383002/diff/180001/ui/
> gl/gl_visual_picker_glx.cc
> > File ui/gl/gl_visual_picker_glx.cc (right):
> >
> >
> https://codereview.chromium.org/2347383002/diff/180001/ui/
> gl/gl_visual_picker_glx.cc#newcode108
> > ui/gl/gl_visual_picker_glx.cc:108: return PickBestGlVisual(filtered_
> visuals,
> > IsRgbaVisual(default_visual_info));
> > On 2016/09/23 20:00:38, Tom Anderson wrote:
> > > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > > For offscreen and opaque windows, the best visual is the root
> visual. Any
> > > other
> > > > visual will typically involve a copy on present, reducing
> performance.
> > > >
> > > > Any reason not to go with that?
> > >
> > > On my system there's a bunch of visuals that have identical XVisualInfo
> > > properties as the default visual (indicated by xdpyinfo), but have
> different
> > GL
> > > properties (indicated by glxinfo). Would this reduce performance if
> it's
> only
> > > the GL stuff that's different? If so, and if we don't need any extra
> things
> > > like double-buffer in the future, I'm fine with removing this.
> >
> > It's hard to know without testing, in particular with closed-source
> drivers.
> > Some of the things I can think of that could impact things are color
> space
> (sRGB
> > vs linear), buffer formats (float vs int), or the use of a colormap
> (which is
> > necessary for different visuals), or internal things such as framebuffer
> > compression.
> > The best way to test is with a fill-rate limited example, and see the
> frame
> > rate. The WebGL aquarium http://webglsamples.org/aquarium/aquarium.html
> may be
> a
> > good choice at high resolution.
> >
>
> I should point out that right now, DWTHX11 tries to use an alpha visual for
> windows that are TYPE_DRAG or TYPE_WINDOW (including browser windows).
> This is
> because normal windows need transparency for extensions (although it looks
> like
> the transparency API is not actually usable at this point). Maybe we should
> only enable transparency for TYPE_DRAG, and for extension windows where we
> know
> we will be needing transparency? Maybe in a separate patch set?
>

I don't think we should be using the transparent visual by default for
browser windows, especially if the visual selection is racy. The argument
for allowing it on "drag" windows was because it's almost surely impossible
to create one before we had a chance to run the GPU process and display
some UI. But I'd be concerned the initial browser window gets created
before the GPU process is done initializing.
Also, using a transparent visual was certainly measured to introduce an
extra blit on NVIDIA GPUs.


> I tried the aquarium sample and I was getting about 10fps using the default
> visual and the alpha visual (on a debug build).
>

You need to use a release (or official) build - you should be getting
closer to 60, you're likely CPU-bound in debug.
It also likely only makes a difference when you don't use a compositing
manager (which introduces a copy either way).


>
>
> > One option is to try the root visual and see if it satisfies
> requirements, and
> > if so pick that, otherwise pick the best match?
> >
>
> done. And I've verified that the check actually returns the root visual on
> my
> system.
>
> >
> https://codereview.chromium.org/2347383002/diff/180001/ui/
> gl/gl_visual_picker_glx.cc#newcode146
> > ui/gl/gl_visual_picker_glx.cc:146: std::remove_if(filtered_
> visuals.begin(),
> > filtered_visuals.end(),
> > On 2016/09/23 20:00:38, Tom Anderson wrote:
> > > On 2016/09/22 21:24:55, piman OOO back 2016-09-12 wrote:
> > > > Note: remove_if doesn't remove anything, it just moves it to the
> end, so I
> > > don't
> > > > think you filter anything if you don't erase elements starting at the
> > iterator
> > > > returned by remove_if.
> > > >
> > > >
> > > > Maybe it would be better to simply not copy it, i.e. by moving the
> > > IsRgbaVisual
> > > > check to the lambda inside the copy_if?
> > >
> > > Done. Nice catch, I think the reason it was returning the right visuals
> > before
> > > was because the selection algorithm in PickBestGlVisual happened to
> pick the
> > > first visual with the highest score.
> >
> > But you don't want to check inside the copy_if? It does seem a little
> simpler
> > (and efficient fwiw).
> >
>
> Done. Sorry, I overlooked the second part of your comment
>
> > https://codereview.chromium.org/2347383002/diff/220001/ui/
> gl/gl_share_group.cc
> > File ui/gl/gl_share_group.cc (right):
> >
> >
> https://codereview.chromium.org/2347383002/diff/220001/ui/
> gl/gl_share_group.cc#newcode61
> > ui/gl/gl_share_group.cc:61: int fbconfig_id =
> compatible->GetCompatibilityKey();
> > nit: compatibility_key instead of fbconfig_id?
> > Also, use unsigned long instead of int for consistency?
>
> Done.
>
> https://codereview.chromium.org/2347383002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

calamity

extensions/browser/api/app_window/app_window_apitest.cc lgtm.

4 years, 2 months ago (2016-09-27 01:10:25 UTC) #50

Tom (Use chromium acct)

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 2 months ago (2016-09-27 02:10:51 UTC) #51

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-27 03:28:15 UTC) #52

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-27 03:28:16 UTC) #53

Tom (Use chromium acct)

On 2016/09/27 01:06:32, piman OOO back 2016-09-26 wrote: > > It's probably a matter of ...

4 years, 2 months ago (2016-09-27 18:57:31 UTC) #54

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 2 months ago (2016-09-27 18:57:40 UTC) #55

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/260001

4 years, 2 months ago (2016-09-27 18:58:08 UTC) #56

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-27 20:42:01 UTC) #58

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-27 20:42:02 UTC) #59

Tom (Use chromium acct)

Pinging sadrul@ and dcheng@ for sadrul: ui/base/x/x11_util.cc ui/base/x/x11_util_internal.h ui/base/x/x11_util_unittest.cc ui/gfx/x/x11_switches.cc ui/gfx/x/x11_switches.h ui/gfx/x/x11_types.h ui/views/test/views_test_base.cc ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc ...

4 years, 2 months ago (2016-09-27 22:36:58 UTC) #60

dcheng

https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h#newcode245 gpu/config/gpu_info.h:245: VisualID rgba_visual; Just to help me understand: the GPU ...

4 years, 2 months ago (2016-09-27 23:25:51 UTC) #61

Tom (Use chromium acct)

https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h#newcode245 gpu/config/gpu_info.h:245: VisualID rgba_visual; On 2016/09/27 23:25:51, dcheng wrote: > Just ...

4 years, 2 months ago (2016-09-27 23:47:49 UTC) #62

dcheng

On 2016/09/27 23:47:49, Tom Anderson wrote: > https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h > File gpu/config/gpu_info.h (right): > > https://codereview.chromium.org/2347383002/diff/260001/gpu/config/gpu_info.h#newcode245 ...

4 years, 2 months ago (2016-09-28 00:08:04 UTC) #63

sadrul

lgtm https://codereview.chromium.org/2347383002/diff/260001/ui/base/x/x11_util_internal.h File ui/base/x/x11_util_internal.h (right): https://codereview.chromium.org/2347383002/diff/260001/ui/base/x/x11_util_internal.h#newcode107 ui/base/x/x11_util_internal.h:107: bool have_gpu_argb_visual_; DISALLOW_COPY_AND_ASSIGN

4 years, 2 months ago (2016-09-28 03:56:32 UTC) #64

Julien Isorce Samsung

Hi Tom, thx a lot for all of it. FWIW, non-owner LGTM ! https://codereview.chromium.org/2347383002/diff/260001/content/browser/gpu/gpu_data_manager_impl_private.cc File ...

4 years, 2 months ago (2016-09-28 17:42:52 UTC) #65

Tom (Use chromium acct)

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 2 months ago (2016-09-28 18:14:57 UTC) #66

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 2 months ago (2016-09-28 18:35:01 UTC) #67

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/280001

4 years, 2 months ago (2016-09-28 18:35:21 UTC) #68

Tom (Use chromium acct)

https://codereview.chromium.org/2347383002/diff/260001/content/browser/gpu/gpu_data_manager_impl_private.cc File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2347383002/diff/260001/content/browser/gpu/gpu_data_manager_impl_private.cc#newcode56 content/browser/gpu/gpu_data_manager_impl_private.cc:56: On 2016/09/28 17:42:51, Julien Isorce wrote: > Left over ...

4 years, 2 months ago (2016-09-28 18:36:01 UTC) #69

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-28 19:57:49 UTC) #70

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-28 19:57:51 UTC) #71

dcheng

https://codereview.chromium.org/2347383002/diff/280001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/2347383002/diff/280001/ui/base/x/x11_util.cc#newcode1492 ui/base/x/x11_util.cc:1492: if (system_visual_id && visuals_.find(system_visual_id) != visuals_.end()) Would you ever ...

4 years, 2 months ago (2016-09-28 22:16:46 UTC) #72

dcheng

+rockot for real I talked with rockot@ offline, and he mentioned that nullable substructures might ...

4 years, 2 months ago (2016-09-28 23:12:11 UTC) #73

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 23:23:36 UTC) #74

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/300001

4 years, 2 months ago (2016-09-29 23:23:59 UTC) #75

Tom (Use chromium acct)

dcheng@ PTAL at the deltas On 2016/09/28 22:16:46, dcheng wrote: > https://codereview.chromium.org/2347383002/diff/280001/ui/base/x/x11_util.cc > File ui/base/x/x11_util.cc ...

4 years, 2 months ago (2016-09-29 23:24:49 UTC) #76

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-30 01:09:45 UTC) #77

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-30 01:09:46 UTC) #78

dcheng

ipc changes lgtm, thanks https://codereview.chromium.org/2347383002/diff/300001/content/browser/browser_main_loop.cc File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2347383002/diff/300001/content/browser/browser_main_loop.cc#newcode390 content/browser/browser_main_loop.cc:390: auto gpu_process_host = GpuProcessHost::Get( Nit: ...

4 years, 2 months ago (2016-09-30 02:40:49 UTC) #79

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 2 months ago (2016-09-30 18:31:54 UTC) #80

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/320001

4 years, 2 months ago (2016-09-30 18:33:16 UTC) #81

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-30 22:40:33 UTC) #82

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-30 22:40:36 UTC) #83

Tom (Use chromium acct)

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 2 months ago (2016-09-30 23:45:49 UTC) #84

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com

4 years, 2 months ago (2016-09-30 23:45:58 UTC) #85

Tom (Use chromium acct)

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, calamity@chromium.org, j.isorce@samsung.com, sadrul@chromium.org, ...

4 years, 2 months ago (2016-09-30 23:45:58 UTC) #86

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2347383002/320001

4 years, 2 months ago (2016-09-30 23:46:48 UTC) #87

commit-bot: I haz the power

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 2 months ago (2016-10-01 02:04:01 UTC) #88

Message was sent while issue was closed.

Description was changed from

==========
X11: Use a better visual for OpenGL

This CL delegates picking a transparent visual to the GPU
process.  Previously, the browser process did not have enough
information to decide which visual to use because it couldn't get
GL-specific visual information directly.  On Nvidia drivers,
picking the wrong transparent visual made the browser unusable.

Main changes introduced:
* Remove command line arguments window-depth, x11-visual-id, and 
  disable_transparent_visuals.
* Remove driver bug DISABLE_TRANSPARENT_VISUALS
* GPU process picks an ARGB visual and a system visual (which may 
  be different from the default visual) and sends it back in GPUInfo

BUG=347333,369209,640170,640170
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
X11: Use a better visual for OpenGL

This CL delegates picking a transparent visual to the GPU
process.  Previously, the browser process did not have enough
information to decide which visual to use because it couldn't get
GL-specific visual information directly.  On Nvidia drivers,
picking the wrong transparent visual made the browser unusable.

Main changes introduced:
* Remove command line arguments window-depth, x11-visual-id, and 
  disable_transparent_visuals.
* Remove driver bug DISABLE_TRANSPARENT_VISUALS
* GPU process picks an ARGB visual and a system visual (which may 
  be different from the default visual) and sends it back in GPUInfo

BUG=347333,369209,640170,640170
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

commit-bot: I haz the power

Description was changed from ========== X11: Use a better visual for OpenGL This CL delegates ...

4 years, 2 months ago (2016-10-01 02:06:30 UTC) #90

Message was sent while issue was closed.

Description was changed from

==========
X11: Use a better visual for OpenGL

This CL delegates picking a transparent visual to the GPU
process.  Previously, the browser process did not have enough
information to decide which visual to use because it couldn't get
GL-specific visual information directly.  On Nvidia drivers,
picking the wrong transparent visual made the browser unusable.

Main changes introduced:
* Remove command line arguments window-depth, x11-visual-id, and 
  disable_transparent_visuals.
* Remove driver bug DISABLE_TRANSPARENT_VISUALS
* GPU process picks an ARGB visual and a system visual (which may 
  be different from the default visual) and sends it back in GPUInfo

BUG=347333,369209,640170,640170
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
X11: Use a better visual for OpenGL

This CL delegates picking a transparent visual to the GPU
process.  Previously, the browser process did not have enough
information to decide which visual to use because it couldn't get
GL-specific visual information directly.  On Nvidia drivers,
picking the wrong transparent visual made the browser unusable.

Main changes introduced:
* Remove command line arguments window-depth, x11-visual-id, and
  disable_transparent_visuals.
* Remove driver bug DISABLE_TRANSPARENT_VISUALS
* GPU process picks an ARGB visual and a system visual (which may
  be different from the default visual) and sends it back in GPUInfo

BUG=347333,369209,640170,640170
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/62ba78ffcdf525eb9ed640724e640fcf22fbbf87
Cr-Commit-Position: refs/heads/master@{#422274}
==========

commit-bot: I haz the power

Patchset 13 (id:??) landed as https://crrev.com/62ba78ffcdf525eb9ed640724e640fcf22fbbf87 Cr-Commit-Position: refs/heads/master@{#422274}

4 years, 2 months ago (2016-10-01 02:06:33 UTC) #91

hubbe

hubbe@chromium.org changed reviewers: + hubbe@chromium.org

4 years, 2 months ago (2016-10-03 19:57:30 UTC) #92

hubbe

This CL means that chrome doesn't work on my desktop anymore. I do not have ...

4 years, 2 months ago (2016-10-03 19:57:31 UTC) #93

hubbe

On 2016/10/03 19:57:31, hubbe wrote: > This CL means that chrome doesn't work on my ...

4 years, 2 months ago (2016-10-03 19:58:10 UTC) #94

Message was sent while issue was closed.

On 2016/10/03 19:57:31, hubbe wrote:
> This CL means that chrome doesn't work on my desktop anymore.
> I do not have any transparent visuals because my desktop is configured for
> 10-bit output. Please make this work or revert this CL.

This is the error I get:
[23547:23547:1003/125357:FATAL:x11_util.cc(1460)] Check failed:
transparent_visual_id_. 
#0 0x7fab2f28bd2e base::debug::StackTrace::StackTrace()
#1 0x7fab2f2f9b8f logging::LogMessage::~LogMessage()
#2 0x7fab2c7b28f8 ui::XVisualManager::XVisualManager()
#3 0x7fab2c7c1770 base::DefaultSingletonTraits<>::New()
#4 0x7fab2c7b8006 base::Singleton<>::get()
#5 0x7fab2c7b21f9 ui::XVisualManager::GetInstance()
#6 0x7fab233f0d2d views::DesktopWindowTreeHostX11::InitX11Window()
#7 0x7fab233f0a5e views::DesktopWindowTreeHostX11::Init()
#8 0x7fab33d2623e BrowserDesktopWindowTreeHostX11::Init()
#9 0x7fab233d4f47 views::DesktopNativeWidgetAura::InitNativeWidget()
#10 0x7fab33d258a9 DesktopBrowserFrameAura::InitNativeWidget()
#11 0x7fab2337f578 views::Widget::Init()
#12 0x7fab33c06b3e BrowserFrame::InitBrowserFrame()
#13 0x7fab3390ebe1 BrowserWindow::CreateBrowserWindow()
#14 0x7fab337945e5 (anonymous namespace)::CreateBrowserWindow()
#15 0x7fab33793f01 Browser::Browser()
#16 0x7fab337f0e3f StartupBrowserCreatorImpl::OpenTabsInBrowser()
#17 0x7fab337f0055 StartupBrowserCreatorImpl::ProcessSpecifiedURLs()
#18 0x7fab337efb4e StartupBrowserCreatorImpl::ProcessStartupURLs()
#19 0x7fab337eee93 StartupBrowserCreatorImpl::ProcessLaunchURLs()
#20 0x7fab337ee5f8 StartupBrowserCreatorImpl::Launch()
#21 0x7fab337e8aa5 StartupBrowserCreator::LaunchBrowser()
#22 0x7fab337e80a9 StartupBrowserCreator::ProcessCmdLineImpl()
#23 0x7fab337e7462 StartupBrowserCreator::Start()
#24 0x7fab319ab660 ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#25 0x7fab319aa2f0 ChromeBrowserMainParts::PreMainMessageLoopRun()
#26 0x7fab29160e81 content::BrowserMainLoop::PreMainMessageLoopRun()
#27 0x7fab288060b9
_ZN4base8internal13FunctorTraitsIMN7content12ChildProcessEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_
#28 0x7fab291691a1
_ZN4base8internal12InvokeHelperILb0EiE8MakeItSoIRKMN7content15BrowserMainLoopEFivEJPS5_EEEiOT_DpOT0_
#29 0x7fab29169147
_ZN4base8internal7InvokerINS0_9BindStateIMN7content15BrowserMainLoopEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEiOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#30 0x7fab2916908c
_ZN4base8internal7InvokerINS0_9BindStateIMN7content15BrowserMainLoopEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE3RunEPNS0_13BindStateBaseE
#31 0x7fab286768fb base::internal::RunMixin<>::Run()
#32 0x7fab29ca515b content::StartupTaskRunner::RunAllTasksNow()
#33 0x7fab2915ecb4 content::BrowserMainLoop::CreateStartupTasks()
#34 0x7fab2916c15d content::BrowserMainRunnerImpl::Initialize()
#35 0x7fab2915b22f content::BrowserMain()
#36 0x7fab2afb3156 content::RunNamedProcessTypeMain()
#37 0x7fab2afb55e2 content::ContentMainRunnerImpl::Run()
#38 0x7fab2afb21f2 content::ContentMain()
#39 0x7fab301146bb ChromeMain
#40 0x7fab30114652 main
#41 0x7fab1d103f45 __libc_start_main
#42 0x7fab30114555 <unknown>

Tom (Use chromium acct)

4 years, 2 months ago (2016-10-03 20:09:56 UTC) #95

Message was sent while issue was closed.

On 2016/10/03 19:57:31, hubbe wrote:
> This CL means that chrome doesn't work on my desktop anymore.
> I do not have any transparent visuals because my desktop is configured for
> 10-bit output. Please make this work or revert this CL.

Speculative fix here: https://codereview.chromium.org/2386313002/
hubbe@ Can you try applying locally and see if that fixes it?

Issue 2347383002: X11: Use better visuals for OpenGL (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix hang #

Patch Set 3 : IPC works #

Patch Set 4 : Moved visual selection code into its own file #

Patch Set 5 : Refactor #

Patch Set 6 : Fix various tests #

Patch Set 7 : Remove GLShareGroupGLX, add GpuDataManagerVisualProxy, refactor #

Patch Set 8 : Rebase #

Patch Set 9 : Try to return root visual #

Patch Set 10 : Fix --single-process #

Patch Set 11 : Final comments #

Patch Set 12 : Shutdown a misbehaving GPU process #

Patch Set 13 : auto* #

Messages