Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
187343007:
Adding fading effect for paint rectangles in HUD. (Closed)
|
Created:
6 years, 9 months ago by malch Modified:
6 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding fading effect for paint rectangles in HUD layer.
Review URL: https://codereview.chromium.org/187343007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256635
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fixes. #
Total comments: 4
Patch Set 3 : More fixes. #
Total comments: 4
Patch Set 4 : And more fixes. #Patch Set 5 : Same set. #Patch Set 6 : Uploading same set once again. #
Total comments: 8
Patch Set 7 : Fixed initial alpha bug. #Patch Set 8 : Changed int to unsigned. Removed const where it's needless. #
Total comments: 18
Patch Set 9 : Fixes. #
Total comments: 4
Patch Set 10 : Final fixes. #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:176: SkColor fadedGreen(const int initial_value, const int step) { static SkColor FadedGreen... https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:178: return SkColorSetARGB(value > 0 ? value : 0, 0, 195, 0); Can we rather fade them reducing alpha to 0? https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:642: std::vector<DebugRect> paint_rects; WAT? https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:649: layer_tree_impl()->SetNeedsRedraw(); Why do we do this unconditionally? https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:650: bool has_new_paint_rects = false; Why do you need this? https://codereview.chromium.org/187343007/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/187343007/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:451: bool LayerPropertyChanged() const { return layer_property_changed_; } Is this a related change?
https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:176: SkColor fadedGreen(const int initial_value, const int step) { On 2014/03/05 13:20:59, caseq wrote: > static SkColor FadedGreen... Done. https://codereview.chromium.org/187343007/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:178: return SkColorSetARGB(value > 0 ? value : 0, 0, 195, 0); On 2014/03/05 13:20:59, caseq wrote: > Can we rather fade them reducing alpha to 0? Done. https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:642: std::vector<DebugRect> paint_rects; On 2014/03/05 13:20:59, caseq wrote: > WAT? Done. https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:649: layer_tree_impl()->SetNeedsRedraw(); On 2014/03/05 13:20:59, caseq wrote: > Why do we do this unconditionally? Done. https://codereview.chromium.org/187343007/diff/1/cc/layers/heads_up_display_l... cc/layers/heads_up_display_layer_impl.cc:650: bool has_new_paint_rects = false; On 2014/03/05 13:20:59, caseq wrote: > Why do you need this? Done. https://codereview.chromium.org/187343007/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/187343007/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:451: bool LayerPropertyChanged() const { return layer_property_changed_; } On 2014/03/05 13:20:59, caseq wrote: > Is this a related change? Done.
https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:729: fade_step_ = 0; You can make things simpler by counting steps from kFadeSteps down to 0. This way, needs_update_ == (fade_step_ > 0). https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:730: paint_rects_ = new_paint_rects; paint_rects_.swap(new_paint_rects);
https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:729: fade_step_ = 0; On 2014/03/05 16:33:34, caseq wrote: > You can make things simpler by counting steps from kFadeSteps down to 0. This > way, needs_update_ == (fade_step_ > 0). Done. https://codereview.chromium.org/187343007/diff/10001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:730: paint_rects_ = new_paint_rects; On 2014/03/05 16:33:34, caseq wrote: > paint_rects_.swap(new_paint_rects); Done.
https://codereview.chromium.org/187343007/diff/30001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/30001/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:178: return SkColorSetARGB(value > 0 ? value : 0, 0, 195, 0); Can value ever be negative? https://codereview.chromium.org/187343007/diff/30001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/30001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:741: needs_update_ = fade_step_ > 0; Why do we need a variable to keep track of non-zero value of fade_step_?
https://codereview.chromium.org/187343007/diff/30001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/30001/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:178: return SkColorSetARGB(value > 0 ? value : 0, 0, 195, 0); On 2014/03/06 10:46:32, caseq wrote: > Can value ever be negative? Done. https://codereview.chromium.org/187343007/diff/30001/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/30001/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.cc:741: needs_update_ = fade_step_ > 0; On 2014/03/06 10:46:32, caseq wrote: > Why do we need a variable to keep track of non-zero value of fade_step_? Done.
https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:176: static SkColor FadedGreen(const int initial_value, const int step) { drop const and consider unsigned https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.h File cc/debug/debug_colors.h (right): https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.h#... cc/debug/debug_colors.h:73: static SkColor PaintRectBorderColor(const int step); no need for const here. also, unsigned perhaps? https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.h#... cc/debug/debug_colors.h:75: static SkColor PaintRectFillColor(const int step); ditto https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:116: const SkColor stroke_color, No need to specify const for anything passed by value. https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:119: const std::string label_text) const; This should by passed by reference.
https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/70002/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:176: static SkColor FadedGreen(const int initial_value, const int step) { On 2014/03/06 11:48:37, caseq wrote: > drop const and consider unsigned Done. https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:116: const SkColor stroke_color, On 2014/03/06 11:48:37, caseq wrote: > No need to specify const for anything passed by value. Done. https://codereview.chromium.org/187343007/diff/70002/cc/layers/heads_up_displ... cc/layers/heads_up_display_layer_impl.h:119: const std::string label_text) const; On 2014/03/06 11:48:37, caseq wrote: > This should by passed by reference. Done.
Generally makes sense to me, Dana & Nat, can you have a look please? https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.h File cc/debug/debug_colors.h (right): https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.h... cc/debug/debug_colors.h:72: static const int kFadeSteps = 50; why is this still signed?
https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.c... cc/debug/debug_colors.cc:177: unsigned value = step * initial_value / DebugColors::kFadeSteps; DCHECK that step is in [0, kFadeSteps]? https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.h File cc/debug/debug_colors.h (right): https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.h... cc/debug/debug_colors.h:72: static const int kFadeSteps = 50; On 2014/03/06 12:40:04, caseq wrote: > why is this still signed? Chromium style guide says to use signed ints and DCHECKs rather than unsigned things except for stuff like size_t for array sizes. https://codereview.chromium.org/187343007/diff/120001/cc/debug/debug_colors.h... cc/debug/debug_colors.h:73: static SkColor PaintRectBorderColor(unsigned step); so these should really be int, sorry. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (left): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:593: SkPaint paint = CreatePaint(); Can you create the SkPaint once and reuse it still? https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:656: new_paint_rects.push_back(debug_rects[i]); How about doing here: new_paint_rects.push_back(); fade_step_ = DebugColors::kFadeSteps; continue; Then below we can: if (new_paint_rects not empty) swap with paint_rects_. if (step > 0) paint all paint_rects_. And there's only one code path for paint rects to DrawDebugRects that way. Do you think that'd be any simpler? https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:720: (!active_tree_->hud_layer() || I'm not sure why this is needed. The HUD layer always causes there to be damage via https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/heads_up... This makes any frame we produce be damaged over the hud layer's area. However it does not cause compositor to trigger new frames. It seems like to do fading, you will need the HUD layer to cause SetNeedsRedraw() after each frame, unless you only want it to fade during new main thread frames? What's the intention?
https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:720: (!active_tree_->hud_layer() || On 2014/03/06 19:03:21, danakj wrote: > I'm not sure why this is needed. The HUD layer always causes there to be damage > via > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/heads_up... > > This makes any frame we produce be damaged over the hud layer's area. From what I can see, we explicitly exclude HUD from the damage calculation via check of layer against HUD (https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/damage_tr...), This effectively excludes any code that is conditional on layerIsAlwaysDamaged() returning true -- if we keep this check, we might as well get rid of layerIsAlwaysDamaged() :-) If we just remove the check, though, we'll stop bailing out of LayerTreeHostImpl::DrawLayers() on frame->has_no_damage as long as HUD is present and will keep updating HUD (and FPS rate) even when there's no actual damage to regular layers and no need to redraw the HUD. This may be alright since this would not happen unless SetNeedsRedraw() is called, but I'd like someone with the CC expertise to validate that :-) I guess we can also consider renaming layerIsAlwaysDamaged() to something else and only return true in case HUD has something to update. > However it does not cause compositor to trigger new frames. It seems like to do > fading, you will need the HUD layer to cause SetNeedsRedraw() after each frame, > unless you only want it to fade during new main thread frames? What's the > intention? The intent is to fade paint rects even if there are no main thread frames and even in the case where no impl-side frames would normally be scheduled (i.e. if the page draws something and goes idle, we need to fade its paint rects out to make evident we do not keep repainting).
On 2014/03/07 09:08:20, caseq wrote: > https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:720: (!active_tree_->hud_layer() || > On 2014/03/06 19:03:21, danakj wrote: > > I'm not sure why this is needed. The HUD layer always causes there to be > damage > > via > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/heads_up... > > > > This makes any frame we produce be damaged over the hud layer's area. > > From what I can see, we explicitly exclude HUD from the damage calculation via > check of layer against HUD > (https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/damage_tr...), It appears the HUD is broken on platforms with partial swap. Running poster circle with --use-gl=osmesa (to get partial swap) the FPS counter does not update anymore. > > This effectively excludes any code that is conditional on layerIsAlwaysDamaged() > returning true -- if we keep this check, we might as well get rid of > layerIsAlwaysDamaged() :-) > > If we just remove the check, though, we'll stop bailing out of > LayerTreeHostImpl::DrawLayers() on frame->has_no_damage as long as HUD is > present and will keep updating HUD (and FPS rate) even when there's no actual > damage to regular layers and no need to redraw the HUD. This may be alright > since this would not happen unless SetNeedsRedraw() is called, but I'd like > someone with the CC expertise to validate that :-) I guess we can also consider > renaming layerIsAlwaysDamaged() to something else and only return true in case > HUD has something to update. > > > > However it does not cause compositor to trigger new frames. It seems like to > do > > fading, you will need the HUD layer to cause SetNeedsRedraw() after each > frame, > > unless you only want it to fade during new main thread frames? What's the > > intention? > > The intent is to fade paint rects even if there are no main thread frames and > even in the case where no impl-side frames would normally be scheduled (i.e. if > the page draws something and goes idle, we need to fade its paint rects out to > make evident we do not keep repainting). Then we'll need to SetNeedsRedraw() after each frame when HUD wants to draw more.
> Then we'll need to SetNeedsRedraw() after each frame when HUD wants to draw > more. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:741: layer_tree_impl()->SetNeedsRedraw(); So it's here :-)
https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:741: layer_tree_impl()->SetNeedsRedraw(); On 2014/03/07 19:36:12, caseq wrote: > So it's here :-) Ah okay, I see. We need to fix the HUD redrawing stuff, the damage is all wrong atm. I'm working on a CL I'll send it to you.
ok, LG overall, a few more comments now that I have my head wrapped around HUD damage again. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:247: } Can we move the SetNeedsRedraw() call up here, and guard it on IsAnimatingHUDContents()? Then if we animate other things in the HUD we can make use of the same code. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.h:51: bool NeedsUpdate() const { return fade_step_ > 0; } Update is the wrong word for this, as that's something we use elsewhere in cc. How about IsAnimatingHUDContents()? https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:720: (!active_tree_->hud_layer() || OK this looks reasonable to me, now that I understand how HUD damage is (or isn't) working again. I'd prefer to split these hud checks off into a separate temporary bool (hud_wants_to_draw?) and add that bool to this if() statement instead.
https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (left): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:593: SkPaint paint = CreatePaint(); On 2014/03/06 19:03:21, danakj wrote: > Can you create the SkPaint once and reuse it still? Done. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:247: } On 2014/03/07 20:50:20, danakj wrote: > Can we move the SetNeedsRedraw() call up here, and guard it on > IsAnimatingHUDContents()? Then if we animate other things in the HUD we can make > use of the same code. Done. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:656: new_paint_rects.push_back(debug_rects[i]); On 2014/03/06 19:03:21, danakj wrote: > How about doing here: > > new_paint_rects.push_back(); > fade_step_ = DebugColors::kFadeSteps; > continue; > > Then below we can: > if (new_paint_rects not empty) swap with paint_rects_. > if (step > 0) paint all paint_rects_. > > And there's only one code path for paint rects to DrawDebugRects that way. Do > you think that'd be any simpler? Done. https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/187343007/diff/120001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.h:51: bool NeedsUpdate() const { return fade_step_ > 0; } On 2014/03/07 20:50:20, danakj wrote: > Update is the wrong word for this, as that's something we use elsewhere in cc. > How about IsAnimatingHUDContents()? Done. https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/187343007/diff/120001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:720: (!active_tree_->hud_layer() || On 2014/03/07 20:50:20, danakj wrote: > OK this looks reasonable to me, now that I understand how HUD damage is (or > isn't) working again. I'd prefer to split these hud checks off into a separate > temporary bool (hud_wants_to_draw?) and add that bool to this if() statement > instead. Done.
Thanks! LGTM https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:650: nit: drop extra whitespace https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:730: paint_rects_.swap(new_paint_rects); can you move the fade_step_ = DebugColors::kFadeSteps; down here? then we have all the fade_step_ code together.
https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:650: On 2014/03/11 17:28:32, danakj wrote: > nit: drop extra whitespace Done. https://codereview.chromium.org/187343007/diff/140001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:730: paint_rects_.swap(new_paint_rects); On 2014/03/11 17:28:32, danakj wrote: > can you move the fade_step_ = DebugColors::kFadeSteps; down here? then we have > all the fade_step_ code together. Done.
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malch@chromium.org/187343007/160001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malch@chromium.org/187343007/160001
Message was sent while issue was closed.
Change committed as 256635 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
