Introduce CrossThreadCopier<SkBitmap> [chromium/src : main]

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 2:25:38 AM10/27/21

to danakj, Hiroshige Hayashizaki, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Rayan Kanso, Yutaka Hirano

Attention is currently required from: danakj, Hiroshige Hayashizaki.

Yutaka Hirano would like danakj and Hiroshige Hayashizaki to review this change.

View Change

Introduce CrossThreadCopier<SkBitmap>

Allow immutable SkBitmap to be transferred across threads

Bug: 1241091
Change-Id: Ia771893245ef2fc1acb05cd4906d64bf2dd406fe
---
M third_party/blink/renderer/platform/wtf/cross_thread_copier.h
1 file changed, 25 insertions(+), 0 deletions(-)

diff --git a/third_party/blink/renderer/platform/wtf/cross_thread_copier.h b/third_party/blink/renderer/platform/wtf/cross_thread_copier.h
index 5f70203..3e1a08a 100644
--- a/third_party/blink/renderer/platform/wtf/cross_thread_copier.h
+++ b/third_party/blink/renderer/platform/wtf/cross_thread_copier.h
@@ -43,6 +43,7 @@
 #include "third_party/blink/renderer/platform/wtf/functional.h"  // FunctionThreadAffinity
 #include "third_party/blink/renderer/platform/wtf/type_traits.h"
 #include "third_party/blink/renderer/platform/wtf/wtf_size_t.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace base {
 template <typename, typename>
@@ -400,6 +401,18 @@
   STATIC_ONLY(CrossThreadCopier);
 };
 
+template <>
+struct CrossThreadCopier<SkBitmap> {
+  STATIC_ONLY(CrossThreadCopier);
+
+  using Type = SkBitmap;
+  static SkBitmap Copy(const SkBitmap& bitmap) = delete;
+  static SkBitmap Copy(SkBitmap&& bitmap) {
+    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";
+    return bitmap;
+  }
+};
+
 }  // namespace WTF
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_WTF_CROSS_THREAD_COPIER_H_

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 2:25:44 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Hiroshige Hayashizaki, danakj, Florin Malita, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki.

View Change

1 comment:

Patchset:
- Patch Set #1:
  Context: https://chromium-review.googlesource.com/c/chromium/src/+/3222803/2/third_party/blink/renderer/core/loader/threaded_icon_loader.cc#27

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 6:24:14 AM10/27/21

to Florin Malita, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Yutaka Hirano, Hiroshige Hayashizaki, danakj

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

Yutaka Hirano would like Florin Malita to review this change.

View Change

Introduce CrossThreadCopier<SkBitmap>

Allow immutable SkBitmap to be transferred across threads

Bug: 1241091
Change-Id: Ia771893245ef2fc1acb05cd4906d64bf2dd406fe
---

M third_party/blink/renderer/platform/graphics/skia/skia_utils.h
1 file changed, 30 insertions(+), 0 deletions(-)

diff --git a/third_party/blink/renderer/platform/graphics/skia/skia_utils.h b/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
index 6765185..b7eea38 100644
--- a/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
+++ b/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
@@ -39,7 +39,9 @@
 #include "third_party/blink/renderer/platform/graphics/image.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/transforms/affine_transform.h"
+#include "third_party/blink/renderer/platform/wtf/cross_thread_copier.h"
 #include "third_party/blink/renderer/platform/wtf/math_extras.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkData.h"
@@ -200,4 +202,20 @@
 
 }  // namespace blink
 
+namespace WTF {
+


+template <>
+struct CrossThreadCopier<SkBitmap> {
+  STATIC_ONLY(CrossThreadCopier);
+
+  using Type = SkBitmap;
+  static SkBitmap Copy(const SkBitmap& bitmap) = delete;
+  static SkBitmap Copy(SkBitmap&& bitmap) {
+    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";
+    return bitmap;
+  }
+};
+

+}  // namespace WTF
+
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_SKIA_SKIA_UTILS_H_

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 6:24:23 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

Yutaka Hirano uploaded patch set #3 to this change.

View Change

Introduce CrossThreadCopier<SkBitmap>

Allow immutable SkBitmap to be transferred across threads.



