From 2d9de41157f132b521b3acaa906a2256d451fda9 Mon Sep 17 00:00:00 2001 From: "sievers@google.com" Date: Fri, 27 May 2011 03:18:36 +0000 Subject: Fix gpu acceleration with --in-process-gpu and --single-process modes. With recent changes that have moved gpu message handling in the browser to the IO thread (and moved the handling of messages between gpu and renderer, that are mediated by the browser, to GpuProcessHost), the routing for such messages was broken when running the gpu thread (rather than process). The new approach is to always instantiate GpuProcessHost (even when running a gpu thread only) and have a real IPC channel between host and gpu thread. This makes the 'in-process' GPU code work similar to what the renderer does when running --single-process. Note that --single-process mode is potentially still a bit fragile with this, since ChildProcess and ChildThread are currently written to only allow a single static instance in one process (it would be better to instantiate GpuProcess and RenderProcess simultaneously), so ambiguous calls to access e.g. the main thread are possible. Review URL: http://codereview.chromium.org/7054005 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86958 0039d316-1c4b-4281-b951-d872f2087c98 --- content/browser/browser_thread.cc | 1 - content/browser/browser_thread.h | 3 - content/browser/gpu/gpu_process_host.cc | 113 ++++++++++++++++++------ content/browser/gpu/gpu_process_host.h | 8 ++ content/browser/gpu/gpu_process_host_ui_shim.cc | 86 +----------------- content/browser/gpu/gpu_process_host_ui_shim.h | 14 +-- content/gpu/gpu_child_thread.cc | 6 +- 7 files changed, 103 insertions(+), 128 deletions(-) (limited to 'content') diff --git a/content/browser/browser_thread.cc b/content/browser/browser_thread.cc index 3809989..31b7e43 100644 --- a/content/browser/browser_thread.cc +++ b/content/browser/browser_thread.cc @@ -17,7 +17,6 @@ static const char* browser_thread_names[BrowserThread::ID_COUNT] = { "Chrome_ProcessLauncherThread", // PROCESS_LAUNCHER "Chrome_CacheThread", // CACHE "Chrome_IOThread", // IO - "Chrome_GpuThread", // GPU #if defined(USE_X11) "Chrome_Background_X11Thread", // BACKGROUND_X11 #endif diff --git a/content/browser/browser_thread.h b/content/browser/browser_thread.h index c226551..3da4012c9 100644 --- a/content/browser/browser_thread.h +++ b/content/browser/browser_thread.h @@ -67,9 +67,6 @@ class BrowserThread : public base::Thread { // This is the thread that processes IPC and network messages. IO, - // This thread issues calls to the GPU in the browser process. - GPU, - #if defined(USE_X11) // This thread has a second connection to the X server and is used to // process UI requests when routing the request to the UI thread would risk diff --git a/content/browser/gpu/gpu_process_host.cc b/content/browser/gpu/gpu_process_host.cc index 3cc8b31..0bc414e 100644 --- a/content/browser/gpu/gpu_process_host.cc +++ b/content/browser/gpu/gpu_process_host.cc @@ -10,6 +10,7 @@ #include "base/metrics/histogram.h" #include "base/process_util.h" #include "base/string_piece.h" +#include "base/threading/thread.h" #include "chrome/browser/tab_contents/render_view_host_delegate_helper.h" #include "content/browser/browser_thread.h" #include "content/browser/gpu/gpu_data_manager.h" @@ -18,6 +19,8 @@ #include "content/browser/renderer_host/render_widget_host_view.h" #include "content/common/content_switches.h" #include "content/common/gpu/gpu_messages.h" +#include "content/gpu/gpu_child_thread.h" +#include "content/gpu/gpu_process.h" #include "ipc/ipc_channel_handle.h" #include "ipc/ipc_switches.h" #include "media/base/media_switches.h" @@ -114,6 +117,51 @@ GpuProcessHost::SurfaceRef::~SurfaceRef() { } #endif // defined(OS_LINUX) +// This class creates a GPU thread (instead of a GPU process), when running +// with --in-process-gpu or --single-process. +class GpuMainThread : public base::Thread { + public: + explicit GpuMainThread(const std::string& channel_id) + : base::Thread("Chrome_InProcGpuThread"), + channel_id_(channel_id), + gpu_process_(NULL), + child_thread_(NULL) { + } + + ~GpuMainThread() { + Stop(); + } + + protected: + virtual void Init() { + // TODO: Currently, ChildProcess supports only a single static instance, + // which is a problem in --single-process mode, where both gpu and renderer + // should be able to create separate instances. + if (GpuProcess::current()) { + child_thread_ = new GpuChildThread(channel_id_); + } else { + gpu_process_ = new GpuProcess(); + // The process object takes ownership of the thread object, so do not + // save and delete the pointer. + gpu_process_->set_main_thread(new GpuChildThread(channel_id_)); + } + } + + virtual void CleanUp() { + delete gpu_process_; + if (child_thread_) + delete child_thread_; + } + + private: + std::string channel_id_; + // Deleted in CleanUp() on the gpu thread, so don't use smart pointers. + GpuProcess* gpu_process_; + GpuChildThread* child_thread_; + + DISALLOW_COPY_AND_ASSIGN(GpuMainThread); +}; + // static GpuProcessHost* GpuProcessHost::GetForRenderer( int renderer_id, content::CauseForGpuLaunch cause) { @@ -136,28 +184,6 @@ GpuProcessHost* GpuProcessHost::GetForRenderer( return NULL; int host_id; - /* TODO(apatrick): this is currently broken because this function is called on - the IO thread from GpuMessageFilter, and we don't have an IO thread object - when running the GPU code in the browser at the moment. - if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess) || - CommandLine::ForCurrentProcess()->HasSwitch(switches::kInProcessGPU)) { - if (!g_browser_process->gpu_thread()) - return NULL; - - // Initialize GL on the GPU thread. - // TODO(apatrick): Handle failure to initialize (asynchronously). - if (!BrowserThread::PostTask( - BrowserThread::GPU, - FROM_HERE, - NewRunnableFunction(&gfx::GLContext::InitializeOneOff))) { - return NULL; - } - - host_id = 0; - } else { - host_id = ++g_last_host_id; - } - */ host_id = ++g_last_host_id; UMA_HISTOGRAM_ENUMERATION("GPU.GPUProcessLaunchCause", @@ -195,10 +221,17 @@ GpuProcessHost* GpuProcessHost::FromID(int host_id) { GpuProcessHost::GpuProcessHost(int host_id) : BrowserChildProcessHost(GPU_PROCESS), host_id_(host_id), - gpu_process_(base::kNullProcessHandle) { + gpu_process_(base::kNullProcessHandle), + in_process_(false) { + if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess) || + CommandLine::ForCurrentProcess()->HasSwitch(switches::kInProcessGPU)) + in_process_ = true; + + // If the 'single GPU process' policy ever changes, we still want to maintain + // it for 'gpu thread' mode and only create one instance of host and thread. + DCHECK(!in_process_ || g_hosts_by_id.IsEmpty()); + g_hosts_by_id.AddWithID(this, host_id_); - if (host_id == 0) - gpu_process_ = base::GetCurrentProcessHandle(); // Post a task to create the corresponding GpuProcessHostUIShim. The // GpuProcessHostUIShim will be destroyed if either the browser exits, @@ -236,7 +269,27 @@ bool GpuProcessHost::Init() { if (!CreateChannel()) return false; - if (!LaunchGpuProcess()) + if (in_process_) { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kDisableGpuWatchdog); + + in_process_gpu_thread_.reset(new GpuMainThread(channel_id())); + + base::Thread::Options options; +#if defined(OS_WIN) + // On Windows the GPU thread needs to pump the compositor child window's + // message loop. TODO(apatrick): make this an IO thread if / when we get rid + // of this child window. Unfortunately it might always be necessary for + // Windows XP because we cannot share the backing store textures between + // processes. + options.message_loop_type = MessageLoop::TYPE_UI; +#else + options.message_loop_type = MessageLoop::TYPE_IO; +#endif + in_process_gpu_thread_->StartWithOptions(options); + + OnProcessLaunched(); // Fake a callback that the process is ready. + } else if (!LaunchGpuProcess()) return false; return Send(new GpuMsg_Initialize()); @@ -426,16 +479,20 @@ void GpuProcessHost::OnProcessLaunched() { // Send the GPU process handle to the UI thread before it has to // respond to any requests to establish a GPU channel. The response // to such requests require that the GPU process handle be known. + + base::ProcessHandle child_handle = in_process_ ? + base::GetCurrentProcessHandle() : handle(); + #if defined(OS_WIN) DuplicateHandle(base::GetCurrentProcessHandle(), - handle(), + child_handle, base::GetCurrentProcessHandle(), &gpu_process_, PROCESS_DUP_HANDLE, FALSE, 0); #else - gpu_process_ = handle(); + gpu_process_ = child_handle; #endif } diff --git a/content/browser/gpu/gpu_process_host.h b/content/browser/gpu/gpu_process_host.h index abf7280..c7c7df7 100644 --- a/content/browser/gpu/gpu_process_host.h +++ b/content/browser/gpu/gpu_process_host.h @@ -23,6 +23,8 @@ namespace IPC { class Message; } +class GpuMainThread; + class GpuProcessHost : public BrowserChildProcessHost, public base::NonThreadSafe { public: @@ -151,6 +153,12 @@ class GpuProcessHost : public BrowserChildProcessHost, // The handle for the GPU process or null if it is not known to be launched. base::ProcessHandle gpu_process_; + // Whether we are running a GPU thread inside the browser process instead + // of a separate GPU process. + bool in_process_; + + scoped_ptr in_process_gpu_thread_; + DISALLOW_COPY_AND_ASSIGN(GpuProcessHost); }; diff --git a/content/browser/gpu/gpu_process_host_ui_shim.cc b/content/browser/gpu/gpu_process_host_ui_shim.cc index 276dca4..312ca9f 100644 --- a/content/browser/gpu/gpu_process_host_ui_shim.cc +++ b/content/browser/gpu/gpu_process_host_ui_shim.cc @@ -4,18 +4,15 @@ #include "content/browser/gpu/gpu_process_host_ui_shim.h" -#include "base/command_line.h" #include "base/id_map.h" #include "base/process_util.h" #include "base/debug/trace_event.h" -#include "chrome/browser/browser_process.h" #include "content/browser/browser_thread.h" #include "content/browser/gpu/gpu_data_manager.h" #include "content/browser/gpu/gpu_process_host.h" #include "content/browser/renderer_host/render_process_host.h" #include "content/browser/renderer_host/render_view_host.h" #include "content/browser/renderer_host/render_widget_host_view.h" -#include "content/common/content_switches.h" #include "content/common/gpu/gpu_messages.h" #if defined(OS_LINUX) @@ -23,7 +20,6 @@ #include // NOLINT #include // NOLINT #include "ui/base/x/x11_util.h" -#include "ui/gfx/gtk_native_view_id_manager.h" #include "ui/gfx/size.h" #endif // defined(OS_LINUX) namespace { @@ -53,41 +49,6 @@ class SendOnIOThreadTask : public Task { scoped_ptr msg_; }; -class UIThreadSender : public IPC::Channel::Sender { - public: - virtual bool Send(IPC::Message* msg) { - // The GPU process must never send a synchronous IPC message to the browser - // process. This could result in deadlock. Unfortunately linux does this for - // GpuHostMsg_ResizeXID. TODO(apatrick): fix this before issuing any GL calls - // on the browser process' GPU thread. -#if !defined(OS_LINUX) - DCHECK(!msg->is_sync()); -#endif - - // When the GpuChannelManager sends an IPC, post it to the UI thread without - // using IPC. - bool success = BrowserThread::PostTask( - BrowserThread::UI, - FROM_HERE, - new RouteToGpuProcessHostUIShimTask(0, *msg)); - - delete msg; - return success; - } -}; - -void ForwardMessageToGpuThread(GpuChannelManager* gpu_channel_manager, - IPC::Message* msg) { - bool success = gpu_channel_manager->OnMessageReceived(*msg); - - // If the message was not handled, it is likely it was intended for the - // GpuChildThread, which does not exist in single process and in process GPU - // mode. - DCHECK(success); - - delete msg; -} - } // namespace RouteToGpuProcessHostUIShimTask::RouteToGpuProcessHostUIShimTask( @@ -107,18 +68,8 @@ void RouteToGpuProcessHostUIShimTask::Run() { } GpuProcessHostUIShim::GpuProcessHostUIShim(int host_id) - : host_id_(host_id), - gpu_channel_manager_(NULL), - ui_thread_sender_(NULL) { + : host_id_(host_id) { g_hosts_by_id.AddWithID(this, host_id_); - if (host_id == 0) { - ui_thread_sender_ = new UIThreadSender; - gpu_channel_manager_ = new GpuChannelManager( - ui_thread_sender_, - NULL, - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), - g_browser_process->shutdown_event()); - } } // static @@ -150,24 +101,9 @@ GpuProcessHostUIShim* GpuProcessHostUIShim::FromID(int host_id) { bool GpuProcessHostUIShim::Send(IPC::Message* msg) { DCHECK(CalledOnValidThread()); - - bool success; - - if (host_id_ == 0) { - success = BrowserThread::PostTask( - BrowserThread::GPU, - FROM_HERE, - NewRunnableFunction(ForwardMessageToGpuThread, - gpu_channel_manager_, - msg)); - } else { - success = BrowserThread::PostTask( - BrowserThread::IO, - FROM_HERE, - new SendOnIOThreadTask(host_id_, msg)); - } - - return success; + return BrowserThread::PostTask(BrowserThread::IO, + FROM_HERE, + new SendOnIOThreadTask(host_id_, msg)); } bool GpuProcessHostUIShim::OnMessageReceived(const IPC::Message& message) { @@ -200,20 +136,6 @@ void GpuProcessHostUIShim::SendToGpuHost(int host_id, IPC::Message* msg) { GpuProcessHostUIShim::~GpuProcessHostUIShim() { DCHECK(CalledOnValidThread()); g_hosts_by_id.Remove(host_id_); - - // Ensure these are destroyed on the GPU thread. - if (gpu_channel_manager_) { - BrowserThread::DeleteSoon(BrowserThread::GPU, - FROM_HERE, - gpu_channel_manager_); - gpu_channel_manager_ = NULL; - } - if (ui_thread_sender_) { - BrowserThread::DeleteSoon(BrowserThread::GPU, - FROM_HERE, - ui_thread_sender_); - ui_thread_sender_ = NULL; - } } bool GpuProcessHostUIShim::OnControlMessageReceived( diff --git a/content/browser/gpu/gpu_process_host_ui_shim.h b/content/browser/gpu/gpu_process_host_ui_shim.h index fc077fa..f50ee95 100644 --- a/content/browser/gpu/gpu_process_host_ui_shim.h +++ b/content/browser/gpu/gpu_process_host_ui_shim.h @@ -14,13 +14,10 @@ #include #include "base/callback.h" +#include "base/task.h" #include "base/memory/linked_ptr.h" #include "base/memory/ref_counted.h" #include "base/threading/non_thread_safe.h" -#include "content/common/gpu/gpu_channel_manager.h" -#include "content/common/gpu/gpu_feature_flags.h" -#include "content/common/gpu/gpu_info.h" -#include "content/common/gpu/gpu_process_launch_causes.h" #include "content/common/message_router.h" namespace gfx { @@ -109,15 +106,6 @@ class GpuProcessHostUIShim // The serial number of the GpuProcessHost / GpuProcessHostUIShim pair. int host_id_; - - // In single process and in process GPU mode, this references the - // GpuChannelManager or null otherwise. It must be called and deleted on the - // GPU thread. - GpuChannelManager* gpu_channel_manager_; - - // This is likewise single process / in process GPU specific. This is a Sender - // implementation that forwards IPC messages to this UI shim on the UI thread. - IPC::Channel::Sender* ui_thread_sender_; }; #endif // CONTENT_BROWSER_GPU_GPU_PROCESS_HOST_UI_SHIM_H_ diff --git a/content/gpu/gpu_child_thread.cc b/content/gpu/gpu_child_thread.cc index 8612a66..d3e6e94 100644 --- a/content/gpu/gpu_child_thread.cc +++ b/content/gpu/gpu_child_thread.cc @@ -116,7 +116,11 @@ bool GpuChildThread::OnControlMessageReceived(const IPC::Message& msg) { } void GpuChildThread::OnInitialize() { - logging::SetLogMessageHandler(GpuProcessLogMessageHandler); + // We don't need to pipe log messages if we are running the GPU thread in + // the browser process. + if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess) && + !CommandLine::ForCurrentProcess()->HasSwitch(switches::kInProcessGPU)) + logging::SetLogMessageHandler(GpuProcessLogMessageHandler); // Load the GL implementation and locate the bindings before starting the GPU // watchdog because this can take a lot of time and the GPU watchdog might -- cgit v1.1