Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551733002:
Converting Mus Demo to use Aura. (Closed)
|
Created:
4 years ago by mfomitchev Modified:
4 years ago CC:
chromium-reviews, rjkroege, Jay Civelli Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConverting Mus Demo to use Aura.
Drawing to a Layer added to the root window provided by the WindowTreeHost
instead of uploading directly to the GPU ourselves.
BUG=NONE
Committed: https://crrev.com/698a7d3936aa4720231205775de527554d36d646
Cr-Commit-Position: refs/heads/master@{#436448}
Patch Set 1 #Patch Set 2 : Adding ui/compositor to deps. #
Total comments: 6
Patch Set 3 : Addressing feedback. #
Total comments: 3
Messages
Total messages: 32 (19 generated)
The CQ bit was checked by mfomitchev@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_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 mfomitchev@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...
mfomitchev@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:76: void SetImage(gfx::Image image) { image_ = image; } const gfx::Image& https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:33: namespace aura { As this file is the directory services/ui it should remain the ui namespace. https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:115: // Layer to which we draw the bitmap. Is there a reason you create a layer vs an aura::Window? Creating an aura::Window seems mildly easier.
The CQ bit was checked by mfomitchev@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/2551733002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:76: void SetImage(gfx::Image image) { image_ = image; } On 2016/12/05 16:06:41, sky wrote: > const gfx::Image& Done (removed). https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:33: namespace aura { On 2016/12/05 16:06:41, sky wrote: > As this file is the directory services/ui it should remain the ui namespace. Done. https://codereview.chromium.org/2551733002/diff/20001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:115: // Layer to which we draw the bitmap. On 2016/12/05 16:06:41, sky wrote: > Is there a reason you create a layer vs an aura::Window? Creating an > aura::Window seems mildly easier. Done. Switched to Window.
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:228: // Re-initialize the bitmap Does this need to realloc the bitmap memory every frame, even if the size doesn't change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:228: // Re-initialize the bitmap On 2016/12/05 22:02:30, kylechar wrote: > Does this need to realloc the bitmap memory every frame, even if the size > doesn't change? There's probably a better way to do this, but when I was trying to reuse the bitmap without a realloc, I was tripping over the SkASSERT in notifyPixelChanged(): https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkBitmap.cpp?q...
LGTM
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE ========== to ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE ==========
Kyle, I am going to try to land this, but if you can suggest a way to avoid a realloc, let me know, and I can address that in a follow up. I don't think it matters too much since it is a demo, but if there's a reasonably easy way, I'll fix it.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480980179080270,
"parent_rev": "9d6250ffc97cf9f9f345358053a34fb63fe110ff", "commit_rev":
"b0b8ab82a1cfa686c65dd506a8a2019db0bb80ef"}
Message was sent while issue was closed.
Description was changed from ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE ========== to ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE ========== to ========== Converting Mus Demo to use Aura. Drawing to a Layer added to the root window provided by the WindowTreeHost instead of uploading directly to the GPU ourselves. BUG=NONE Committed: https://crrev.com/698a7d3936aa4720231205775de527554d36d646 Cr-Commit-Position: refs/heads/master@{#436448} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/698a7d3936aa4720231205775de527554d36d646 Cr-Commit-Position: refs/heads/master@{#436448}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.cc (right): https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo.cc:228: // Re-initialize the bitmap On 2016/12/05 23:07:14, mfomitchev wrote: > On 2016/12/05 22:02:30, kylechar wrote: > > Does this need to realloc the bitmap memory every frame, even if the size > > doesn't change? > > There's probably a better way to do this, but when I was trying to reuse the > bitmap without a realloc, I was tripping over the SkASSERT in > notifyPixelChanged(): > https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkBitmap.cpp?q... Just had a chance to look at this in a bit of detail. The change is actually better than before. We were making a copy of the SkBitmap pixel buffer anyways. So it had to write the pixels, allocate a pixel buffer then copy everything. Now it just allocates then writes, no copy involved. Thankfully gfx::ImageSkiaRep sets the pixel buffer as immutable so we don't start overwriting the data with a new frame.
Message was sent while issue was closed.
I see. Thanks for the information! On Fri, Dec 16, 2016 at 9:28 AM <kylechar@chromium.org> wrote: > lgtm > > > > > > https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... > File services/ui/demo/mus_demo.cc (right): > > > https://codereview.chromium.org/2551733002/diff/40001/services/ui/demo/mus_de... > services/ui/demo/mus_demo.cc:228: // Re-initialize the bitmap > On 2016/12/05 23:07:14, mfomitchev wrote: > > On 2016/12/05 22:02:30, kylechar wrote: > > > Does this need to realloc the bitmap memory every frame, even if the > size > > > doesn't change? > > > > There's probably a better way to do this, but when I was trying to > reuse the > > bitmap without a realloc, I was tripping over the SkASSERT in > > notifyPixelChanged(): > > > > https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkBitmap.cpp?q... > > Just had a chance to look at this in a bit of detail. The change is > actually better than before. We were making a copy of the SkBitmap pixel > buffer anyways. So it had to write the pixels, allocate a pixel buffer > then copy everything. Now it just allocates then writes, no copy > involved. Thankfully gfx::ImageSkiaRep sets the pixel buffer as > immutable so we don't start overwriting the data with a new frame. > > https://codereview.chromium.org/2551733002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