Bug: 1241091
Change-Id: Ia771893245ef2fc1acb05cd4906d64bf2dd406fe
---
M third_party/blink/renderer/platform/graphics/skia/skia_utils.h
1 file changed, 30 insertions(+), 0 deletions(-)

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Florin Malita (Gerrit)

unread,

Oct 27, 2021, 9:19:29 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

3 comments:

Patchset:
- Patch Set #3:
  Some comments in cross_thread_copier.h [1] seem to indicate a preference for keeping specializations local. Could this be relocated to that header?
  [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/cross_thread_copier.h;drc=c7e95945a5e198023f3c9675db533f77ec0ade7f;l=213
File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #3, Line 214:

    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";

- ```
    return bitmap;
```
  This is a suitable implementation for the const ref version deleted above, as it makes a shallow copy of the bitmap (metadata only, guaranteed shared pixels).
  If only the rvalue ref version is required, then we don't need to make a copy at all, right? Just move/transfer both bitmap metadata and pixels ownership?
```
  return std::move(bitmap);
  
(no immutability check should be needed if we're not making an actual bitmap copy)
```
- Patch Set #3, Line 215: return bitmap;
  For this rvalue ref version, you can use the SkBitmap move constructor:
```
  return std::move(bitmap);
```

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Florin Malita (Gerrit)

unread,

Oct 27, 2021, 9:22:05 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

Patch Set #3, Line 214:

    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";
    return bitmap;

- This is a suitable implementation for the const ref version deleted above, as it makes a shallow cop […]
  Please ignore this comment, gerrit fail. The input bitmap could be sharing pixel refs with other bitmaps, so the immutability check is needed.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 27, 2021, 10:20:33 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;
  Is it normal to not define Copy from const& as well?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 10:34:43 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

View Change

2 comments:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;
  Is it normal to not define Copy from const& as well?

- It is not very common but that is valid I think. Please see CrossThreadCopier<std::unique_ptr<T, Deleter>>.
  https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/cross_thread_copier.h;l=216?q=cross_thread_copi&ss=chromium

- Patch Set #3, Line 215: return bitmap;

- For this rvalue ref version, you can use the SkBitmap move constructor: […]
  Done

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 27, 2021, 10:44:55 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Florin Malita, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;

- It is not very common but that is valid I think. […]
  Hm, but std::unique_ptr is move-only so can't be copied. SkBitmap can be copied relatively cheaply though. What reason to prevent it here?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 11:34:18 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;

- Hm, but std::unique_ptr is move-only so can't be copied. […]
  To avoid accidental copy, I guess.
  I can allow copying: do you want to require it to be immutable?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 11:39:11 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;

- To avoid accidental copy, I guess. […]
  PS5 allows copying. I rmeoved the CHECK for immutability.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 27, 2021, 11:52:38 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Florin Malita, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;

- PS5 allows copying. I rmeoved the CHECK for immutability.
  It should be immutable, otherwise it has to copy the pixel buffer.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Florin Malita (Gerrit)

unread,

Oct 27, 2021, 11:54:46 AM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Hiroshige Hayashizaki, Yutaka Hirano.

View Change

2 comments:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #5, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) { return bitmap; }
  We should keep the immutability check.
  The way skia bitmaps work is there's a cheap SkBitmap object which wraps an expensive shareable SkPixelRef.
  When you copy an SkBitmap, you're only copying the shallow part and the SkPixelRef is shared between copies: https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/core/SkBitmap.h;l=67
  This is only safe to do cross-thread when the bitmap is marked immutable (otherwise the shared pixel ref could be updated from a different thread).
- Patch Set #5, Line 213: static SkBitmap Copy(SkBitmap bitmap) { return bitmap; }
  Is this second version required? If so, it should also check immutability.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 12:01:15 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Florin Malita, Hiroshige Hayashizaki.

View Change

2 comments:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #5, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) { return bitmap; }

- We should keep the immutability check. […]
  Done

- Patch Set #5, Line 213: static SkBitmap Copy(SkBitmap bitmap) { return bitmap; }
  Is this second version required? If so, it should also check immutability.

- Done

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 12:01:25 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Florin Malita, Hiroshige Hayashizaki.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #3, Line 212: static SkBitmap Copy(const SkBitmap& bitmap) = delete;

- It should be immutable, otherwise it has to copy the pixel buffer.

- Done

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Florin Malita (Gerrit)

unread,

Oct 27, 2021, 12:24:23 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki, Yutaka Hirano.

Patch set 6:Code-Review +1

View Change

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

vmpstr (Gerrit)

unread,

Oct 27, 2021, 12:47:56 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #6, Line 216: static SkBitmap Copy(SkBitmap bitmap) {
  Is there an advantage in having this version along with the above?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 27, 2021, 12:55:38 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: vmpstr, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #6, Line 216: static SkBitmap Copy(SkBitmap bitmap) {
  Is there an advantage in having this version along with the above?

- I think this one should be a SkBitmap&& and move() the bitmap. The impl would be about the same as the copying one though, perhaps skipping an atomic refcount on the SkPixelRef

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

vmpstr (Gerrit)

unread,

Oct 27, 2021, 1:42:41 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #6, Line 216: static SkBitmap Copy(SkBitmap bitmap) {

- I think this one should be a SkBitmap&& and move() the bitmap. […]
  I think maybe this is the only version that's needed for that. If it's an lvalue that's passed, it will make a copy when accepting it as a parameter and then move it in return. If it's an rvalue it will use a move ctor to construct it and then also move it in return. Does that make sense?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 27, 2021, 2:46:06 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: vmpstr, Hiroshige Hayashizaki, Yutaka Hirano.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #6, Line 216: static SkBitmap Copy(SkBitmap bitmap) {

- I think maybe this is the only version that's needed for that. […]
  Yes, we can write just a receive-by-value as a single version. Or we can write a const& and a && version.
  The latter has a chance to be more performant but it's all relatively moot here, I think.
  The worst case is mixing const& or && with a by-value as they overlap, as you have pointed out.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 27, 2021, 10:16:13 PM10/27/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: vmpstr, danakj, Hiroshige Hayashizaki.

View Change

1 comment:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #6, Line 216: static SkBitmap Copy(SkBitmap bitmap) {

- Yes, we can write just a receive-by-value as a single version. […]
  Done

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

Oct 28, 2021, 2:08:42 AM10/28/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Yutaka Hirano.

View Change

3 comments:

Commit Message:
- Patch Set #7, Line 11: Bug: 1241091
  nit: could you cc me on the crbug?
File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #7, Line 207: template <>
  In general, declaring `CrossThreadCopier<T>` separately (i.e. in other header files) from `CrossThreadCopier` and `T` has a risk of violating the One Definition Rule (`CrossThreadCopier<SkBitmap>` is used when `skia_utils.h` is included, and not used when not included), while this might be hard because SkBitmap is outside Blink.
  Is it possible to declare this in third_party/blink/renderer/platform/wtf/cross_thread_copier.h ?
  Maybe not, and then probably we should add some comments.
- Patch Set #7, Line 208: CrossThreadCopierPassThrough<SkBitmap>
  Is this `CrossThreadCopierPassThrough` needed?
  We have a complete `Copy(const SkBitmap& bitmap)` below.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 28, 2021, 3:29:58 AM10/28/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki.

View Change

3 comments:

Commit Message:

- Patch Set #7, Line 11: Bug: 1241091
  nit: could you cc me on the crbug?

- Done
File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #7, Line 207: template <>

- In general, declaring `CrossThreadCopier<T>` separately (i.e. […]
  That depends on whether WTF wants to depend on skia. Currently including "third_party/skia/include/core/SkBitmap.h" leads to a compile error [1].
  1: https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1000906/overview

- Patch Set #7, Line 208: CrossThreadCopierPassThrough<SkBitmap>

- Is this `CrossThreadCopierPassThrough` needed? […]
  Oh sorry we don't need this. Fixed.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 28, 2021, 9:52:06 PM10/28/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, danakj, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: danakj, Hiroshige Hayashizaki.

View Change

1 comment:

Patchset:
- Patch Set #8:
  danakji@, hiroshige@, are you fine with the latest CL?

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Oct 29, 2021, 9:21:38 AM10/29/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Hiroshige Hayashizaki, Yutaka Hirano.

Patch set 8:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #8:
  LGTM

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Oct 29, 2021, 11:36:19 AM10/29/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, danakj, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Hiroshige Hayashizaki.

Patch set 8:Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #8:
  I'm landing this now to unblock the https://crrev.com/c/3222803. hiroshige@, if you have further comments I'll be happy to address them in a follow-up CL.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Oct 29, 2021, 12:54:56 PM10/29/21

to Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, danakj, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  danakj: Looks good to me
  Florin Malita: Looks good to me
  Yutaka Hirano: Commit

Introduce CrossThreadCopier<SkBitmap>

Allow immutable SkBitmap to be transferred across threads.

Bug: 1241091
Change-Id: Ia771893245ef2fc1acb05cd4906d64bf2dd406fe

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3244824
Reviewed-by: danakj <dan...@chromium.org>
Reviewed-by: Florin Malita <fma...@chromium.org>
Commit-Queue: Yutaka Hirano <yhi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936435}
---
M third_party/blink/renderer/platform/graphics/skia/skia_utils.h
1 file changed, 40 insertions(+), 0 deletions(-)

diff --git a/third_party/blink/renderer/platform/graphics/skia/skia_utils.h b/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
index 6765185..f12204f 100644


--- a/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
+++ b/third_party/blink/renderer/platform/graphics/skia/skia_utils.h
@@ -39,7 +39,9 @@
 #include "third_party/blink/renderer/platform/graphics/image.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/transforms/affine_transform.h"
+#include "third_party/blink/renderer/platform/wtf/cross_thread_copier.h"
 #include "third_party/blink/renderer/platform/wtf/math_extras.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkData.h"

@@ -200,4 +202,25 @@


 
 }  // namespace blink
 
