content/gpu/gpu_child_thread.cc - Issue 1716813002: Use GpuPreferences to avoid directly accessing switches in gpu related code

Unified Diff: content/gpu/gpu_child_thread.cc

Issue 1716813002: Use GpuPreferences to avoid directly accessing switches in gpu related code (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Update Created 4 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: content/gpu/gpu_child_thread.cc

diff --git a/content/gpu/gpu_child_thread.cc b/content/gpu/gpu_child_thread.cc

index 339351f492bd95f0ad64ec3e08df60ac7cb4bd7e..23e9d77032ff2e0d1aaf1ad1d774d8c762dd3ffc 100644

--- a/content/gpu/gpu_child_thread.cc

+++ b/content/gpu/gpu_child_thread.cc

@@ -9,6 +9,7 @@

#include "base/bind.h"

#include "base/lazy_instance.h"

+#include "base/strings/string_number_conversions.h"

#include "base/threading/worker_pool.h"

#include "build/build_config.h"

#include "content/child/child_process.h"

@@ -22,10 +23,14 @@

#include "content/public/common/content_client.h"

#include "content/public/common/content_switches.h"

#include "content/public/gpu/content_gpu_client.h"

+#include "gpu/command_buffer/service/gpu_switches.h"

#include "gpu/config/gpu_info_collector.h"

+#include "gpu/config/gpu_switches.h"

+#include "gpu/config/gpu_util.h"

#include "ipc/ipc_channel_handle.h"

#include "ipc/ipc_sync_message_filter.h"

#include "ui/gl/gl_implementation.h"

+#include "ui/gl/gl_switches.h"

#include "ui/gl/gpu_switching_manager.h"

#if defined(USE_OZONE)

@@ -39,6 +44,79 @@ namespace {

static base::LazyInstance<scoped_refptr<ThreadSafeSender> >

g_thread_safe_sender = LAZY_INSTANCE_INITIALIZER;

+bool GetSizeTFromSwitch(const base::CommandLine* command_line,

+ const base::StringPiece& switch_string,

+ size_t* value) {

+ if (!command_line->HasSwitch(switch_string))

+ return false;

+ std::string switch_value(command_line->GetSwitchValueASCII(switch_string));

+ return base::StringToSizeT(switch_value, value);

+gpu::GpuPreferences GetGpuPreferencesFromCommandLine() {

+ // TODO(penghuang): share below code with

+ // android_webview/browser/deferred_gpu_command_service.cc

+ // For any modification of below code, deferred_gpu_command_service.cc should

+ // be updated as well.

+ DCHECK(base::CommandLine::InitializedForCurrentProcess());

+ const base::CommandLine* command_line =

+ base::CommandLine::ForCurrentProcess();

+ gpu::GpuPreferences gpu_preferences;

+ gpu_preferences.single_process =

+ command_line->HasSwitch(switches::kSingleProcess);

+ gpu_preferences.in_process_gpu =

+ command_line->HasSwitch(switches::kInProcessGPU);

+ gpu_preferences.ui_prioritize_in_gpu_process =

+ command_line->HasSwitch(switches::kUIPrioritizeInGpuProcess);

+ gpu_preferences.compile_shader_always_succeeds =

+ command_line->HasSwitch(switches::kCompileShaderAlwaysSucceeds);

+ gpu_preferences.disable_gl_error_limit =

+ command_line->HasSwitch(switches::kDisableGLErrorLimit);

+ gpu_preferences.disable_glsl_translator =

+ command_line->HasSwitch(switches::kDisableGLSLTranslator);

+ gpu_preferences.disable_gpu_driver_bug_workarounds =

+ command_line->HasSwitch(switches::kDisableGpuDriverBugWorkarounds);

+ gpu_preferences.disable_shader_name_hashing =

+ command_line->HasSwitch(switches::kDisableShaderNameHashing);

+ gpu_preferences.enable_gpu_command_logging =

+ command_line->HasSwitch(switches::kEnableGPUCommandLogging);

+ gpu_preferences.enable_gpu_debugging =

+ command_line->HasSwitch(switches::kEnableGPUDebugging);

+ gpu_preferences.enable_gpu_service_logging_gpu =

+ command_line->HasSwitch(switches::kEnableGPUServiceLoggingGPU);

+ gpu_preferences.disable_gpu_program_cache =

+ command_line->HasSwitch(switches::kDisableGpuProgramCache);

+ gpu_preferences.enforce_gl_minimums =

+ command_line->HasSwitch(switches::kEnforceGLMinimums);

+ if (GetSizeTFromSwitch(command_line, switches::kForceGpuMemAvailableMb,

+ &gpu_preferences.force_gpu_mem_available)) {

+ gpu_preferences.force_gpu_mem_available *= 1024 * 1024;

+ }

+ if (GetSizeTFromSwitch(command_line, switches::kGpuProgramCacheSizeKb,

+ &gpu_preferences.gpu_program_cache_size)) {

+ gpu_preferences.gpu_program_cache_size *= 1024;

+ }

+ gpu_preferences.enable_share_group_async_texture_upload =

+ command_line->HasSwitch(switches::kEnableShareGroupAsyncTextureUpload);

+ gpu_preferences.enable_subscribe_uniform_extension =

+ command_line->HasSwitch(switches::kEnableSubscribeUniformExtension);

+ gpu_preferences.enable_threaded_texture_mailboxes =

+ command_line->HasSwitch(switches::kEnableThreadedTextureMailboxes);

+ gpu_preferences.gl_shader_interm_output =

+ command_line->HasSwitch(switches::kGLShaderIntermOutput);

+ gpu_preferences.emulate_shader_precision =

+ command_line->HasSwitch(switches::kEmulateShaderPrecision);

+ gpu_preferences.enable_gl_path_rendering =

+ command_line->HasSwitch(switches::kEnableGLPathRendering);

+ gpu_preferences.enable_gpu_service_logging =

+ command_line->HasSwitch(switches::kEnableGPUServiceLogging);

+ gpu_preferences.enable_gpu_service_tracing =

+ command_line->HasSwitch(switches::kEnableGPUServiceTracing);

+ gpu_preferences.enable_unsafe_es3_apis =

+ command_line->HasSwitch(switches::kEnableUnsafeES3APIs);

+ return gpu_preferences;

bool GpuProcessLogMessageHandler(int severity,

const char* file, int line,

size_t message_start,

@@ -135,6 +213,7 @@ ChildThreadImpl::Options GetOptions(

} // namespace

GpuChildThread::GpuChildThread(

+ const gpu::GpuPreferences* gpu_preferences,

GpuWatchdogThread* watchdog_thread,

bool dead_on_arrival,

const gpu::GPUInfo& gpu_info,

@@ -142,6 +221,8 @@ GpuChildThread::GpuChildThread(

GpuMemoryBufferFactory* gpu_memory_buffer_factory,

gpu::SyncPointManager* sync_point_manager)

: ChildThreadImpl(GetOptions(gpu_memory_buffer_factory)),

+ gpu_preferences_(gpu_preferences ?

+ *gpu_preferences : GetGpuPreferencesFromCommandLine()),

boliu 2016/02/26 22:17:32 Don't do this. Either make the caller construct a

Peng 2016/02/29 16:09:22 Removed the gpu_preferences for this constructor.

boliu 2016/02/29 16:26:40 const* is generally against the style guide. Eithe

Peng 2016/02/29 19:29:50 https://google.github.io/styleguide/cppguide.html#

https://google.github.io/styleguide/cppguide.html#Reference_Arguments Base on google code style. non-const references is disallowed, but const pointer is OK.

piman@ LGTMed the duplicate code in DeferredGpuCommandService & GpuChildThread (I think he doesn't object it as a short term solution, but he is OOO, I cannot confirm it right now). In followup CLs, we will get rid of gpu related switches by sending gpu::GpuPreferences from browser to gpu service. It will remove those duplicate code. I filed a new issue for it(http://crbug.com/590825), and added it in the TODO inline comment.

boliu 2016/02/29 19:48:22 My earlier point was that some of these switches s

On 2016/02/29 19:29:50, Peng wrote: > On 2016/02/29 16:26:40, boliu wrote: > > On 2016/02/29 16:09:22, Peng wrote: > > > On 2016/02/26 22:17:32, boliu wrote: > > > > Don't do this. Either make the caller construct a GpuPreferences, or don't > > > have > > > > the parameter at all. > > > > > > Removed the gpu_preferences for this constructor. But for the other > > > constructor, may I ask why we cannot use a pointer there? > > > > const* is generally against the style guide. Either it should be const&, or > just > > non-const* for an output parameter. > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > Base on google code style. non-const references is disallowed, but const pointer > is OK. > > > > > > The GpuChildThread are > > > created in several places. To modify there places to supply a GpuPreferences > > is > > > inconvenient. That is why piman@ suggested we put the GpuPreference and > parse > > > the command line in this class. > > > > > > > > > > > > > > > > > Again, this suggests once again there should not be two copies of > > > > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the > public > > > > content API? > > > > > > I would like to put GetGpuPreferencesFromCommandLine() in content/public, > but > > it > > > has a little problem. Seems content/public/common are used by browser, gpu > and > > > renderer processes, but this function uses gpu::GpuPreferences and it is in > > the > > > gpu/command_buffer/service . I believe gpu service code should not be used > by > > > renderer and browser processes indirectly, right? Any suggestion? > > > > Webview only needs it from browser side, so the public API only needs to be in > > content/public/browser. But inside content, it's needed from child as well. I > > don't know if there is any precedence for something like that. Ask piman or > > another content owner maybe? > > piman@ LGTMed the duplicate code in DeferredGpuCommandService & GpuChildThread > (I think he doesn't object it as a short term solution, but he is OOO, I cannot > confirm it right now). In followup CLs, we will get rid of gpu related switches > by sending gpu::GpuPreferences from browser to gpu service. It will remove those > duplicate code.

My earlier point was that some of these switches should not be removed. How are you going to deal with the duplicated code then?

Peng 2016/02/29 20:30:49 If some switches cannot be removed, we will only r

On 2016/02/29 19:48:22, boliu wrote: > On 2016/02/29 19:29:50, Peng wrote: > > On 2016/02/29 16:26:40, boliu wrote: > > > On 2016/02/29 16:09:22, Peng wrote: > > > > On 2016/02/26 22:17:32, boliu wrote: > > > > > Don't do this. Either make the caller construct a GpuPreferences, or > don't > > > > have > > > > > the parameter at all. > > > > > > > > Removed the gpu_preferences for this constructor. But for the other > > > > constructor, may I ask why we cannot use a pointer there? > > > > > > const* is generally against the style guide. Either it should be const&, or > > just > > > non-const* for an output parameter. > > > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > > > Base on google code style. non-const references is disallowed, but const > pointer > > is OK. > > > > > > > > > The GpuChildThread are > > > > created in several places. To modify there places to supply a > GpuPreferences > > > is > > > > inconvenient. That is why piman@ suggested we put the GpuPreference and > > parse > > > > the command line in this class. > > > > > > > > > > > > > > > > > > > > > > Again, this suggests once again there should not be two copies of > > > > > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the > > public > > > > > content API? > > > > > > > > I would like to put GetGpuPreferencesFromCommandLine() in content/public, > > but > > > it > > > > has a little problem. Seems content/public/common are used by browser, gpu > > and > > > > renderer processes, but this function uses gpu::GpuPreferences and it is > in > > > the > > > > gpu/command_buffer/service . I believe gpu service code should not be used > > by > > > > renderer and browser processes indirectly, right? Any suggestion? > > > > > > Webview only needs it from browser side, so the public API only needs to be > in > > > content/public/browser. But inside content, it's needed from child as well. > I > > > don't know if there is any precedence for something like that. Ask piman or > > > another content owner maybe? > > > > piman@ LGTMed the duplicate code in DeferredGpuCommandService & GpuChildThread > > (I think he doesn't object it as a short term solution, but he is OOO, I > cannot > > confirm it right now). In followup CLs, we will get rid of gpu related > switches > > by sending gpu::GpuPreferences from browser to gpu service. It will remove > those > > duplicate code. > > My earlier point was that some of these switches should not be removed. How are > you going to deal with the duplicated code then? > > > I filed a new issue for it(http://crbug.com/590825), and added it in the TODO > > inline comment. > > >

If some switches cannot be removed, we will only removed them from gpu process's command line, and keep them for browser process's command line, so we will only parse command line in browser code (process), and pass the preferences to gpu service code (process) by IPC or a parameter.

dead_on_arrival_(dead_on_arrival),

sync_point_manager_(sync_point_manager),

gpu_info_(gpu_info),

@@ -156,6 +237,7 @@ GpuChildThread::GpuChildThread(

}

GpuChildThread::GpuChildThread(

+ const gpu::GpuPreferences* gpu_preferences,

const InProcessChildThreadParams& params,

GpuMemoryBufferFactory* gpu_memory_buffer_factory,

gpu::SyncPointManager* sync_point_manager)

@@ -164,6 +246,8 @@ GpuChildThread::GpuChildThread(

.AddStartupFilter(new GpuMemoryBufferMessageFilter(

gpu_memory_buffer_factory))

.Build()),

+ gpu_preferences_(gpu_preferences ?

+ *gpu_preferences : GetGpuPreferencesFromCommandLine()),

boliu 2016/02/26 22:17:32 ditto

dead_on_arrival_(false),

sync_point_manager_(sync_point_manager),

in_browser_process_(true),

@@ -357,11 +441,12 @@ void GpuChildThread::OnInitialize() {

// Defer creation of the render thread. This is to prevent it from handling

// IPC messages before the sandbox has been enabled and all other necessary

// initialization has succeeded.

- gpu_channel_manager_.reset(new GpuChannelManager(

- this, watchdog_thread_.get(), base::ThreadTaskRunnerHandle::Get().get(),

- ChildProcess::current()->io_task_runner(),

- ChildProcess::current()->GetShutDownEvent(), sync_point_manager_,

- gpu_memory_buffer_factory_));

+ gpu_channel_manager_.reset(

+ new GpuChannelManager(gpu_preferences_, this, watchdog_thread_.get(),

+ base::ThreadTaskRunnerHandle::Get().get(),

+ ChildProcess::current()->io_task_runner(),

+ ChildProcess::current()->GetShutDownEvent(),

+ sync_point_manager_, gpu_memory_buffer_factory_));

#if defined(USE_OZONE)

ui::OzonePlatform::GetInstance()

« content/gpu/gpu_child_thread.h ('K') | « content/gpu/gpu_child_thread.h ('k') | content/gpu/gpu_main.cc » ('j') | content/gpu/gpu_main.cc » ('J')