Adding GpuMemoryManager to track GpuCommandBufferStub visibility and last_used_time and dictate mem…

content/common/gpu/gpu_memory_management.h

Index: content/common/gpu/gpu_memory_management.h
diff --git a/content/common/gpu/gpu_memory_management.h b/content/common/gpu/gpu_memory_management.h
new file mode 100644
index 0000000000000000000000000000000000000000..4039cca1620bc8e24e6c95e40e1d3746f590c70a
--- /dev/null
+++ b/content/common/gpu/gpu_memory_management.h
@@ -0,0 +1,77 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_COMMON_GPU_GPU_MEMORY_MANAGEMENT_H_
+#define CONTENT_COMMON_GPU_GPU_MEMORY_MANAGEMENT_H_

[nduca, 2012/01/31 06:53:47]

OK , so this file shouldn't exist. These individual classes should be classes in
their associated class' files.

[mmocny, 2012/01/31 18:54:57]

Done.

+#pragma once
+
+#if defined(ENABLE_GPU)
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/time.h"
+
+#include "content/common/gpu/gpu_memory_allocation.h"
+
+////////////////////////////////////////////////////////////////////////////////
+
+struct GpuSurfaceState {

[nduca, 2012/01/31 06:53:47]

This I think should be a public inner struct called SurfaceState inside
GpuCommandBufferShim

[mmocny, 2012/01/31 18:54:57]

Done.

+  int32 surface_id;
+  bool visible;
+  base::TimeTicks last_used_time;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+
+class GpuMemoryManageableCommandBufferStub {

[nduca, 2012/01/31 06:53:47]

This should be a class in GpuCommandBufferStub and should be either
a)  GpuCommandBufferStubBase or
b) GpuCommandBufferStub and we rename the existing class to be
CommandBufferStubImplmpl.

[mmocny, 2012/01/31 18:54:57]

Done.

+public:
+  virtual ~GpuMemoryManageableCommandBufferStub() {}
+
+  virtual const GpuSurfaceState& surface_state() = 0;
+  virtual const std::vector<int32>& affected_surface_ids() = 0;
+  virtual void SendMemoryAllocation(const GpuMemoryAllocation& allocation) = 0;

[nduca, 2012/01/31 06:53:47]

SendMemoryAllocationToProxy?

[mmocny, 2012/01/31 18:54:57]

Done.

+};
+
+class GpuMemoryManagerClient {

[nduca, 2012/01/31 06:53:47]

This class should be defined in gpu_memory_manager.h, just before the actual
GpuMemoryManager class.

[mmocny, 2012/01/31 18:54:57]

Done.

+public:
+  virtual ~GpuMemoryManagerClient() {}
+
+  virtual void AppendAllCommandBufferStubs(
+      std::vector<GpuMemoryManageableCommandBufferStub*>& stubs_with_surface,
+      std::vector<GpuMemoryManageableCommandBufferStub*>& stubs_without_surface)
+          = 0;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+

[jonathan.backer, 2012/01/31 18:13:58]

I have a feeling that these comparators are only used in GpuMemoryManager.cc. If
so, why not define them there?

[mmocny, 2012/01/31 18:54:57]

Also used in tests, but they are moved to GpuMemoryManager header file.
On 2012/01/31 18:13:58, jonathan.backer wrote:

+/*
+ * Returns true if lhs stub is more "important" than rhs stub
+ */
+inline bool IsMoreImportant(GpuMemoryManageableCommandBufferStub* lhs,

[nduca, 2012/01/31 06:53:47]

Why both a comparator and a function? Pick one or the other?

[mmocny, 2012/01/31 18:54:57]

Done.

+                            GpuMemoryManageableCommandBufferStub* rhs) {
+  const GpuSurfaceState& lhs_ss = lhs->surface_state();
+  const GpuSurfaceState& rhs_ss = rhs->surface_state();
+
+  if (lhs_ss.visible)
+    return !rhs_ss.visible || (lhs_ss.last_used_time > rhs_ss.last_used_time);
+  else
+    return !rhs_ss.visible && (lhs_ss.last_used_time > rhs_ss.last_used_time);
+}
+
+/*
+ * Comparator used to sort stubs into most-to-least "important" order
+ */
+struct StubComparator {

[nduca, 2012/01/31 06:53:47]

class; struct feels awkward here.

[nduca, 2012/01/31 06:53:47]

I think this should be an inner class in the GpuMemoryManager header file. Its
only really exposed for the purposes of testing.

[mmocny, 2012/01/31 18:54:57]

Done.

[mmocny, 2012/01/31 18:54:57]

Done.

+  bool operator()(GpuMemoryManageableCommandBufferStub* lhs,
+                  GpuMemoryManageableCommandBufferStub* rhs) {
+    return IsMoreImportant(lhs, rhs);
+  }
+};
+
+////////////////////////////////////////////////////////////////////////////////
+
+#endif
+
+#endif // CONTENT_COMMON_GPU_GPU_MEMORY_MANAGEMENT_H_