[blimp] Add SkPicture caching support.

blimp/client/feature/compositor/blimp_client_picture_cache.cc

+// Copyright 2016 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.
+
+#include "blimp/client/feature/compositor/blimp_client_picture_cache.h"
+
+#include <stdint.h>
+#include <map>
+#include <memory>
+#include <vector>
+
+#include "base/logging.h"
+#include "blimp/common/compositor/blimp_picture_cache_registry.h"
+#include "cc/proto/picture_cache.h"
+#include "third_party/skia/include/core/SkPicture.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/core/SkStream.h"
+
+namespace blimp {
+namespace client {
+
+namespace {
+// Helper function to deserialize the content of |picture_data| into an
+// SkPicture.
+sk_sp<SkPicture> DeserializePicture(
+    SkPicture::InstallPixelRefProc pixel_deserializer,
+    const cc::PictureData& picture_data) {
+  SkMemoryStream stream(picture_data.data);
+  return SkPicture::MakeFromStream(&stream, pixel_deserializer);
+}
+
+}  // namespace
+
+BlimpClientPictureCache::BlimpClientPictureCache(
+    SkPicture::InstallPixelRefProc pixel_deserializer)
+    : pixel_deserializer_(pixel_deserializer) {}
+
+BlimpClientPictureCache::~BlimpClientPictureCache() {}

[vmpstr, 2016/06/01 00:10:56]

nit: = default

[nyquist, 2016/06/04 00:24:57]

Done.

+
+sk_sp<const SkPicture> BlimpClientPictureCache::GetPicture(
+    uint32_t engine_picture_id) {
+  DCHECK(pictures_.find(engine_picture_id) != pictures_.end());
+  return pictures_.find(engine_picture_id)->second;

[vmpstr, 2016/06/01 00:10:56]

return pictures_[engine_picture_id];

[nyquist, 2016/06/04 00:24:57]

Done.

+}
+
+void BlimpClientPictureCache::ApplyCacheUpdate(
+    const cc::PictureCacheUpdate& cache_update) {
+  for (const cc::PictureData& picture_data : cache_update) {
+    sk_sp<SkPicture> deserialized_picture =

[vmpstr, 2016/06/01 00:10:56]

I think we need to be a bit more consistent with constness here and throughout.
If GetPicture returns sk_sp<const SkPicture>, then we probably want to propagate
that in the rest of the functions/structures. 

[nyquist, 2016/06/04 00:24:57]

Done.

+        DeserializePicture(pixel_deserializer_, picture_data);
+    DCHECK(pictures_.find(picture_data.unique_id) == pictures_.end());
+    pictures_.insert(
+        std::make_pair(picture_data.unique_id, deserialized_picture));

[vmpstr, 2016/06/01 00:10:56]

I think pictures_.emplace(picture_data.unique_id,
std::move(deserialized_picture)); reads better, but it's up to you. 

[nyquist, 2016/06/04 00:24:57]

Does this look OK?

+#if DCHECK_IS_ON()
+    last_added_.insert(picture_data.unique_id);
+#endif
+  }
+}
+
+void BlimpClientPictureCache::Flush() {
+  std::vector<uint32_t> added;
+  std::vector<uint32_t> removed;
+  registry_.Commit(&added, &removed);
+
+#if DCHECK_IS_ON()
+  // Verify that the incoming cache update matches the new items.
+  DCHECK_EQ(added.size(), last_added_.size());
+  DCHECK(std::set<uint32_t>(added.begin(), added.end()) == last_added_);
+  last_added_.clear();
+#endif
+
+  for (const auto& it : removed) {
+    auto entry = pictures_.find(it);
+    if (entry == pictures_.end()) {
+      NOTREACHED() << "PictureMap does not contain entry " << it;

[vmpstr, 2016/06/01 00:10:56]

Can this if be a DCHECK instead? If not, can you add a comment why not.

[nyquist, 2016/06/04 00:24:57]

Done.

+      continue;
+    }
+    pictures_.erase(entry);
+  }
+}
+
+void BlimpClientPictureCache::MarkPictureForUnregistration(
+    uint32_t engine_picture_id) {
+  registry_.DecrementRefCount(engine_picture_id);
+}
+
+void BlimpClientPictureCache::MarkPictureForRegistration(
+    uint32_t engine_picture_id) {
+  registry_.IncrementRefCount(engine_picture_id);
+}
+
+}  // namespace client
+}  // namespace blimp