Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770693002:
ash: HiDPI user avatar for SessionController (Closed)
|
Created:
3 years, 9 months ago by xiyuan Modified:
3 years, 9 months ago CC:
chromium-reviews, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, rsesek+watch_chromium.org, kalyank, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionash: HiDPI user avatar for SessionController
- Add mojo structs for ImageSkia and ImageSkiaRep;
- Use ImageSkia instead of SkBitmap for user avatar;
BUG=702689, 655874
TBR=ben@chromium.org
Review-Url: https://codereview.chromium.org/2770693002
Cr-Commit-Position: refs/heads/master@{#459804}
Committed: https://chromium.googlesource.com/chromium/src/+/fda72a9f6483a0cf08aa68091eb8f8af464bec64
Patch Set 1 #Patch Set 2 : rebase, use shared buffer, add tests #
Total comments: 6
Patch Set 3 : fix gn check #
Total comments: 50
Patch Set 4 : buffer size uint32 -> uint64, support 0.0f scale #Patch Set 5 : bail on negative scale when Read ImageSkiaRep #
Total comments: 4
Patch Set 6 : for msw comments, split SharedBufferSkBitmap out of ImageRep #Patch Set 7 : fix compile #
Total comments: 8
Patch Set 8 : make mojo similar to c++, update test #
Total comments: 4
Patch Set 9 : nits, and gn check #
Dependent Patchsets: Messages
Total messages: 56 (39 generated)
Description was changed from ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and friends; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689 ========== to ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and ImageSkiaRep; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689, 655874 ==========
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiyuan@chromium.org changed reviewers: + msw@chromium.org, tsepez@chromium.org
A shared buffer based mojo transport for ImageSkia(Rep) and use it for user avatar. msw@: please drive the review. tsepez@: mojo changes listed below ash/public/interfaces/session_controller.mojom ui/gfx/image/mojo/image.typemap ui/gfx/image/mojo/image_skia.mojom ui/gfx/image/mojo/image_skia_struct_traits.cc ui/gfx/image/mojo/image_skia_struct_traits.h ui/gfx/image/mojo/image_traits_test_service.mojom Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:13: uint32 buffer_byte_size; Could it be bigger than this on 64bit platforms? Do we validate this against info from the actual handle itself? https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:16: float scale; Can this be zero or negative?
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:13: uint32 buffer_byte_size; On 2017/03/23 18:11:39, Tom Sepez wrote: > Could it be bigger than this on 64bit platforms? Maybe in the future. For now, I don't think we use such huge images. uint32 is 4G, rought 1G ARGB pixels. And if image is this big, we should probably not use this transport. How about add a CHECK in traits to ensure we don't overflow? > Do we validate this against info from the actual handle itself? If the info is incorrect, I imagine the Map call would fail and current code bail on that. Is this good enough? If not, is there a way to get the size info from the handle? I did not find out how to do that. https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:16: float scale; On 2017/03/23 18:11:39, Tom Sepez wrote: > Can this be zero or negative? It should never be <= 0. I could add DCHECK in traits. Is there other way you want to add to enforce this?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:13: uint32 buffer_byte_size; On 2017/03/23 18:30:41, xiyuan wrote: > On 2017/03/23 18:11:39, Tom Sepez wrote: > > Could it be bigger than this on 64bit platforms? > > Maybe in the future. For now, I don't think we use such huge images. uint32 is > 4G, rought 1G ARGB pixels. And if image is this big, we should probably not use > this transport. How about add a CHECK in traits to ensure we don't overflow? > > > Do we validate this against info from the actual handle itself? > > If the info is incorrect, I imagine the Map call would fail and current code > bail on that. Is this good enough? > > If not, is there a way to get the size info from the handle? I did not find out > how to do that. Changed uint32 -> uint64 and added a DCHECK for huge images (> 4k x 4k). https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:16: float scale; On 2017/03/23 18:30:41, xiyuan wrote: > On 2017/03/23 18:11:39, Tom Sepez wrote: > > Can this be zero or negative? > > It should never be <= 0. I could add DCHECK in traits. Is there other way you > want to add to enforce this? Actually, the scale can be 0 for an unscaled ImageSkiaRep. Fixed that and added a test case. Also added a DCHECK to catch scale < 0 case.
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Changed uint32 -> uint64 and added a DCHECK for huge images (> 4k x 4k). > > https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... > ui/gfx/image/mojo/image_skia.mojom:16: float scale; > On 2017/03/23 18:30:41, xiyuan wrote: > > On 2017/03/23 18:11:39, Tom Sepez wrote: > > > Can this be zero or negative? > > > > It should never be <= 0. I could add DCHECK in traits. Is there other way you > > want to add to enforce this? > > Actually, the scale can be 0 for an unscaled ImageSkiaRep. Fixed that and added > a test case. Also added a DCHECK to catch scale < 0 case. Are these being read in a more privileged process that in which they are written? If so, we'll want to return false from read rather than DCHECK() which doesn't actually produce any code in a release build.
Patchset #5 (id:80001) has been deleted
On 2017/03/23 20:02:40, Tom Sepez wrote: > > Changed uint32 -> uint64 and added a DCHECK for huge images (> 4k x 4k). > > > > > https://codereview.chromium.org/2770693002/diff/20001/ui/gfx/image/mojo/image... > > ui/gfx/image/mojo/image_skia.mojom:16: float scale; > > On 2017/03/23 18:30:41, xiyuan wrote: > > > On 2017/03/23 18:11:39, Tom Sepez wrote: > > > > Can this be zero or negative? > > > > > > It should never be <= 0. I could add DCHECK in traits. Is there other way > you > > > want to add to enforce this? > > > > Actually, the scale can be 0 for an unscaled ImageSkiaRep. Fixed that and > added > > a test case. Also added a DCHECK to catch scale < 0 case. > > Are these being read in a more privileged process that in which they are > written? If so, we'll want to return false from read rather than DCHECK() which > doesn't actually produce any code in a release build. Right. Made ImageSkiaRep trait Read to return false on negative scale. This CL uses the mojo structs to send images from browser to ash. Browser is a lower privileged process and ash should be protected.
This is awesome, thank you for working on this! https://codereview.chromium.org/2770693002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/session_controller_client.cc (right): https://codereview.chromium.org/2770693002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/session_controller_client.cc:65: // Make sure all representations are loaded before sending via mojo. Hmm, I wonder if there's a way to ensure we always do this; can this be moved into the struct traits? (also, do we need to call SetReadOnly() or MakeThreadSafe() before allowing shared memory access?) https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD.gn File ui/gfx/image/mojo/BUILD.gn (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:9: "image_skia.mojom", optional: do you think it'd make sense to name this image.mojom and hope that we'll get a struct corresponding to gfx::Image here soon? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:13: "//skia/public/interfaces", I'm not sure this dependency is necessary (not used in mojom) https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:32: "image_traits_test_service.mojom", Hopefully we can testing the struct traits without a test service. See the recent "Testing typemaps" mail: https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/_m5qIDZgAf8/di... I used that guidance to add this test: https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_struct_traits_unitt... https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:7: // Mojo transport for ImageSkiaRep via shared buffer. I wonder if we should [add a todo/bug comment to] make the shared buffer work at the SkBitmap level (and then maybe skip ImageSkiaRep, and let ImageSkia simply be an array of SkBitmaps)? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:9: // Shared buffer handle to holds a serialized SkBitmap. nit: "to hold" or "that holds" https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:10: handle<shared_buffer> shared_buffer_handle; I'm not well versed with shared memory patterns (mojo or otherwise). I don't have any specific concerns, but you may wish to include an additional reviewer to ensure we haven't missed anything. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:20: // buffer. Note it is caller's responsibility to ensure the ImageSkia contains nit: "Note that it is the caller's" https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:24: skia::mojom::Bitmap::Serialize(&(input.sk_bitmap()))); q: Do we need to make read only or make thread safe, etc. before this? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:26: // Use a context to serialize bitmap to a shared buffer only once. nit: "the bitmap" https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:33: memcpy(mapping.get(), serialized_sk_bitmap.data(), context->buffer_byte_size); hmm, I would have thought we could avoid a memcpy by somehow mapping the buffer to the existing SkBitmap's underlying data, but perhaps that's not possible. In any case, we can work to improve performance later, having a working mapping at all is a great step forward. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:78: if (!skia::mojom::Bitmap::Deserialize(serialized_sk_bitmap, &bitmap)) Again, I'm wondering if there's a more performance-focused way to do this, but that can probably be addressed later; this is likely good enough for now. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:117: return *(static_cast<std::vector<gfx::ImageSkiaRep>*>(context)); optional nit: // See the comment in SetUpContext regarding context usage. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia_struct_traits.h (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.h:39: return input.sk_bitmap().drawsNothing(); nit: use ImageSkiaRep::is_null() or input.sk_bitmap().isNull()? (I'm not exactly sure what the tradeoff is here...) https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.h:56: return input.image_reps().empty(); q: I wonder if it's always safe to call this; image_reps() has CHECK(CanRead()); https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_traits_unittest.cc (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:23: bool ContainsSameRepresentation(const std::vector<ImageSkiaRep>& reps, nit: look at gfx::test::ImageSkiaStructureMatches, maybe that'd be useful? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:25: for (const auto& rep : reps) { Should we check that image.image_reps().size() == reps.size()? (I guess that wouldn't work for the null/empty reps that were omitted) Similarly, I wonder if we should check that |image| doesn't contain additional reps that are missing from |reps|, but that wouldn't be the case for null/empty reps that were omitted. Maybe we could just check that all other reps of |image| not in |reps| are null/empty? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:28: image.GetRepresentation(rep.scale()).sk_bitmap())) { Should we be checking HasRepresentation before calling GetRepresentation? (we likely wouldn't want this to generate any reps that didn't exist before) https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:35: class TestImageSkiaSource : public ImageSkiaSource { nit: add a comment https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:52: class ImageTraitsTest : public testing::Test, See my earlier comment; hopefully the test service approach isn't needed. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:96: empty_bitmap.allocN32Pixels(0, 0); optional nit: check if the bitmap is null? https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:97: ImageSkiaRep image_rep(empty_bitmap, 1.0f); nit: after this, ASSERT_TRUE(image_rep.is_null()); optionally name |empty_rep| https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:129: TEST_F(ImageTraitsTest, ImageSkiaWithNoRepsTreatedAsNull) { Hmm, I wonder if this behavior will bite us later... https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:137: optional nit: remove blank line https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/typemaps.gni File ui/gfx/typemaps.gni (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/typemaps.gni#new... ui/gfx/typemaps.gni:7: "//ui/gfx/image/mojo/image.typemap", wdyt about keeping the image mojo stuff in //ui/gfx/mojo? https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia.mojom:15: // Corresponding scale of the bitmap. nit: "or 0 if unscaled"? Maybe explain what that means? https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:75: // An acceptable scale must be greater or equal to 0. nit: "greater than"
lgtm
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2770693002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/session_controller_client.cc (right): https://codereview.chromium.org/2770693002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/session_controller_client.cc:65: // Make sure all representations are loaded before sending via mojo. On 2017/03/23 20:47:51, msw wrote: > Hmm, I wonder if there's a way to ensure we always do this; can this be moved > into the struct traits? (also, do we need to call SetReadOnly() or > MakeThreadSafe() before allowing shared memory access?) I left this part out of traits because only caller knows exactly what should be transferred and can limit memory usage by only loading what are needed. However, I agree it would make it error prone and harder to write code. Moved EnsureRepsForSupportedScales() to struct trait. MakeThreadSafe() is not need to for using shared memory. A SkBitmap in ImageSkia/ImageSkiaRep is immutable [1] so copying its pixel data should not be problem. Also MakeThreadSafe() has a side effect that removes ImageSkiaSource from storage. This might not be the right thing. A few ImageSkia is implemented to get representation asynchronously, e.g extensions::IconImage::Source[2]. EnsureRepsForSupportedScales would trigger the load and the ImageSkia would be filled with correct reps later. But if MakeThreadSafe is used, the source is deleted at the call and we get stub image reps instead. [1]: https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=d62fe... [2]: https://cs.chromium.org/chromium/src/extensions/browser/extension_icon_image.... https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD.gn File ui/gfx/image/mojo/BUILD.gn (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:9: "image_skia.mojom", On 2017/03/23 20:47:51, msw wrote: > optional: do you think it'd make sense to name this image.mojom and hope that > we'll get a struct corresponding to gfx::Image here soon? Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:13: "//skia/public/interfaces", On 2017/03/23 20:47:51, msw wrote: > I'm not sure this dependency is necessary (not used in mojom) Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/BUILD... ui/gfx/image/mojo/BUILD.gn:32: "image_traits_test_service.mojom", On 2017/03/23 20:47:51, msw wrote: > Hopefully we can testing the struct traits without a test service. > See the recent "Testing typemaps" mail: > https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/_m5qIDZgAf8/di... > I used that guidance to add this test: > https://cs.chromium.org/chromium/src/ash/public/cpp/shelf_struct_traits_unitt... Good to know. Ken confirmed that it does not support handles ATM. I will keep using this test service for all test cases to make it consistent. Add a comment of why a test service is used. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:7: // Mojo transport for ImageSkiaRep via shared buffer. On 2017/03/23 20:47:51, msw wrote: > I wonder if we should [add a todo/bug comment to] make the shared buffer work at > the SkBitmap level (and then maybe skip ImageSkiaRep, and let ImageSkia simply > be an array of SkBitmaps)? I think that would be helpful - Modified the CL to extract the shared buffer part out as SharedBufferSkBitmap and map it to SkBitmap. - I left a TODO comment to move SharedBufferSkBitmap into skia and use shared buffer more efficiently. And we could use gfx.mojom.SharedBufferSkBitmap ATM to use SkBitmap with mojo via shared buffer. I want to keep mojom.ImageSkiaRep because it stores a scale factor. Storing the bitmap with its corresponding scale together is a plus to me. Otherwise, we need to put scale in mojom.ImageSkia and has to keep the order. And this way is more similar to our C++ side classes. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:9: // Shared buffer handle to holds a serialized SkBitmap. On 2017/03/23 20:47:51, msw wrote: > nit: "to hold" or "that holds" Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:10: handle<shared_buffer> shared_buffer_handle; On 2017/03/23 20:47:51, msw wrote: > I'm not well versed with shared memory patterns (mojo or otherwise). I don't > have any specific concerns, but you may wish to include an additional reviewer > to ensure we haven't missed anything. Acknowledged. Will ask sadrul@ to take a look. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia.mojom:20: // buffer. Note it is caller's responsibility to ensure the ImageSkia contains On 2017/03/23 20:47:51, msw wrote: > nit: "Note that it is the caller's" Update comment to reflect that we do MakeThreadSafe() in struct traits. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:24: skia::mojom::Bitmap::Serialize(&(input.sk_bitmap()))); On 2017/03/23 20:47:51, msw wrote: > q: Do we need to make read only or make thread safe, etc. before this? We dont' need to. A SkBitmap in ImageSkiaRep is immutable. [1] [1]: https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia_rep.cc?rcl=1597c... https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:26: // Use a context to serialize bitmap to a shared buffer only once. On 2017/03/23 20:47:51, msw wrote: > nit: "the bitmap" Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:33: memcpy(mapping.get(), serialized_sk_bitmap.data(), context->buffer_byte_size); On 2017/03/23 20:47:51, msw wrote: > hmm, I would have thought we could avoid a memcpy by somehow mapping the buffer > to the existing SkBitmap's underlying data, but perhaps that's not possible. In > any case, we can work to improve performance later, having a working mapping at > all is a great step forward. I would love to do that as well but not sure how feasible it is either. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:78: if (!skia::mojom::Bitmap::Deserialize(serialized_sk_bitmap, &bitmap)) On 2017/03/23 20:47:52, msw wrote: > Again, I'm wondering if there's a more performance-focused way to do this, but > that can probably be addressed later; this is likely good enough for now. Yes, I am not happy about this too. :( Right now, we copy the pixels *4* times for the transport: serialize to a vector, copy to shared buffer, copy from shared buffer to a vector, deserialize. We can get rid of two (to and from vector) when merging into bitmap struct traits. Will do that in a follow up. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.cc:117: return *(static_cast<std::vector<gfx::ImageSkiaRep>*>(context)); On 2017/03/23 20:47:52, msw wrote: > optional nit: // See the comment in SetUpContext regarding context usage. Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_skia_struct_traits.h (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.h:39: return input.sk_bitmap().drawsNothing(); On 2017/03/23 20:47:52, msw wrote: > nit: use ImageSkiaRep::is_null() or input.sk_bitmap().isNull()? > (I'm not exactly sure what the tradeoff is here...) is_null does not check empty bitmap (0x0 image). drawsNothing covers both cases and it is cheap to call. https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkBitmap.h... https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_skia_struct_traits.h:56: return input.image_reps().empty(); On 2017/03/23 20:47:52, msw wrote: > q: I wonder if it's always safe to call this; image_reps() has CHECK(CanRead()); CanRead() only fails if ImageSkia is not marked as thread safe and is accessed from a thread different from where it is created. We should be good now since we call MakeThreadSafe(). https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... File ui/gfx/image/mojo/image_traits_unittest.cc (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:23: bool ContainsSameRepresentation(const std::vector<ImageSkiaRep>& reps, On 2017/03/23 20:47:52, msw wrote: > nit: look at gfx::test::ImageSkiaStructureMatches, maybe that'd be useful? Acknowledged. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:25: for (const auto& rep : reps) { On 2017/03/23 20:47:52, msw wrote: > Should we check that image.image_reps().size() == reps.size()? > (I guess that wouldn't work for the null/empty reps that were omitted) > > Similarly, I wonder if we should check that |image| doesn't contain additional > reps that are missing from |reps|, but that wouldn't be the case for null/empty > reps that were omitted. > > Maybe we could just check that all other reps of |image| not in |reps| are > null/empty? Done. Revised code to get image_reps() from |image|, strip out null/empty ones, then compare the two list. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:28: image.GetRepresentation(rep.scale()).sk_bitmap())) { On 2017/03/23 20:47:52, msw wrote: > Should we be checking HasRepresentation before calling GetRepresentation? (we > likely wouldn't want this to generate any reps that didn't exist before) Good point. GetRepresentation should not be used. Code revised and should no longer has the problem. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:35: class TestImageSkiaSource : public ImageSkiaSource { On 2017/03/23 20:47:52, msw wrote: > nit: add a comment Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:52: class ImageTraitsTest : public testing::Test, On 2017/03/23 20:47:52, msw wrote: > See my earlier comment; hopefully the test service approach isn't needed. Tried it and handles are not supported in Deserialize(Serialize()) API. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:96: empty_bitmap.allocN32Pixels(0, 0); On 2017/03/23 20:47:52, msw wrote: > optional nit: check if the bitmap is null? Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:97: ImageSkiaRep image_rep(empty_bitmap, 1.0f); On 2017/03/23 20:47:52, msw wrote: > nit: after this, ASSERT_TRUE(image_rep.is_null()); optionally name |empty_rep| This is the tricky part, might be a bug with current code. ImageSkiaRep with empty SkBitmap is not considered as null. is_null() returns false. Thus it would be included in the reps returned from ImageSkia::image_reps(). https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:129: TEST_F(ImageTraitsTest, ImageSkiaWithNoRepsTreatedAsNull) { On 2017/03/23 20:47:52, msw wrote: > Hmm, I wonder if this behavior will bite us later... Possible. But there is not much we could do at this level. Only the caller of us knows how to fill up the reps. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/image/mojo/image... ui/gfx/image/mojo/image_traits_unittest.cc:137: On 2017/03/23 20:47:52, msw wrote: > optional nit: remove blank line Done. https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/typemaps.gni File ui/gfx/typemaps.gni (right): https://codereview.chromium.org/2770693002/diff/40001/ui/gfx/typemaps.gni#new... ui/gfx/typemaps.gni:7: "//ui/gfx/image/mojo/image.typemap", On 2017/03/23 20:47:52, msw wrote: > wdyt about keeping the image mojo stuff in //ui/gfx/mojo? I don't have a strong preference. Let me know what you feel best. https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia.mojom (right): https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia.mojom:15: // Corresponding scale of the bitmap. On 2017/03/23 20:47:53, msw wrote: > nit: "or 0 if unscaled"? Maybe explain what that means? Done. https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/100001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:75: // An acceptable scale must be greater or equal to 0. On 2017/03/23 20:47:53, msw wrote: > nit: "greater than" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice work, and thanks for the good explanations/answers. Just a few questions and a minor nit left. https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.mojom (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:9: // to make less copies. optional nit: indent two more spaces? https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:28: // be made thread-safe (all reps loaded and be immutable) after using this. Does calling EnsureRepsForSupportedScales actually make the ImageSkia immutable and thread safe? I'm not sure about that. https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.typemap (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.typemap:18: "gfx.mojom.SharedBufferSkBitmap=SkBitmap[nullable_is_same_type]", Is there any worry in aliasing two mojom types to one c++ type? https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:16: // Strip out empty image reps. Needed because ImageSkia::image_reps does not I wonder if filtering out empty image reps is an unnecessary step that makes our traits code more complicated than it should be. What's the harm in keeping these? Wouldn't that be more like the c++ class?
https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.mojom (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:9: // to make less copies. On 2017/03/24 17:59:32, msw wrote: > optional nit: indent two more spaces? Done. https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:28: // be made thread-safe (all reps loaded and be immutable) after using this. On 2017/03/24 17:59:32, msw wrote: > Does calling EnsureRepsForSupportedScales actually make the ImageSkia immutable > and thread safe? I'm not sure about that. Oops. Forgot to update this after switching back from MakeThreadSafe. EnsureRepsForSupportedScales only loads the reps. Comments changed. https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.typemap (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.typemap:18: "gfx.mojom.SharedBufferSkBitmap=SkBitmap[nullable_is_same_type]", On 2017/03/24 17:59:32, msw wrote: > Is there any worry in aliasing two mojom types to one c++ type? It seems to me that the mojo side must be unique but C++ side does not have to be. The mojo template is smart enough to figure that out because it knows which mojo type will be used. And yes, we should move this into //skia, not only it is cleaner but also saves two memory copies. I will have a follow up CL, hopefully next week. https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/140001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:16: // Strip out empty image reps. Needed because ImageSkia::image_reps does not On 2017/03/24 17:59:33, msw wrote: > I wonder if filtering out empty image reps is an unnecessary step that makes our > traits code more complicated than it should be. What's the harm in keeping > these? Wouldn't that be more like the c++ class? My intention is to save some data to be transported. Maybe not worth it. Changed to just use what C++ class provides to keep it more straightforward.
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Nice, lgtm with a couple minor optional nits. Thank you! https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.mojom (right): https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:27: // Mojo transport for an ImageSkia via shared buffer. Note the ImageSkia would optional nit: "Note that transporting an ImageSkia over mojo will load all of its image representations for supported scales." https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:119: auto* image_reps = new std::vector<gfx::ImageSkiaRep>(input.image_reps()); optional nit: inline |return new std::vector|...
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image.mojom (right): https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image.mojom:27: // Mojo transport for an ImageSkia via shared buffer. Note the ImageSkia would On 2017/03/24 18:47:04, msw wrote: > optional nit: "Note that transporting an ImageSkia over mojo will load all of > its image representations for supported scales." Nice. Thanks. https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... File ui/gfx/image/mojo/image_skia_struct_traits.cc (right): https://codereview.chromium.org/2770693002/diff/160001/ui/gfx/image/mojo/imag... ui/gfx/image/mojo/image_skia_struct_traits.cc:119: auto* image_reps = new std::vector<gfx::ImageSkiaRep>(input.image_reps()); On 2017/03/24 18:47:04, msw wrote: > optional nit: inline |return new std::vector|... Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiyuan@chromium.org changed reviewers: + ben@chromium.org, sky@chromium.org
sky@, could you do owner reivew for ash/* ?
ben@, could you approve the gfx/image.mojom dependency on
'+mojo/public',
'+skia/public/interfaces',
Thanks.
ash LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and ImageSkiaRep; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689, 655874 ========== to ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and ImageSkiaRep; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689, 655874 TBR=ben@chromium.org ==========
The CQ bit was checked by xiyuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2770693002/#ps180001 (title: "nits, and gn check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490630093133180,
"parent_rev": "86f58fb4b5a14bdd59b12c31fdd5b0761a84b5ff", "commit_rev":
"fda72a9f6483a0cf08aa68091eb8f8af464bec64"}
Message was sent while issue was closed.
Description was changed from ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and ImageSkiaRep; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689, 655874 TBR=ben@chromium.org ========== to ========== ash: HiDPI user avatar for SessionController - Add mojo structs for ImageSkia and ImageSkiaRep; - Use ImageSkia instead of SkBitmap for user avatar; BUG=702689, 655874 TBR=ben@chromium.org Review-Url: https://codereview.chromium.org/2770693002 Cr-Commit-Position: refs/heads/master@{#459804} Committed: https://chromium.googlesource.com/chromium/src/+/fda72a9f6483a0cf08aa68091eb8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/fda72a9f6483a0cf08aa68091eb8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
