Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031383002:
Allow animation in Android viewer (Closed)
DescriptionAllow animation in Android viewer
To do that, we let ALooper_pollAll return immediately rather than wait indefinitely.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002
Committed: https://skia.googlesource.com/skia/+/38d1adf1f7f482326d7047d52d56e65e9196482a
Patch Set 1 #
Total comments: 9
Patch Set 2 : Revision #
Total comments: 2
Messages
Total messages: 13 (5 generated)
Description was changed from ========== Allow animation in Android viewer To do that, we let ALooper_pollAll to return immediately rather than to wait indefinitely. BUG=skia: ========== to ========== Allow animation in Android viewer To do that, we let ALooper_pollAll to return immediately rather than to wait indefinitely. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002 ==========
Description was changed from ========== Allow animation in Android viewer To do that, we let ALooper_pollAll to return immediately rather than to wait indefinitely. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002 ========== to ========== Allow animation in Android viewer To do that, we let ALooper_pollAll return immediately rather than wait indefinitely. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002 ==========
liyuqian@google.com changed reviewers: + jvanverth@google.com, scroggo@google.com
https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... File tools/viewer/sk_app/android/surface_glue_android.cpp (right): https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:25: #include "Timer.h" I think you just use SkTIme, which is defined in SkTime.h - you do not appear to use anything from Timer.h. Why not include SkTime.h directly? https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:164: static double now_ms() { return SkTime::GetNSecs() * 1e-6; } Why not call SkTime::GetMSecs()? And while we're at it, why not use SkTime::GetMSecs() at the single call site? https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:184: int ident; It's easier to follow (and less error-prone) if you declare variables where they're used. e.g. const int ident = ALooper_pollAll(0, nullptr, &events, (void**)&source); https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:186: struct android_poll_source* source; You don't use either of these variables. Will ALooper_pollAll let you pass nullptr instead?
Thank you for the helpful comments! I've revised the code accordingly. https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... File tools/viewer/sk_app/android/surface_glue_android.cpp (right): https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:25: #include "Timer.h" On 2016/06/03 19:11:06, scroggo wrote: > I think you just use SkTIme, which is defined in SkTime.h - you do not appear to > use anything from Timer.h. Why not include SkTime.h directly? Done. https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:164: static double now_ms() { return SkTime::GetNSecs() * 1e-6; } On 2016/06/03 19:11:06, scroggo wrote: > Why not call SkTime::GetMSecs()? > > And while we're at it, why not use SkTime::GetMSecs() at the single call site? Done. https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:184: int ident; On 2016/06/03 19:11:06, scroggo wrote: > It's easier to follow (and less error-prone) if you declare variables where > they're used. e.g. > > const int ident = ALooper_pollAll(0, nullptr, &events, (void**)&source); Done. https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:186: struct android_poll_source* source; On 2016/06/03 19:11:06, scroggo wrote: > You don't use either of these variables. Will ALooper_pollAll let you pass > nullptr instead? Unfortunately, the Android developer document doesn't say that null is allowed. But I checked their source code and they do allow null in the current implementation. Is it safe for me to proceed with null?
lgtm https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... File tools/viewer/sk_app/android/surface_glue_android.cpp (right): https://codereview.chromium.org/2031383002/diff/1/tools/viewer/sk_app/android... tools/viewer/sk_app/android/surface_glue_android.cpp:186: struct android_poll_source* source; On 2016/06/06 13:38:27, liyuqian wrote: > On 2016/06/03 19:11:06, scroggo wrote: > > You don't use either of these variables. Will ALooper_pollAll let you pass > > nullptr instead? > > Unfortunately, the Android developer document doesn't say that null is allowed. > But I checked their source code and they do allow null in the current > implementation. Is it safe for me to proceed with null? I suppose their implementation could vary based on the version of Android? I would recommend going on the safe side and passing pointers, just in case. But I would give them names that specify that they are unused: // We do not need these values, but a ALooper_pollAll does not guarantee that they can be null: int events_unused; struct android_poll_source* source_unused;
https://codereview.chromium.org/2031383002/diff/20001/tools/viewer/sk_app/and... File tools/viewer/sk_app/android/surface_glue_android.cpp (right): https://codereview.chromium.org/2031383002/diff/20001/tools/viewer/sk_app/and... tools/viewer/sk_app/android/surface_glue_android.cpp:184: const int ident = ALooper_pollAll(0, nullptr, nullptr, nullptr); I'm surprised a timeout of 0 doesn't cause a busy loop (i.e., eat up 100% of CPU). On the other hand, a timeout of 1 seems like it would affect our timing.
The CQ bit was checked by liyuqian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031383002/20001
The CPU usage is checked and it does look good. https://codereview.chromium.org/2031383002/diff/20001/tools/viewer/sk_app/and... File tools/viewer/sk_app/android/surface_glue_android.cpp (right): https://codereview.chromium.org/2031383002/diff/20001/tools/viewer/sk_app/and... tools/viewer/sk_app/android/surface_glue_android.cpp:184: const int ident = ALooper_pollAll(0, nullptr, nullptr, nullptr); On 2016/06/06 14:18:11, jvanverth1 wrote: > I'm surprised a timeout of 0 doesn't cause a busy loop (i.e., eat up 100% of > CPU). On the other hand, a timeout of 1 seems like it would affect our timing. I checked the CPU usage on Android and it indeed didn't eat up 100% of CPU.
Message was sent while issue was closed.
Description was changed from ========== Allow animation in Android viewer To do that, we let ALooper_pollAll return immediately rather than wait indefinitely. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002 ========== to ========== Allow animation in Android viewer To do that, we let ALooper_pollAll return immediately rather than wait indefinitely. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2031383002 Committed: https://skia.googlesource.com/skia/+/38d1adf1f7f482326d7047d52d56e65e9196482a ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/38d1adf1f7f482326d7047d52d56e65e9196482a |
