Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716193002:
android: Add draw completion for CompositorView (Closed)
Description[Reland] android: Add draw completion for CompositorView
Add a new callback from native CompositorImpl that corresponds to all
swap buffers. Then plumb it up to CompositorView and use it to implement
the draw completion imlementation for the SurfaceView.
Reland: fix bug causing crash when argument is evaluated out of order.
BUG=512636
Review-Url: https://codereview.chromium.org/2716193002
Cr-Original-Commit-Position: refs/heads/master@{#453965}
Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6cedde1626186
Review-Url: https://codereview.chromium.org/2716193002
Cr-Commit-Position: refs/heads/master@{#456004}
Committed: https://chromium.googlesource.com/chromium/src/+/e07c48a43cd71db4e575a370f787b36fee6b643d
Patch Set 1 #
Total comments: 2
Patch Set 2 : review, handle recursion #Patch Set 3 : null check #Patch Set 4 : rebase #Patch Set 5 : compile fix after rebase #Patch Set 6 : another fix, doing a bad job rebasing here.. #
Total comments: 4
Patch Set 7 : fix x86 #
Messages
Total messages: 47 (25 generated)
The CQ bit was checked by boliu@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.
boliu@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
ptal part 1 is https://codereview.chromium.org/2718903002/
https://codereview.chromium.org/2716193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2716193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:209: if (!mDrawingFinishedCallback.isEmpty()) nativeSetNeedsComposite(mNativeCompositorView); Is this if statement needed? I'd rather we universally call setNeedsComposite to make the sequence of events more predictable.
also rewrote the list a bit to handle potentially android somehow recursively calling RedrawNeeded in the callback. doubt it's a problem, but doesn't hurt to be careful https://codereview.chromium.org/2716193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2716193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:209: if (!mDrawingFinishedCallback.isEmpty()) nativeSetNeedsComposite(mNativeCompositorView); On 2017/02/28 00:20:02, aelias wrote: > Is this if statement needed? I'd rather we universally call setNeedsComposite > to make the sequence of events more predictable. Done.
lgtm
changwan, this needs your review for chrome compositor as well
boliu@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor for chrome/*compositor*
lgtm sorry it took a while!
The CQ bit was checked by boliu@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2716193002/#ps60001 (title: "rebase")
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
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_...)
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2716193002/#ps80001 (title: "compile fix after rebase")
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 boliu@chromium.org
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2716193002/#ps100001 (title: "another fix, doing a bad job rebasing here..")
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": 100001, "attempt_start_ts": 1488389831993280,
"parent_rev": "815482d233a6c1a9bab29f2e258f3bc75cdb7d77", "commit_rev":
"eddd0d4f361b16d62eae38b198d6cedde1626186"}
Message was sent while issue was closed.
Description was changed from ========== android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. BUG=512636 ========== to ========== android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. BUG=512636 Review-Url: https://codereview.chromium.org/2716193002 Cr-Commit-Position: refs/heads/master@{#453965} Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6...
Message was sent while issue was closed.
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339: for (Runnable r : runnables) { drive by nit but since this is an ArrayList, you should use the for (int i = 0; i <...) here because this will allocate an iterator (and you don't have an LinkedList were it would be expensive).
Message was sent while issue was closed.
https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339: for (Runnable r : runnables) { On 2017/03/03 21:00:16, Ted C wrote: > drive by nit but since this is an ArrayList, you should use the for (int i = 0; > i <...) here because this will allocate an iterator (and you don't have an > LinkedList were it would be expensive). Wait, is linked list less expensive?
Message was sent while issue was closed.
On 2017/03/03 21:04:05, boliu wrote: > https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java > (right): > > https://codereview.chromium.org/2716193002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339: > for (Runnable r : runnables) { > On 2017/03/03 21:00:16, Ted C wrote: > > drive by nit but since this is an ArrayList, you should use the for (int i = > 0; > > i <...) here because this will allocate an iterator (and you don't have an > > LinkedList were it would be expensive). > > Wait, is linked list less expensive? No, it is more that get(index) on a LinkedList is expensive and you can argue that the foreach is better in that case.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2732213008/ by hzl@google.com. The reason for reverting is: Revert the patch because of bug 697804..
Message was sent while issue was closed.
https://codereview.chromium.org/2716193002/diff/100001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2716193002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:692: context_provider, ahh, for anyone wondering why this is x86 crash only, this is the problem. std::move(context_provider) and context_provider in the same argument list. c++ does not guarantee argument evaluation order, and looks like on x86, it's backwards! std::move happens first, so by the time it gets to here, context_provider is null already I'll reland, and then merge after other m58 issues have been settled.
Message was sent while issue was closed.
Description was changed from ========== android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. BUG=512636 Review-Url: https://codereview.chromium.org/2716193002 Cr-Commit-Position: refs/heads/master@{#453965} Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6... ========== to ========== [Reland] android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. Reland: fix bug causing crash when argument is evaluated out of order. BUG=512636 Review-Url: https://codereview.chromium.org/2716193002 Cr-Commit-Position: refs/heads/master@{#453965} Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6... ==========
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2716193002/#ps120001 (title: "fix x86")
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": 120001, "attempt_start_ts": 1489122519048440,
"parent_rev": "59547521e04b9b5ec38d51588d7eab562ac0c630", "commit_rev":
"e07c48a43cd71db4e575a370f787b36fee6b643d"}
Message was sent while issue was closed.
Description was changed from ========== [Reland] android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. Reland: fix bug causing crash when argument is evaluated out of order. BUG=512636 Review-Url: https://codereview.chromium.org/2716193002 Cr-Commit-Position: refs/heads/master@{#453965} Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6... ========== to ========== [Reland] android: Add draw completion for CompositorView Add a new callback from native CompositorImpl that corresponds to all swap buffers. Then plumb it up to CompositorView and use it to implement the draw completion imlementation for the SurfaceView. Reland: fix bug causing crash when argument is evaluated out of order. BUG=512636 Review-Url: https://codereview.chromium.org/2716193002 Cr-Original-Commit-Position: refs/heads/master@{#453965} Committed: https://chromium.googlesource.com/chromium/src/+/eddd0d4f361b16d62eae38b198d6... Review-Url: https://codereview.chromium.org/2716193002 Cr-Commit-Position: refs/heads/master@{#456004} Committed: https://chromium.googlesource.com/chromium/src/+/e07c48a43cd71db4e575a370f787... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e07c48a43cd71db4e575a370f787...
Message was sent while issue was closed.
wnwen@chromium.org changed reviewers: + wnwen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2716193002/diff/100001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2716193002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:692: context_provider, On 2017/03/09 20:24:09, boliu wrote: > ahh, for anyone wondering why this is x86 crash only, this is the problem. > > std::move(context_provider) and context_provider in the same argument list. c++ > does not guarantee argument evaluation order, and looks like on x86, it's > backwards! > > std::move happens first, so by the time it gets to here, context_provider is > null already > > I'll reland, and then merge after other m58 issues have been settled. Thanks for getting to the root of this bug and the explanation, Bo! It makes sense now why it's just x86. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
