Skip to content

Commit

Permalink
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.
Browse files Browse the repository at this point in the history
This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=wolenetz@chromium.org, sky@chromium.org

Review URL: https://codereview.chromium.org/1548443002

Cr-Commit-Position: refs/heads/master@{#375620}
  • Loading branch information
anforowicz authored and Commit bot committed Feb 16, 2016
1 parent 601699b commit 2573ce7
Show file tree
Hide file tree
Showing 69 changed files with 349 additions and 204 deletions.
13 changes: 10 additions & 3 deletions cc/layers/texture_layer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ gpu::Mailbox MailboxFromChar(char value) {
}

gpu::SyncToken SyncTokenFromUInt(uint32_t value) {
return gpu::SyncToken(gpu::CommandBufferNamespace::GPU_IO, 0, 0x123, value);
return gpu::SyncToken(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123), value);
}

class MockLayerTreeHost : public LayerTreeHost {
Expand Down Expand Up @@ -145,8 +146,14 @@ struct CommonMailboxObjects {
explicit CommonMailboxObjects(SharedBitmapManager* manager)
: mailbox_name1_(MailboxFromChar('1')),
mailbox_name2_(MailboxFromChar('2')),
sync_token1_(gpu::CommandBufferNamespace::GPU_IO, 123, 0x234, 1),
sync_token2_(gpu::CommandBufferNamespace::GPU_IO, 123, 0x234, 2) {
sync_token1_(gpu::CommandBufferNamespace::GPU_IO,
123,
gpu::CommandBufferId::FromUnsafeValue(0x234),
1),
sync_token2_(gpu::CommandBufferNamespace::GPU_IO,
123,
gpu::CommandBufferId::FromUnsafeValue(0x234),
2) {
release_mailbox1_ = base::Bind(&MockMailboxCallback::Release,
base::Unretained(&mock_callback_),
mailbox_name1_);
Expand Down
3 changes: 2 additions & 1 deletion cc/output/gl_renderer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2185,7 +2185,8 @@ TEST_F(GLRendererTest, OverlaySyncTokensAreProcessed) {
viewport_rect, gfx::Transform());
root_pass->has_transparent_background = false;

gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0, 0x123, 29);
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123), 29);
TextureMailbox mailbox =
TextureMailbox(gpu::Mailbox::Generate(), sync_token, GL_TEXTURE_2D,
gfx::Size(256, 256), true);
Expand Down
15 changes: 10 additions & 5 deletions cc/resources/resource_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ class TextureStateTrackingContext : public TestWebGraphicsContext3D {

void genSyncToken(GLuint64 fence_sync, GLbyte* sync_token) override {
gpu::SyncToken sync_token_data(gpu::CommandBufferNamespace::GPU_IO, 0,
0x123, fence_sync);
gpu::CommandBufferId::FromUnsafeValue(0x123),
fence_sync);
sync_token_data.SetVerifyFlush();
memcpy(sync_token, &sync_token_data, sizeof(sync_token_data));
}
Expand Down Expand Up @@ -199,7 +200,8 @@ class ResourceProviderContext : public TestWebGraphicsContext3D {

void genSyncToken(GLuint64 fence_sync, GLbyte* sync_token) override {
gpu::SyncToken sync_token_data(gpu::CommandBufferNamespace::GPU_IO, 0,
0x123, fence_sync);
gpu::CommandBufferId::FromUnsafeValue(0x123),
fence_sync);
sync_token_data.SetVerifyFlush();
// Commit the produceTextureCHROMIUM calls at this point, so that
// they're associated with the sync point.
Expand Down Expand Up @@ -2835,7 +2837,8 @@ class ResourceProviderTestTextureMailboxGLFilters
use_image_texture_targets_));

unsigned texture_id = 1;
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0, 0x12,
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x12),
0x34);
unsigned target = GL_TEXTURE_2D;
const GLuint64 current_fence_sync = context->GetNextFenceSync();
Expand Down Expand Up @@ -2978,7 +2981,8 @@ TEST_P(ResourceProviderTest, TextureMailbox_GLTextureExternalOES) {
gpu_memory_buffer_manager_.get(), NULL, 0, 1,
use_gpu_memory_buffer_resources_, use_image_texture_targets_));

gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0, 0x12, 0x34);
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x12), 0x34);
const GLuint64 current_fence_sync = context->GetNextFenceSync();
unsigned target = GL_TEXTURE_EXTERNAL_OES;

Expand Down Expand Up @@ -3047,7 +3051,8 @@ TEST_P(ResourceProviderTest,
gpu_memory_buffer_manager_.get(), NULL, 0, 1,
use_gpu_memory_buffer_resources_, use_image_texture_targets_));

gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0, 0x12, 0x34);
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x12), 0x34);
const GLuint64 current_fence_sync = context->GetNextFenceSync();
unsigned target = GL_TEXTURE_2D;

Expand Down
10 changes: 6 additions & 4 deletions cc/resources/video_resource_updater_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,9 @@ class VideoResourceUpdaterTest : public testing::Test {
gpu::Mailbox mailbox;
mailbox.name[0] = 51;

const gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
0x123, 7);
const gpu::SyncToken sync_token(
gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123), 7);
scoped_refptr<media::VideoFrame> video_frame =
media::VideoFrame::WrapNativeTexture(
media::PIXEL_FORMAT_ARGB,
Expand Down Expand Up @@ -219,8 +220,9 @@ class VideoResourceUpdaterTest : public testing::Test {
for (int i = 0; i < kPlanesNum; ++i) {
mailbox[i].name[0] = 50 + 1;
}
const gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
0x123, 7);
const gpu::SyncToken sync_token(
gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123), 7);
const unsigned target = GL_TEXTURE_RECTANGLE_ARB;
scoped_refptr<media::VideoFrame> video_frame =
media::VideoFrame::WrapYUV420NativeTextures(
Expand Down
3 changes: 2 additions & 1 deletion cc/test/render_pass_test_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ void AddOneOfEveryQuadType(RenderPass* to_pass,
const float vertex_opacity[] = {1.0f, 1.0f, 1.0f, 1.0f};

static const gpu::SyncToken kSyncTokenForMailboxTextureQuad(
gpu::CommandBufferNamespace::GPU_IO, 0, 0x123, 30);
gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123), 30);
*sync_token_for_mailbox_tebxture = kSyncTokenForMailboxTextureQuad;

