Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1251693002:
cc: Consider Surface active if frame received recently (Closed)
|
Created:
5 years, 5 months ago by brianderson Modified:
5 years, 4 months ago CC:
chromium-reviews, cc-bugs_chromium.org, jbauman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Consider Surface active if frame received recently
Current logic considers a child surface active if
it has submitted two frames in the last two vsyncs.
This hurts 30fps or 24fps video use cases, so change
the heuristic to consider a child surface active if it
has submitted a frame anytime within the last 3 vsyncs.
BUG=493751
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/b189b0f12544df1f65bd5a189b005e86908b1495
Cr-Commit-Position: refs/heads/master@{#340299}
Patch Set 1 #Patch Set 2 : test #Patch Set 3 : 20fps + single surface 20fps #
Total comments: 11
Patch Set 4 : address some comments #Patch Set 5 : active_child_surface_ids_ #
Total comments: 14
Patch Set 6 : address comments; needs rebase #Patch Set 7 : rebase on resize logging patch #
Depends on Patchset: Messages
Total messages: 18 (3 generated)
brianderson@chromium.org changed reviewers: + mithro@mithis.com, sunnyps@chromium.org
ptal
On 2015/07/21 22:44:53, brianderson wrote: > ptal Can you add a test for 24fps? I think the updated heuristic will fail for that case. At 24fps we see 1 frame for 2 vsyncs followed by 1 update for 3 vsyncs.
On 2015/07/21 22:51:47, sunnyps wrote: > On 2015/07/21 22:44:53, brianderson wrote: > > ptal > > Can you add a test for 24fps? I think the updated heuristic will fail for that > case. At 24fps we see 1 frame for 2 vsyncs followed by 1 update for 3 vsyncs. I can make it explicit that we want to wait for 220-24fps content and add a prev_prev set of damaged surfaces. My thinking initially was that most browser animations aren't 60fps and that this heuristic would be good enough - but I guess that wouldn't cover the case of mouse input related UI updates.
Addressed Sunny's comments. Had to restructure the code a little to handle a single surface operating at 20fps since we would go idle too early if needs_draw_ was false in between frames.
Left a few comments. Tests look good. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.cc (right): https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:100: expect_damage_from_root_surface_ = root_surface_damaged_; Set root_surface_damaged_ to false here instead of below. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:264: child_surface_ids_to_expect_damage_from_.size() != 0); Can you use all_active_child_surfaces_ready_to_draw_ here? https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:273: // will cause stay_active to go false even if we have nothing to draw. nit: "stay_active" instead of "stay active" https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:276: DCHECK(child_surface_ids_damaged_.empty()); Can probably add more DCHECKs here - like DCHECK(!entire_display_damaged_) etc. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:84: std::set<SurfaceId> child_surface_ids_damaged_prev_prev_; Can you use an array of sets here? Optional: I've found that instead of storing damage for the previous N frames, it's easier to store the expected damage for the next N frames. Then every time you have damage you add it to the next N frames. This works well in conjunction with a circular buffer so you can just move the pointer/index to the correct set (or damage) every frame and you don't need to calculate it every frame. This is what BufferQueue does - it tracks damage for ChromeOS ozone surfaceless implementation. If you do make this change can you also call it active_child_surface_ids_?
https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.cc (right): https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:100: expect_damage_from_root_surface_ = root_surface_damaged_; On 2015/07/23 03:02:40, sunnyps wrote: > Set root_surface_damaged_ to false here instead of below. This will work for the case where this method is called on line 126, however it would be incorrect for the call on 277, which is why I kept it out. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:264: child_surface_ids_to_expect_damage_from_.size() != 0); On 2015/07/23 03:02:40, sunnyps wrote: > Can you use all_active_child_surfaces_ready_to_draw_ here? I think that would work, however "child_surface_ids_to_expect_damage_from_.size() != 0" expresses the intent more directly and is more orthogonal to the other conditions since all_active_child_surfaces_ready_to_draw_ has some overlap in meaning with needs_draw_. I should probably change this to the following though: !child_surface_ids_to_expect_damage_from_.empty() https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:273: // will cause stay_active to go false even if we have nothing to draw. On 2015/07/23 03:02:40, sunnyps wrote: > nit: "stay_active" instead of "stay active" Looks like it's already stay_active, but I'll add |'s around it. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:276: DCHECK(child_surface_ids_damaged_.empty()); On 2015/07/23 03:02:40, sunnyps wrote: > Can probably add more DCHECKs here - like DCHECK(!entire_display_damaged_) etc. Sounds good. I will added a DCHECK(!root_surface_damaged_) too. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:84: std::set<SurfaceId> child_surface_ids_damaged_prev_prev_; On 2015/07/23 03:02:40, sunnyps wrote: > Can you use an array of sets here? Sure, although I'd like to keep child_surface_ids_damaged_ separate since it has special meaning for the current frame. If I stick with the current approach, I'll probably just put prev and prev_prev in an array together. If I take your approach below, I could also include child_surface_ids_to_expect_damage_from_ in the array which would be nice. > > Optional: I've found that instead of storing damage for the previous N frames, > it's easier to store the expected damage for the next N frames. Then every time > you have damage you add it to the next N frames. This works well in conjunction > with a circular buffer so you can just move the pointer/index to the correct set > (or damage) every frame and you don't need to calculate it every frame. > > This is what BufferQueue does - it tracks damage for ChromeOS ozone surfaceless > implementation. Nice observation! Now that the code does a union of the damaged IDs instead of an intersection, this approach would work and I think it would result in more readable code, especially with the variable renaming. I think there would be a small overall increase in runtime cost for a large number of damaged surfaces, but I'm not sure how much we care about that use case, especially since this heuristic will likely be gone if a time comes where we do care. All per-frame costs of the current vs. proposed approach are equal except for updating damage expectations. If we Let N be the number of surfaces damaged: Current cost: 2 * O(N): Two calls to STLSetUnion; Proposed cost: 2 * O(N log N): Two calls to set::insert per damaged surface. For the values of N we are working with right now, it probably doesn't matter though. Wdyt? > > If you do make this change can you also call it active_child_surface_ids_? That would be a good name.
On 2015/07/23 18:21:46, brianderson wrote: > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > File cc/surfaces/display_scheduler.cc (right): > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.cc:100: expect_damage_from_root_surface_ = > root_surface_damaged_; > On 2015/07/23 03:02:40, sunnyps wrote: > > Set root_surface_damaged_ to false here instead of below. > > This will work for the case where this method is called on line 126, however it > would be incorrect for the call on 277, which is why I kept it out. > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.cc:264: > child_surface_ids_to_expect_damage_from_.size() != 0); > On 2015/07/23 03:02:40, sunnyps wrote: > > Can you use all_active_child_surfaces_ready_to_draw_ here? > > I think that would work, however > "child_surface_ids_to_expect_damage_from_.size() != 0" expresses the intent more > directly and is more orthogonal to the other conditions since > all_active_child_surfaces_ready_to_draw_ has some overlap in meaning with > needs_draw_. > > I should probably change this to the following though: > !child_surface_ids_to_expect_damage_from_.empty() > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.cc:273: // will cause stay_active to go false even > if we have nothing to draw. > On 2015/07/23 03:02:40, sunnyps wrote: > > nit: "stay_active" instead of "stay active" > > Looks like it's already stay_active, but I'll add |'s around it. > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.cc:276: > DCHECK(child_surface_ids_damaged_.empty()); > On 2015/07/23 03:02:40, sunnyps wrote: > > Can probably add more DCHECKs here - like DCHECK(!entire_display_damaged_) > etc. > > Sounds good. I will added a DCHECK(!root_surface_damaged_) too. > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > File cc/surfaces/display_scheduler.h (right): > > https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... > cc/surfaces/display_scheduler.h:84: std::set<SurfaceId> > child_surface_ids_damaged_prev_prev_; > On 2015/07/23 03:02:40, sunnyps wrote: > > Can you use an array of sets here? > > Sure, although I'd like to keep child_surface_ids_damaged_ separate since it has > special meaning for the current frame. > > If I stick with the current approach, I'll probably just put prev and prev_prev > in an array together. If I take your approach below, I could also include > child_surface_ids_to_expect_damage_from_ in the array which would be nice. > > > > > Optional: I've found that instead of storing damage for the previous N frames, > > it's easier to store the expected damage for the next N frames. Then every > time > > you have damage you add it to the next N frames. This works well in > conjunction > > with a circular buffer so you can just move the pointer/index to the correct > set > > (or damage) every frame and you don't need to calculate it every frame. > > > > This is what BufferQueue does - it tracks damage for ChromeOS ozone > surfaceless > > implementation. > > Nice observation! Now that the code does a union of the damaged IDs instead of > an intersection, this approach would work and I think it would result in more > readable code, especially with the variable renaming. > > I think there would be a small overall increase in runtime cost for a large > number of damaged surfaces, but I'm not sure how much we care about that use > case, especially since this heuristic will likely be gone if a time comes where > we do care. > > All per-frame costs of the current vs. proposed approach are equal except for > updating damage expectations. If we Let N be the number of surfaces damaged: > Current cost: 2 * O(N): Two calls to STLSetUnion; > Proposed cost: 2 * O(N log N): Two calls to set::insert per damaged surface. > > For the values of N we are working with right now, it probably doesn't matter > though. Wdyt? Hmm. If we change the data structure from set to unordered_set, the proposed approach would end up being cheaper than the current approach even for large N. I'll go ahead with the proposed approach since it's easier to read, but keep std::set for now and switch to unordered_set later if we really want to. The switch to unordered_set will require several other changes too, and really I'm not sure it'll matter for the N's we're looking at. > > > > > If you do make this change can you also call it active_child_surface_ids_? > > That would be a good name.
Ptal. Using a circular buffer of active_child_surface_ids now. Also renamed expect_damage_from_root_surface_ to root_surface_active to be consistent.
LGTM with nits. https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/1251693002/diff/40001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:84: std::set<SurfaceId> child_surface_ids_damaged_prev_prev_; On 2015/07/23 18:21:46, brianderson wrote: > On 2015/07/23 03:02:40, sunnyps wrote: > > Can you use an array of sets here? > > Sure, although I'd like to keep child_surface_ids_damaged_ separate since it has > special meaning for the current frame. > > If I stick with the current approach, I'll probably just put prev and prev_prev > in an array together. If I take your approach below, I could also include > child_surface_ids_to_expect_damage_from_ in the array which would be nice. > > > > > Optional: I've found that instead of storing damage for the previous N frames, > > it's easier to store the expected damage for the next N frames. Then every > time > > you have damage you add it to the next N frames. This works well in > conjunction > > with a circular buffer so you can just move the pointer/index to the correct > set > > (or damage) every frame and you don't need to calculate it every frame. > > > > This is what BufferQueue does - it tracks damage for ChromeOS ozone > surfaceless > > implementation. > > Nice observation! Now that the code does a union of the damaged IDs instead of > an intersection, this approach would work and I think it would result in more > readable code, especially with the variable renaming. > > I think there would be a small overall increase in runtime cost for a large > number of damaged surfaces, but I'm not sure how much we care about that use > case, especially since this heuristic will likely be gone if a time comes where > we do care. > > All per-frame costs of the current vs. proposed approach are equal except for > updating damage expectations. If we Let N be the number of surfaces damaged: > Current cost: 2 * O(N): Two calls to STLSetUnion; > Proposed cost: 2 * O(N log N): Two calls to set::insert per damaged surface. > > For the values of N we are working with right now, it probably doesn't matter > though. Wdyt? It doesn't look like this could ever be a performance bottleneck - either std::set or base::hash_set is ok. I don't think we can use std::unordered_set yet - it's a part of C++ 11 standard library. > > > > > If you do make this change can you also call it active_child_surface_ids_? > > That would be a good name. > https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.cc (right): https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:83: UpdateFutureActiveChildSurfaceIDs(surface_id); nit: Instead of a separate method, just update the active surfaces sets here and add a comment. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:110: UpdateActiveSurfaces(); nit: No need for a separate method here. It's easier to read if all the updating happens here. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:184: if (all_active_child_surfaces_ready_to_draw_ && root_surface_active_) { nit: Rename to child_surfaces_ready_to_draw_ https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:248: nit: This could be better structured as: if (!stay_active) DoSomething; if (!needs_draw) DoSomething; if (pending_swaps >= max_pending_swaps || root_surface_resources_locked) return; DrawAndSwap(); https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:266: ClearActiveSurfaces(); nit: Probably doesn't need a separate method. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:291: std::set<SurfaceId>& DisplayScheduler::CurrentActiveChildSurfaceIDs() { nit: inline this https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:315: current_active_child_surface_ids_index_++; nit: index = (index + 1) % N https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:319: all_active_child_surfaces_ready_to_draw_ = nit: This could be an inline function. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:86: static const int kActiveChildSurfaceIdsSize = 3; nit: kNumFramesSurfaceIsActive?
https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.cc (right): https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:184: if (all_active_child_surfaces_ready_to_draw_ && root_surface_active_) { On 2015/07/23 21:10:38, sunnyps wrote: > nit: Rename to child_surfaces_ready_to_draw_ Done. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:248: On 2015/07/23 21:10:38, sunnyps wrote: > nit: This could be better structured as: > > if (!stay_active) > DoSomething; > > if (!needs_draw) > DoSomething; > > if (pending_swaps >= max_pending_swaps || root_surface_resources_locked) > return; > > DrawAndSwap(); Done. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:315: current_active_child_surface_ids_index_++; On 2015/07/23 21:10:38, sunnyps wrote: > nit: index = (index + 1) % N Done. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.cc:319: all_active_child_surfaces_ready_to_draw_ = On 2015/07/23 21:10:38, sunnyps wrote: > nit: This could be an inline function. My goal in writing all the helper methods was to put all the active_child_surface_ids_ logic in one place. This file isn't that big though, so I'll go ahead and inline the methods that are not used more than once. UpdateActiveSurfaces is the only one used more than once and will be the only one I keep. https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... File cc/surfaces/display_scheduler.h (right): https://codereview.chromium.org/1251693002/diff/80001/cc/surfaces/display_sch... cc/surfaces/display_scheduler.h:86: static const int kActiveChildSurfaceIdsSize = 3; On 2015/07/23 21:10:38, sunnyps wrote: > nit: kNumFramesSurfaceIsActive? Done.
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/1251693002/#ps120001 (title: "rebase on resize logging patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1251693002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1251693002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b189b0f12544df1f65bd5a189b005e86908b1495 Cr-Commit-Position: refs/heads/master@{#340299}
Message was sent while issue was closed.
Going to revert his while I debug locally why this made things worse instead of better.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1258273005/ by brianderson@chromium.org. The reason for reverting is: This caused regressions in 24/30fps cast tests in stead of improving them. See crbug.com/514075 for details. . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
