Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414533003:
Make ui::Compositor BeginFrame subscription robust (Closed)
|
Created:
5 years, 2 months ago by jdduke (slow) Modified:
5 years, 2 months ago CC:
chromium-reviews, Ian Vollick, sievers+watch_chromium.org, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, cc-bugs_chromium.org, mithro-old Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ui::Compositor BeginFrame subscription robust
The current logic for removing a ui::Compositor's BeginFrame observer
while dispatching the BeginFrame signal is broken. Make it robust by
using an ObserverList.
BUG=394562
Committed: https://crrev.com/7620c3c8de8b82db289524a767751f6228bc9293
Cr-Commit-Position: refs/heads/master@{#355649}
Patch Set 1 #Patch Set 2 : Test #
Total comments: 2
Patch Set 3 : Proper unsubscribe #
Total comments: 13
Patch Set 4 : Cleanup test, add comment, move unsubscription #Patch Set 5 : Cleanup comment #
Messages
Total messages: 21 (4 generated)
jdduke@chromium.org changed reviewers: + danakj@chromium.org
Dana could you take a look? Nasty bugs here, ran into it playing with vsync-aligned input dispatch in the browser.
oops forgot to review this.. looking..
danakj@chromium.org changed reviewers: + brianderson@chromium.org
The bug this points to is pretty general. Why do we want to allow removing a begin frame observer inside of begin frame signals? +brianderson
On 2015/10/21 20:18:40, danakj wrote: > The bug this points to is pretty general. Why do we want to allow removing a > begin frame observer inside of begin frame signals? > > +brianderson For vsync-aligned input, we might have a subscriber that is interested in flushing just one event, after which it no longer cares about the BeginFrame signal.
https://codereview.chromium.org/1414533003/diff/20001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1414533003/diff/20001/ui/compositor/composito... ui/compositor/compositor.cc:370: host_->SetChildrenNeedBeginFrames(false); What if might_have is true, but actually this was the last observer and you don't have any now. Where will we set false on the host?
https://codereview.chromium.org/1414533003/diff/20001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1414533003/diff/20001/ui/compositor/composito... ui/compositor/compositor.cc:370: host_->SetChildrenNeedBeginFrames(false); On 2015/10/21 20:23:51, danakj wrote: > What if might_have is true, but actually this was the last observer and you > don't have any now. Where will we set false on the host? Good call, we can check after iterating over the observers in SendBeginFramesToChildren.
Sorry you had to debug this Jared. Simon initially had this as an ObserverList and we didn't anticipate an Observer synchronously removing itself and changed it. +mitrho: Looks like another good reason that https://codereview.chromium.org/1337803002/ should support re-entrancy like you have it. Taking a look at this particular patch now.
lgtm I have a few nits, but they're not super important since this logic is behind a flag and will transition over to the DisplayScheduler soon where it should be easier to test. https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (left): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:197: DCHECK(begin_frame_observer_list_.empty()); Why is this no longer valid? https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:362: DCHECK(std::find(begin_frame_observer_list_.begin(), Any way to keep this check? https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:110: // Simulate to trigger new BeginFrame by using |args|. Thanks for making this test more realistic, even though it doesn't seem directly related to your fix. https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:139: TEST_F(CompositorTest, RemoveBeginFrameObserverWhileSendingBeginFrame) { It would be nice if these tests could verify the state of LTH::SetChildrenNeedBeginFrames too, but it looks like that would require pretty big changes to to the test framework.
https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (left): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:197: DCHECK(begin_frame_observer_list_.empty()); On 2015/10/21 23:12:59, brianderson wrote: > Why is this no longer valid? ObserverList does that for us (the second "true" argument to the template). https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:362: DCHECK(std::find(begin_frame_observer_list_.begin(), On 2015/10/21 23:12:59, brianderson wrote: > Any way to keep this check? ObserverList also does that for us. https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:139: TEST_F(CompositorTest, RemoveBeginFrameObserverWhileSendingBeginFrame) { On 2015/10/21 23:12:59, brianderson wrote: > It would be nice if these tests could verify the state of > LTH::SetChildrenNeedBeginFrames too, but it looks like that would require pretty > big changes to to the test framework. Yeah, I started looking into that kind of validation, but it gets a bit tricky to pull out some of the state machine bits.
https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:373: if (!begin_frame_observer_list_.might_have_observers()) { Do you need to do this here still, since it is done in SendBeginFramesToChildren? Can you leave a comment explaining here and on the one in SendBeginFramesToChildren to explain them? https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:167: EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args)).Times(0); put _ instead of expected_args. We don't want to have it called with any args. https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:175: EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args)).Times(0); dittos
https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/1414533003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:373: if (!begin_frame_observer_list_.might_have_observers()) { On 2015/10/22 18:35:59, danakj wrote: > Do you need to do this here still, since it is done in > SendBeginFramesToChildren? > > Can you leave a comment explaining here and on the one in > SendBeginFramesToChildren to explain them? I considered that, I suppose it depends on the semantics we want. If we don't mind a lingering unsubscribe for a single frame, that would be fine with me, though I'd still add a comment here detailing when the SendBeginFramesToChildren call will occur. I'll make the change and we can go from there.
Added some comments, moved the unsubscribe. https://chromiumcodereview.appspot.com/1414533003/diff/40001/ui/compositor/co... File ui/compositor/compositor_unittest.cc (right): https://chromiumcodereview.appspot.com/1414533003/diff/40001/ui/compositor/co... ui/compositor/compositor_unittest.cc:167: EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args)).Times(0); On 2015/10/22 18:36:00, danakj wrote: > put _ instead of expected_args. We don't want to have it called with any args. Done. https://chromiumcodereview.appspot.com/1414533003/diff/40001/ui/compositor/co... ui/compositor/compositor_unittest.cc:175: EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args)).Times(0); On 2015/10/22 18:35:59, danakj wrote: > dittos Done.
Thanks. LGTM. Maybe ping brian make sure he's okay with only the one unsubscribe.
On 2015/10/22 20:39:34, danakj wrote: > Thanks. LGTM. Maybe ping brian make sure he's okay with only the one > unsubscribe. He confirmed we should be OK.
The CQ bit was checked by jdduke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1414533003/#ps80001 (title: "Cleanup comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414533003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414533003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7620c3c8de8b82db289524a767751f6228bc9293 Cr-Commit-Position: refs/heads/master@{#355649} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