+namespace WTF {
+

+// We define CrossThreadCopier<SKBitMap> here because we cannot include skia
+// headers in platform/wtf.


+template <>
+struct CrossThreadCopier<SkBitmap> {
+  STATIC_ONLY(CrossThreadCopier);
+
+  using Type = SkBitmap;

+  static SkBitmap Copy(const SkBitmap& bitmap) {


+    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";
+    return bitmap;
+  }
+  static SkBitmap Copy(SkBitmap&& bitmap) {

+    CHECK(bitmap.isImmutable()) << "Only immutable bitmaps can be transferred.";
+    return std::move(bitmap);


+  }
+};
+
+}  // namespace WTF
+
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_SKIA_SKIA_UTILS_H_

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Rayan Kanso (Gerrit)

unread,

Oct 29, 2021, 12:57:12 PM10/29/21

to Chromium LUCI CQ, Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, danakj, vmpstr, Florin Malita, Hiroshige Hayashizaki, chromium...@chromium.org

Attention is currently required from: Yutaka Hirano.

View Change

2 comments:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #9, Line 219: isImmutable
  I think the check should be bitmap.isImmutable() || bitmap.isNull()
  null SkBitmaps are not immutable, and will still fail the `isImmutable` check even if you explicitly call setImmutable on the object.
  Passing default-initialized SkBitmaps is a common use case for when things go wrong.
File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:
- Patch Set #8, Line 215: CHECK(bitmap.isImmutable
  Should this be a DCHECK? a CHECK seems extreme.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Yutaka Hirano (Gerrit)

unread,

Nov 1, 2021, 12:14:03 AM11/1/21

to Chromium LUCI CQ, Yutaka Hirano, blink-re...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, danakj, vmpstr, Florin Malita, Hiroshige Hayashizaki, Rayan Kanso, chromium...@chromium.org

View Change

2 comments:

File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #9, Line 219: isImmutable

- I think the check should be bitmap.isImmutable() || bitmap.isNull() […]
  Oh thank you, I overlooked that. I'll make a follow-up change.
File third_party/blink/renderer/platform/graphics/skia/skia_utils.h:

- Patch Set #8, Line 215: CHECK(bitmap.isImmutable
  Should this be a DCHECK? a CHECK seems extreme.

- I think CHECK is fine in this case, given it's difficult to detect and reproduce thread race issues.

To view, visit change 3244824. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward