Fix layering violation where content is included by gpu. (issue 11267035)

28 views
Skip to first unread message

ccam...@chromium.org

unread,
Oct 25, 2012, 4:22:53 AM10/25/12
to ccam...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org, apat...@chromium.org
Reviewers: ,

Message:
There are also a massive number of methods in PpapiCommandBufferProxy which
are
marked as NOTIMPLEMENTED or NOTREACHED. Of these, only SetParent is marked
that
it may be added int the future. Should I remove all of the NOTREACHED or
NOTIMPLEMENTED methods from the interface as well?

Description:
Fix layering violation where content is included by gpu.

Make WebGraphicsContext3DCommandBufferImpl aways use CommandBufferProxyImpl
instead of the more abstract CommandBufferProxy. Then remove methods used
only by WebGraphicsContext3DCommandBufferImpl from CommandBufferProxy.

BUG=157175


Please review this at https://codereview.chromium.org/11267035/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M content/common/gpu/client/command_buffer_proxy_impl.h
M content/common/gpu/client/gpu_channel_host.h
M content/common/gpu/client/gpu_channel_host.cc
M content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
M content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
M content/renderer/pepper/pepper_platform_context_3d_impl.h
M content/renderer/pepper/pepper_platform_context_3d_impl.cc
M gpu/ipc/command_buffer_proxy.h
M ppapi/proxy/ppapi_command_buffer_proxy.h
M ppapi/proxy/ppapi_command_buffer_proxy.cc


Index: content/common/gpu/client/command_buffer_proxy_impl.h
diff --git a/content/common/gpu/client/command_buffer_proxy_impl.h
b/content/common/gpu/client/command_buffer_proxy_impl.h
index
856159df8ac58f52ea5e4595824c056358b2e8c1..b9da1a361443e73eed8fd7133ac7581ca0ba671b
100644
--- a/content/common/gpu/client/command_buffer_proxy_impl.h
+++ b/content/common/gpu/client/command_buffer_proxy_impl.h
@@ -19,6 +19,7 @@
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "content/common/gpu/client/gpu_video_decode_accelerator_host.h"
+#include "content/common/gpu/gpu_memory_allocation.h"
#include "gpu/command_buffer/common/command_buffer.h"
#include "gpu/command_buffer/common/command_buffer_shared.h"
#include "ipc/ipc_listener.h"
@@ -31,7 +32,6 @@ class SharedMemory;