ResourceId resource1 = resource_provider->CreateResource(
Expand Down
4 changes: 2 additions & 2 deletions cc/test/test_web_graphics_context_3d.cc
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,8 @@ GLuint64 TestWebGraphicsContext3D::insertFenceSync() {

void TestWebGraphicsContext3D::genSyncToken(GLuint64 fence_sync,
GLbyte* sync_token) {
gpu::SyncToken sync_token_data(gpu::CommandBufferNamespace::GPU_IO, 0, 0,
fence_sync);
gpu::SyncToken sync_token_data(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId(), fence_sync);
sync_token_data.SetVerifyFlush();
memcpy(sync_token, &sync_token_data, sizeof(sync_token_data));
}
Expand Down
3 changes: 2 additions & 1 deletion cc/trees/layer_tree_host_perftest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,8 @@ class BrowserCompositorInvalidateLayerTreePerfTest
scoped_ptr<SingleReleaseCallback> callback = SingleReleaseCallback::Create(
base::Bind(&EmptyReleaseCallback));

gpu::SyncToken next_sync_token(gpu::CommandBufferNamespace::GPU_IO, 0, 1,
gpu::SyncToken next_sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(1),
next_fence_sync_);
next_sync_token.SetVerifyFlush();
TextureMailbox mailbox(gpu_mailbox, next_sync_token, GL_TEXTURE_2D);
Expand Down
4 changes: 2 additions & 2 deletions components/mus/gles2/command_buffer_driver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ CommandBufferDriver::Client::~Client() {}

CommandBufferDriver::CommandBufferDriver(
gpu::CommandBufferNamespace command_buffer_namespace,
uint64_t command_buffer_id,
gpu::CommandBufferId command_buffer_id,
gfx::AcceleratedWidget widget,
scoped_refptr<GpuState> gpu_state)
: command_buffer_namespace_(command_buffer_namespace),
Expand Down Expand Up @@ -450,7 +450,7 @@ void CommandBufferDriver::OnFenceSyncRelease(uint64_t release) {

bool CommandBufferDriver::OnWaitFenceSync(
gpu::CommandBufferNamespace namespace_id,
uint64_t command_buffer_id,
gpu::CommandBufferId command_buffer_id,
uint64_t release) {
DCHECK(CalledOnValidThread());
DCHECK(IsScheduled());
Expand Down
9 changes: 5 additions & 4 deletions components/mus/gles2/command_buffer_driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/time/time.h"
#include "gpu/command_buffer/common/capabilities.h"
#include "gpu/command_buffer/common/command_buffer.h"
#include "gpu/command_buffer/common/command_buffer_id.h"
#include "gpu/command_buffer/common/constants.h"
#include "mojo/public/cpp/bindings/array.h"
#include "mojo/public/cpp/system/buffer.h"
Expand Down Expand Up @@ -52,7 +53,7 @@ class CommandBufferDriver : base::NonThreadSafe {
virtual void UpdateVSyncParameters(int64_t timebase, int64_t interval) = 0;
};
CommandBufferDriver(gpu::CommandBufferNamespace command_buffer_namespace,
uint64_t command_buffer_id,
gpu::CommandBufferId command_buffer_id,
gfx::AcceleratedWidget widget,
scoped_refptr<GpuState> gpu_state);
~CommandBufferDriver();
Expand Down Expand Up @@ -81,7 +82,7 @@ class CommandBufferDriver : base::NonThreadSafe {
gpu::CommandBufferNamespace GetNamespaceID() const {
return command_buffer_namespace_;
}
uint64_t GetCommandBufferID() const { return command_buffer_id_; }
gpu::CommandBufferId GetCommandBufferID() const { return command_buffer_id_; }
gpu::SyncPointOrderData* sync_point_order_data() {
return sync_point_order_data_.get();
}
Expand Down Expand Up @@ -113,13 +114,13 @@ class CommandBufferDriver : base::NonThreadSafe {
const base::TimeDelta interval);
void OnFenceSyncRelease(uint64_t release);
bool OnWaitFenceSync(gpu::CommandBufferNamespace namespace_id,
uint64_t command_buffer_id,
gpu::CommandBufferId command_buffer_id,
uint64_t release);
void OnParseError();
void OnContextLost(uint32_t reason);

const gpu::CommandBufferNamespace command_buffer_namespace_;
const uint64_t command_buffer_id_;
const gpu::CommandBufferId command_buffer_id_;
gfx::AcceleratedWidget widget_;
scoped_ptr<Client> client_;
scoped_ptr<gpu::CommandBufferService> command_buffer_;
Expand Down
6 changes: 4 additions & 2 deletions components/mus/gles2/command_buffer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ void CommandBufferImpl::InitializeOnGpuThread(
void(mojom::CommandBufferInitializeResultPtr)>& callback) {
DCHECK(!driver_);
driver_.reset(new CommandBufferDriver(
gpu::CommandBufferNamespace::MOJO, ++g_next_command_buffer_id,
gpu::CommandBufferNamespace::MOJO,
gpu::CommandBufferId::FromUnsafeValue(++g_next_command_buffer_id),
gfx::kNullAcceleratedWidget, gpu_state_));
driver_->set_client(make_scoped_ptr(new CommandBufferDriverClientImpl(this)));
client_ = mojo::MakeProxy(client.PassInterface());
Expand All @@ -199,7 +200,8 @@ void CommandBufferImpl::InitializeOnGpuThread(
if (result) {
initialize_result = mojom::CommandBufferInitializeResult::New();
initialize_result->command_buffer_namespace = driver_->GetNamespaceID();
initialize_result->command_buffer_id = driver_->GetCommandBufferID();
initialize_result->command_buffer_id =
driver_->GetCommandBufferID().GetUnsafeValue();
initialize_result->capabilities = driver_->GetCapabilities();
}
gpu_state_->control_task_runner()->PostTask(
Expand Down
9 changes: 5 additions & 4 deletions components/mus/gles2/command_buffer_local.cc
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ gpu::CommandBufferNamespace CommandBufferLocal::GetNamespaceID() const {
return gpu::CommandBufferNamespace::MOJO_LOCAL;
}

uint64_t CommandBufferLocal::GetCommandBufferID() const {
gpu::CommandBufferId CommandBufferLocal::GetCommandBufferID() const {
DCHECK(CalledOnValidThread());
return driver_->GetCommandBufferID();
}
Expand Down Expand Up @@ -421,9 +421,10 @@ void CommandBufferLocal::MakeProgressAndUpdateState() {

void CommandBufferLocal::InitializeOnGpuThread(base::WaitableEvent* event,
bool* result) {
driver_.reset(new CommandBufferDriver(gpu::CommandBufferNamespace::MOJO_LOCAL,
++g_next_command_buffer_id, widget_,
gpu_state_));
driver_.reset(new CommandBufferDriver(
gpu::CommandBufferNamespace::MOJO_LOCAL,
gpu::CommandBufferId::FromUnsafeValue(++g_next_command_buffer_id),
widget_, gpu_state_));
const size_t kSharedStateSize = sizeof(gpu::CommandBufferSharedState);
void* memory = nullptr;
mojo::ScopedSharedBufferHandle duped;
Expand Down
3 changes: 2 additions & 1 deletion components/mus/gles2/command_buffer_local.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "components/mus/public/interfaces/command_buffer.mojom.h"
#include "gpu/command_buffer/client/gpu_control.h"
#include "gpu/command_buffer/common/command_buffer.h"
#include "gpu/command_buffer/common/command_buffer_id.h"
#include "gpu/command_buffer/common/command_buffer_shared.h"
#include "ui/gfx/geometry/size.h"
#include "ui/gfx/gpu_memory_buffer.h"
Expand Down Expand Up @@ -88,7 +89,7 @@ class CommandBufferLocal : public gpu::CommandBuffer,
bool IsGpuChannelLost() override;
void EnsureWorkVisible() override;
gpu::CommandBufferNamespace GetNamespaceID() const override;
uint64_t GetCommandBufferID() const override;
gpu::CommandBufferId GetCommandBufferID() const override;
int32_t GetExtraCommandBufferData() const override;
uint64_t GenerateFenceSyncRelease() override;
bool IsFenceSyncRelease(uint64_t release) override;
Expand Down
6 changes: 3 additions & 3 deletions content/browser/download/save_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@
#include <vector>

#include "base/files/file_path.h"
#include "content/common/id_type.h"
#include "gpu/command_buffer/common/id_type.h"
#include "url/gurl.h"

namespace content {

class SavePackage;
using SavePackageId = IdType32<SavePackage>;
using SavePackageId = gpu::IdType32<SavePackage>;

class SaveItem;
using SaveItemId = IdType32<SaveItem>;
using SaveItemId = gpu::IdType32<SaveItem>;

// Map from save_item_id into final file path.
using FinalNamesMap = std::map<SaveItemId, base::FilePath>;
Expand Down
6 changes: 4 additions & 2 deletions content/common/cc_messages_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -617,10 +617,12 @@ TEST_F(CCMessagesTest, UnusedSharedQuadStates) {
TEST_F(CCMessagesTest, Resources) {
IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
gfx::Size arbitrary_size(757, 1281);
gpu::SyncToken arbitrary_token1(gpu::CommandBufferNamespace::GPU_IO, 0, 0x123,
gpu::SyncToken arbitrary_token1(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123),
71234838);
arbitrary_token1.SetVerifyFlush();
gpu::SyncToken arbitrary_token2(gpu::CommandBufferNamespace::GPU_IO, 0, 0x123,
gpu::SyncToken arbitrary_token2(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(0x123),
53589793);
arbitrary_token2.SetVerifyFlush();

Expand Down
13 changes: 8 additions & 5 deletions content/common/gpu/client/command_buffer_proxy_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "content/common/view_messages.h"
#include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
#include "gpu/command_buffer/common/cmd_buffer_common.h"
#include "gpu/command_buffer/common/command_buffer_id.h"
#include "gpu/command_buffer/common/command_buffer_shared.h"
#include "gpu/command_buffer/common/gpu_memory_allocation.h"
#include "gpu/command_buffer/common/sync_token.h"
Expand All @@ -31,8 +32,9 @@ namespace content {

namespace {

uint64_t CommandBufferProxyID(int channel_id, int32_t route_id) {
return (static_cast<uint64_t>(channel_id) << 32) | route_id;
gpu::CommandBufferId CommandBufferProxyID(int channel_id, int32_t route_id) {
return gpu::CommandBufferId::FromUnsafeValue(
(static_cast<uint64_t>(channel_id) << 32) | route_id);
}

} // namespace
Expand Down Expand Up @@ -538,7 +540,7 @@ gpu::CommandBufferNamespace CommandBufferProxyImpl::GetNamespaceID() const {
return gpu::CommandBufferNamespace::GPU_IO;
}

uint64_t CommandBufferProxyImpl::GetCommandBufferID() const {
gpu::CommandBufferId CommandBufferProxyImpl::GetCommandBufferID() const {
return command_buffer_id_;
}

Expand Down Expand Up @@ -602,8 +604,9 @@ void CommandBufferProxyImpl::SignalSyncToken(const gpu::SyncToken& sync_token,
bool CommandBufferProxyImpl::CanWaitUnverifiedSyncToken(
const gpu::SyncToken* sync_token) {
// Can only wait on an unverified sync token if it is from the same channel.
const uint64_t token_channel = sync_token->command_buffer_id() >> 32;
const uint64_t channel = command_buffer_id_ >> 32;
const uint64_t token_channel =
sync_token->command_buffer_id().GetUnsafeValue() >> 32;
const uint64_t channel = command_buffer_id_.GetUnsafeValue() >> 32;
if (sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO ||
token_channel != channel) {
return false;
Expand Down
5 changes: 3 additions & 2 deletions content/common/gpu/client/command_buffer_proxy_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "base/observer_list.h"
#include "gpu/command_buffer/client/gpu_control.h"
#include "gpu/command_buffer/common/command_buffer.h"
#include "gpu/command_buffer/common/command_buffer_id.h"
#include "gpu/command_buffer/common/command_buffer_shared.h"
#include "gpu/command_buffer/common/gpu_memory_allocation.h"
#include "ipc/ipc_listener.h"
Expand Down Expand Up @@ -121,7 +122,7 @@ class CommandBufferProxyImpl
bool IsGpuChannelLost() override;
void EnsureWorkVisible() override;
gpu::CommandBufferNamespace GetNamespaceID() const override;
uint64_t GetCommandBufferID() const override;
gpu::CommandBufferId GetCommandBufferID() const override;
int32_t GetExtraCommandBufferData() const override;
uint64_t GenerateFenceSyncRelease() override;
bool IsFenceSyncRelease(uint64_t release) override;
Expand Down Expand Up @@ -229,7 +230,7 @@ class CommandBufferProxyImpl
// |*this| is owned by |*channel_| and so is always outlived by it, so using a
// raw pointer is ok.
GpuChannelHost* channel_;
const uint64_t command_buffer_id_;
const gpu::CommandBufferId command_buffer_id_;
const int32_t route_id_;
const int32_t stream_id_;
uint32_t flush_count_;
Expand Down
5 changes: 2 additions & 3 deletions content/common/gpu/client/gpu_context_tests.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,8 @@ CONTEXT_TEST_F(SignalTest, InvalidSignalSyncTokenTest) {

// Signalling something that doesn't exist should run the callback
// immediately.
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO,
0,
1297824234,
gpu::SyncToken sync_token(gpu::CommandBufferNamespace::GPU_IO, 0,
gpu::CommandBufferId::FromUnsafeValue(1297824234),
9123743439);
TestSignalSyncToken(sync_token);
};
Expand Down
Loading

0 comments on commit 2573ce7

Please sign in to comment.