namespace content {
class GpuChannelHost;
-struct GpuMemoryAllocationForRenderer;

// Client side proxy that forwards messages synchronously to a
// CommandBufferStub.
@@ -70,9 +70,6 @@ class CommandBufferProxyImpl
virtual void WaitSyncPoint(uint32 sync_point) OVERRIDE;
virtual bool SignalSyncPoint(uint32 sync_point,
const base::Closure& callback) OVERRIDE;
- virtual void SetMemoryAllocationChangedCallback(
- const base::Callback<void(const GpuMemoryAllocationForRenderer&)>&
- callback) OVERRIDE;
virtual bool SetParent(CommandBufferProxy* parent_command_buffer,
uint32 parent_texture_id) OVERRIDE;
virtual void SetChannelErrorCallback(const base::Closure& callback)
OVERRIDE;
@@ -99,6 +96,10 @@ class CommandBufferProxyImpl
virtual void SetContextLostReason(
gpu::error::ContextLostReason reason) OVERRIDE;

+ void SetMemoryAllocationChangedCallback(
+ const base::Callback<void(const GpuMemoryAllocationForRenderer&)>&
+ callback) OVERRIDE;
+
// TODO(apatrick): this is a temporary optimization while skia is calling
// ContentGLContext::MakeCurrent prior to every GL call. It saves
returning 6
// ints redundantly when only the error is needed for the
Index: content/common/gpu/client/gpu_channel_host.cc
diff --git a/content/common/gpu/client/gpu_channel_host.cc
b/content/common/gpu/client/gpu_channel_host.cc
index
aa556c8ead36187f0da1a2612361a258af33a596..2677550dfc95acf1296a36214db43fb477332b5f
100644
--- a/content/common/gpu/client/gpu_channel_host.cc
+++ b/content/common/gpu/client/gpu_channel_host.cc
@@ -108,9 +108,9 @@ bool GpuChannelHost::Send(IPC::Message* message) {
return false;
}

-CommandBufferProxy* GpuChannelHost::CreateViewCommandBuffer(
+CommandBufferProxyImpl* GpuChannelHost::CreateViewCommandBuffer(
int32 surface_id,
- CommandBufferProxy* share_group,
+ CommandBufferProxyImpl* share_group,
const std::string& allowed_extensions,
const std::vector<int32>& attribs,
const GURL& active_url,
@@ -147,9 +147,9 @@ CommandBufferProxy*
GpuChannelHost::CreateViewCommandBuffer(
#endif
}

-CommandBufferProxy* GpuChannelHost::CreateOffscreenCommandBuffer(
+CommandBufferProxyImpl* GpuChannelHost::CreateOffscreenCommandBuffer(
const gfx::Size& size,
- CommandBufferProxy* share_group,
+ CommandBufferProxyImpl* share_group,
const std::string& allowed_extensions,
const std::vector<int32>& attribs,
const GURL& active_url,
@@ -201,7 +201,7 @@ GpuVideoDecodeAcceleratorHost*
GpuChannelHost::CreateVideoDecoder(
}

void GpuChannelHost::DestroyCommandBuffer(
- CommandBufferProxy* command_buffer) {
+ CommandBufferProxyImpl* command_buffer) {
TRACE_EVENT0("gpu", "GpuChannelHost::DestroyCommandBuffer");

#if defined(ENABLE_GPU)
Index: content/common/gpu/client/gpu_channel_host.h
diff --git a/content/common/gpu/client/gpu_channel_host.h
b/content/common/gpu/client/gpu_channel_host.h
index
8b68626581f703750aa2be041a77f4a9db865e15..38457e06634f2086992ff02359b80dfa4f65f321
100644
--- a/content/common/gpu/client/gpu_channel_host.h
+++ b/content/common/gpu/client/gpu_channel_host.h
@@ -27,7 +27,6 @@
#include "ui/gfx/size.h"
#include "ui/gl/gpu_preference.h"

-class CommandBufferProxy;
class GURL;
class MessageLoop;
class TransportTextureService;
@@ -113,18 +112,18 @@ class GpuChannelHost : public IPC::Sender,
virtual bool Send(IPC::Message* msg) OVERRIDE;

// Create and connect to a command buffer in the GPU process.
- CommandBufferProxy* CreateViewCommandBuffer(
+ CommandBufferProxyImpl* CreateViewCommandBuffer(
int32 surface_id,
- CommandBufferProxy* share_group,
+ CommandBufferProxyImpl* share_group,
const std::string& allowed_extensions,
const std::vector<int32>& attribs,
const GURL& active_url,
gfx::GpuPreference gpu_preference);

// Create and connect to a command buffer in the GPU process.
- CommandBufferProxy* CreateOffscreenCommandBuffer(
+ CommandBufferProxyImpl* CreateOffscreenCommandBuffer(
const gfx::Size& size,
- CommandBufferProxy* share_group,
+ CommandBufferProxyImpl* share_group,
const std::string& allowed_extensions,
const std::vector<int32>& attribs,
const GURL& active_url,
@@ -138,7 +137,7 @@ class GpuChannelHost : public IPC::Sender,
media::VideoDecodeAccelerator::Client* client);

// Destroy a command buffer created by this channel.
- void DestroyCommandBuffer(CommandBufferProxy* command_buffer);
+ void DestroyCommandBuffer(CommandBufferProxyImpl* command_buffer);

// Collect rendering stats from GPU process.
bool CollectRenderingStatsForSurface(
Index: content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
diff --git
a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
index
55d2d2a4f3c9aa49a01da9ae9019515424c4b3ca..e5b1de523723d71e5de0093f70c2c726b71c9d12
100644
--- a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
+++ b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
@@ -170,7 +170,7 @@ WebGraphicsContext3DCommandBufferImpl::
}

void WebGraphicsContext3DCommandBufferImpl::InitializeWithCommandBuffer(
- CommandBufferProxy* command_buffer,
+ CommandBufferProxyImpl* command_buffer,
const WebGraphicsContext3D::Attributes& attributes,
bool bind_generates_resources) {
DCHECK(command_buffer);
@@ -278,7 +278,7 @@ bool
WebGraphicsContext3DCommandBufferImpl::InitializeCommandBuffer(
// for our share group isn't deleted.
// (There's also a lock in our destructor.)
base::AutoLock lock(g_all_shared_contexts_lock.Get());
- CommandBufferProxy* share_group = NULL;
+ CommandBufferProxyImpl* share_group = NULL;
if (attributes_.shareResources) {
WebGraphicsContext3DCommandBufferImpl* share_group_context =
g_all_shared_contexts.Pointer()->empty() ?
Index: content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
diff --git
a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
index
d25eb53c574a0d0dba67f49e6fff7cb23fa3dc57..489e43f0ca9f1029b6d6899ace306bef8d4bdf42
100644
--- a/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
+++ b/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
@@ -11,6 +11,7 @@
#include "base/callback.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/weak_ptr.h"
+#include "content/common/gpu/client/command_buffer_proxy_impl.h"
#include "content/common/gpu/gpu_process_launch_causes.h"
#include "googleurl/src/gurl.h"

#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h"
@@ -23,7 +24,6 @@
#define FLIP_FRAMEBUFFER_VERTICALLY
#endif

-class CommandBufferProxy;
namespace gpu {

class TransferBuffer;
@@ -81,7 +81,7 @@ class WebGraphicsContext3DCommandBufferImpl
virtual ~WebGraphicsContext3DCommandBufferImpl();

void InitializeWithCommandBuffer(
- CommandBufferProxy* command_buffer,
+ CommandBufferProxyImpl* command_buffer,
const Attributes& attributes,
bool bind_generates_resources);

@@ -99,7 +99,7 @@ class WebGraphicsContext3DCommandBufferImpl
// Gets the context ID (relative to the channel).
int GetContextID();

- CommandBufferProxy* GetCommandBufferProxy() { return command_buffer_; }
+ CommandBufferProxyImpl* GetCommandBufferProxy() { return
command_buffer_; }

gpu::gles2::GLES2Implementation* GetImplementation() { return gl_; }

@@ -719,7 +719,7 @@ class WebGraphicsContext3DCommandBufferImpl
bool initialized_;
WebGraphicsContext3DCommandBufferImpl* parent_;
uint32 parent_texture_id_;
- CommandBufferProxy* command_buffer_;
+ CommandBufferProxyImpl* command_buffer_;
gpu::gles2::GLES2CmdHelper* gles2_helper_;
gpu::TransferBuffer* transfer_buffer_;
gpu::gles2::GLES2Implementation* gl_;
Index: content/renderer/pepper/pepper_platform_context_3d_impl.cc
diff --git a/content/renderer/pepper/pepper_platform_context_3d_impl.cc
b/content/renderer/pepper/pepper_platform_context_3d_impl.cc
index
27c967e05f973b0bf3eb2e59814b34385130f465..41ad54c0c9d9b85c92e35fedcc5006e765b35916
100644
--- a/content/renderer/pepper/pepper_platform_context_3d_impl.cc
+++ b/content/renderer/pepper/pepper_platform_context_3d_impl.cc
@@ -102,7 +102,7 @@ bool PlatformContext3DImpl::Init(const int32*
attrib_list,
attribs.push_back(PP_GRAPHICS3DATTRIB_NONE);
}

- CommandBufferProxy* share_buffer = NULL;
+ CommandBufferProxyImpl* share_buffer = NULL;
if (share_context) {
PlatformContext3DImpl* share_impl =
static_cast<PlatformContext3DImpl*>(share_context);
@@ -144,7 +144,7 @@ bool PlatformContext3DImpl::Init(const int32*
attrib_list,
parent_gles2->helper()->CommandBufferHelper::Finish();
parent_texture_id_ = parent_gles2->MakeTextureId();

- CommandBufferProxy* parent_command_buffer =
+ CommandBufferProxyImpl* parent_command_buffer =
parent_context_->GetCommandBufferProxy();
if (!command_buffer_->SetParent(parent_command_buffer,
parent_texture_id_))
return false;
@@ -178,7 +178,7 @@ void PlatformContext3DImpl::SetParentContext(
parent_gles2->helper()->CommandBufferHelper::Flush();
parent_texture_id_ = parent_gles2->MakeTextureId();

- CommandBufferProxy* parent_command_buffer =
+ CommandBufferProxyImpl* parent_command_buffer =
parent_context_->GetCommandBufferProxy();
command_buffer_->SetParent(parent_command_buffer, parent_texture_id_);
}
Index: content/renderer/pepper/pepper_platform_context_3d_impl.h
diff --git a/content/renderer/pepper/pepper_platform_context_3d_impl.h
b/content/renderer/pepper/pepper_platform_context_3d_impl.h
index
2947e6d267253622e10dff70132000432492eef6..4c76c1ea605f72b91c1edcdac0132434516ece5e
100644
--- a/content/renderer/pepper/pepper_platform_context_3d_impl.h
+++ b/content/renderer/pepper/pepper_platform_context_3d_impl.h
@@ -56,7 +56,7 @@ class PlatformContext3DImpl
scoped_refptr<GpuChannelHost> channel_;
unsigned int parent_texture_id_;
bool has_alpha_;
- CommandBufferProxy* command_buffer_;
+ CommandBufferProxyImpl* command_buffer_;
base::Closure context_lost_callback_;
ConsoleMessageCallback console_message_callback_;
base::WeakPtrFactory<PlatformContext3DImpl> weak_ptr_factory_;
Index: gpu/ipc/command_buffer_proxy.h
diff --git a/gpu/ipc/command_buffer_proxy.h b/gpu/ipc/command_buffer_proxy.h
index
3ce13de5778e3cf6fcfd26c45f5291c8f7849b72..56713a630a9f19f0538aae6ac42b188f4cc9dcdb
100644
--- a/gpu/ipc/command_buffer_proxy.h
+++ b/gpu/ipc/command_buffer_proxy.h
@@ -11,11 +11,6 @@
#include "gpu/command_buffer/common/command_buffer.h"
#include "gpu/command_buffer/common/command_buffer_shared.h"

-// TODO(fsamuel): this is a layering violation. http://crbug.com/157175
-namespace content {
-struct GpuMemoryAllocationForRenderer;
-}
-
// Client side proxy that forwards messages synchronously to a
// CommandBufferStub.
class GPU_EXPORT CommandBufferProxy : public gpu::CommandBuffer {
@@ -55,11 +50,6 @@ class GPU_EXPORT CommandBufferProxy : public
gpu::CommandBuffer {
virtual bool SignalSyncPoint(uint32 sync_point,
const base::Closure& callback) = 0;

- // Register a callback to invoke whenever we recieve a new memory
allocation.
- virtual void SetMemoryAllocationChangedCallback(
- const base::Callback<void(
- const content::GpuMemoryAllocationForRenderer&)>& callback) = 0;
-
// Reparent a command buffer. TODO(apatrick): going forward, the notion
of
// the parent / child relationship between command buffers is going away
in
// favor of the notion of surfaces that can be drawn to in one command
buffer
Index: ppapi/proxy/ppapi_command_buffer_proxy.cc
diff --git a/ppapi/proxy/ppapi_command_buffer_proxy.cc
b/ppapi/proxy/ppapi_command_buffer_proxy.cc
index
2dea1bf6d1901ff838633450c0a4a77fd0251d90..19285e3346117e83c17887b62013ac13243c3315
100644
--- a/ppapi/proxy/ppapi_command_buffer_proxy.cc
+++ b/ppapi/proxy/ppapi_command_buffer_proxy.cc
@@ -75,12 +75,6 @@ bool PpapiCommandBufferProxy::SignalSyncPoint(uint32
sync_point,
return false;
}

-void PpapiCommandBufferProxy::SetMemoryAllocationChangedCallback(
- const base::Callback<void(
- const content::GpuMemoryAllocationForRenderer&)>& callback) {
- NOTIMPLEMENTED();
-}
-
bool PpapiCommandBufferProxy::SetParent(
CommandBufferProxy* parent_command_buffer,
uint32 parent_texture_id) {
Index: ppapi/proxy/ppapi_command_buffer_proxy.h
diff --git a/ppapi/proxy/ppapi_command_buffer_proxy.h
b/ppapi/proxy/ppapi_command_buffer_proxy.h
index
d78ee87cde62491399478b5045d61252d20b5894..2e9d5da76ac1c20e84a80063e49ded797a755fac
100644
--- a/ppapi/proxy/ppapi_command_buffer_proxy.h
+++ b/ppapi/proxy/ppapi_command_buffer_proxy.h
@@ -39,9 +39,6 @@ class PPAPI_PROXY_EXPORT PpapiCommandBufferProxy : public
CommandBufferProxy {
virtual void WaitSyncPoint(uint32 sync_point) OVERRIDE;
virtual bool SignalSyncPoint(uint32 sync_point,
const base::Closure& callback) OVERRIDE;
- virtual void SetMemoryAllocationChangedCallback(
- const base::Callback<void(
- const content::GpuMemoryAllocationForRenderer&)>& callback)
OVERRIDE;
virtual bool SetParent(CommandBufferProxy* parent_command_buffer,
uint32 parent_texture_id) OVERRIDE;
virtual void SetChannelErrorCallback(const base::Closure& callback)
OVERRIDE;


ccam...@chromium.org

unread,
Oct 25, 2012, 4:58:41 AM10/25/12
to ccam...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org, apat...@chromium.org
I went ahead and got rid of all of the NOTIMPLEMENTED entrypoints from
CommandBufferProxy. SetParent remains because the trajectory is undecided.

https://codereview.chromium.org/11267035/

ccam...@chromium.org

unread,
Oct 26, 2012, 1:38:47 PM10/26/12
to ccam...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org, apat...@chromium.org
Ping. Ptal.

https://codereview.chromium.org/11267035/

apat...@chromium.org

unread,
Oct 26, 2012, 3:20:32 PM10/26/12
to ccam...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
On 2012/10/26 17:38:47, ccameron1 wrote:
> Ping. Ptal.

Did you set your reviewers?

https://codereview.chromium.org/11267035/

ccam...@chromium.org

unread,
Oct 26, 2012, 3:24:12 PM10/26/12
to pi...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
Looks like the reviewers line got lost at some point.

https://codereview.chromium.org/11267035/

pi...@chromium.org

unread,
Oct 26, 2012, 7:35:20 PM10/26/12
to ccam...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
lgtm



https://codereview.chromium.org/11267035/

ccam...@chromium.org

unread,
Oct 27, 2012, 7:26:32 PM10/27/12
to pi...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
Thanks!!

https://codereview.chromium.org/11267035/

commi...@chromium.org

unread,
Oct 27, 2012, 7:26:41 PM10/27/12
to ccam...@chromium.org, pi...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccam...@chromium.org/11267035/10001


https://chromiumcodereview.appspot.com/11267035/

commi...@chromium.org

unread,
Oct 28, 2012, 9:05:28 AM10/28/12
to ccam...@chromium.org, pi...@chromium.org, chromium...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, j...@chromium.org
Change committed as 164564

https://chromiumcodereview.appspot.com/11267035/
Reply all
Reply to author
Forward
0 new messages