Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
958843004:
Fix for 424682: Draw correct repaint regions instead of a conservative bounding box (Closed)
DescriptionDraw correct repaint regions instead of a conservative bounding box
Previously, the update_rect() of the LayerImpl was used for the repaint
regions in the DebugRectHistory; now the base LayerImpl class defines a
virtual method giving back an invalidation Region that is overwritten by
PictureLayerImpl to use the already existent invalidations_ Region.
Regions give a more tighter bound on the invalidations than the
single conservative rect that was used before on a per-layer basis.
BUG=424682
Committed: https://crrev.com/350219ee4e5d02ca86f2f898172411f7da3d6902
Cr-Commit-Position: refs/heads/master@{#319094}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Refactoring according to suggestions by danakj #Patch Set 3 : reverted twin-layer check as it does not solve it #
Total comments: 2
Patch Set 4 : Updated PictureLayerImpl::GetInvalidationRegion to calculate intersection of invalidation_ and upda… #
Total comments: 7
Patch Set 5 : ran git cl format and improved comment #Patch Set 6 : added name to AUTHORS #
Messages
Total messages: 45 (15 generated)
daplatz@googlemail.com changed reviewers: + danakj@chromium.org
hi danakj, attached my fix for 424682. cheers, daniel
I updated your CL description a little (fixed class names and some typos). A few comments but it looks good. https://codereview.chromium.org/958843004/diff/1/cc/debug/debug_rect_history.cc File cc/debug/debug_rect_history.cc (left): https://codereview.chromium.org/958843004/diff/1/cc/debug/debug_rect_history.... cc/debug/debug_rect_history.cc:71: if (!layer->update_rect().IsEmpty() && layer->DrawsContent()) { Can this use the region too? https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:600: // layers that can provide it (e.g. picture_layer_imp.h) drop the reference to picture_layer_imp.h here I think. https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:601: // Used by debug_rect_history.h to track repaint regions. Listing callers of functions is sure to go wrong, we can find callsites from code search, so I think just remove this too. https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:602: virtual Region GetInvalidationRegion() { return Region(update_rect_); }; virtual function bodies must be in the .cc file (this won't compile with clang) https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:1228: Region PictureLayerImpl::GetInvalidationRegion() { functions in the same order as they appear in the .h file https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl.h File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.h:108: Region GetInvalidationRegion() override; move this up with the LayerImpl overrides
made the changed according to your suggestions. https://codereview.chromium.org/958843004/diff/1/cc/debug/debug_rect_history.cc File cc/debug/debug_rect_history.cc (left): https://codereview.chromium.org/958843004/diff/1/cc/debug/debug_rect_history.... cc/debug/debug_rect_history.cc:71: if (!layer->update_rect().IsEmpty() && layer->DrawsContent()) { On 2015/02/26 23:53:05, danakj wrote: > Can this use the region too? I made the change; it makes sense; but for this i also had to slightly update the PictureLayerImpl::GetInvalidationRegion method as it seems when there is a twinlayer, the invalidations are only ever reset on that twin; not doing it made the repaint rect on the hud not fade out but stay indefintively. https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:600: // layers that can provide it (e.g. picture_layer_imp.h) On 2015/02/26 23:53:05, danakj wrote: > drop the reference to picture_layer_imp.h here I think. Acknowledged. https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:601: // Used by debug_rect_history.h to track repaint regions. On 2015/02/26 23:53:05, danakj wrote: > Listing callers of functions is sure to go wrong, we can find callsites from > code search, so I think just remove this too. Acknowledged. https://codereview.chromium.org/958843004/diff/1/cc/layers/layer_impl.h#newco... cc/layers/layer_impl.h:602: virtual Region GetInvalidationRegion() { return Region(update_rect_); }; On 2015/02/26 23:53:05, danakj wrote: > virtual function bodies must be in the .cc file (this won't compile with clang) Acknowledged. https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.cc:1228: Region PictureLayerImpl::GetInvalidationRegion() { On 2015/02/26 23:53:05, danakj wrote: > functions in the same order as they appear in the .h file Acknowledged. https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl.h File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/958843004/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl.h:108: Region GetInvalidationRegion() override; On 2015/02/26 23:53:05, danakj wrote: > move this up with the LayerImpl overrides Acknowledged.
please not that in patch-set 2 i forgot to check in the change in debug_rect_history from using update_rect() to GetInValidationRegion in the if block. that's why i falsely though the twinlayer check fixed it. Anyway, i rolled back the twinlayer check and left the if block in debug_rect_history with the update_rect().IsEmpty() check. Any guess why invalidations_ dont seem to be getting reset as the update_rect() does?
danakj@chromium.org changed reviewers: + enne@chromium.org
On 2015/02/27 12:06:55, daplatz wrote: > please not that in patch-set 2 i forgot to check in the change in > debug_rect_history from using update_rect() to GetInValidationRegion in the if > block. that's why i falsely though the twinlayer check fixed it. > Anyway, i rolled back the twinlayer check and left the if block in > debug_rect_history with the update_rect().IsEmpty() check. > > Any guess why invalidations_ dont seem to be getting reset as the update_rect() > does? LayerImpl::ResetAllChangeTrackingForSubtree resets the update_rect, but not the invalidation region. I guess we have two choices here: - make ResetAllChangeTrackingForSubtree virtual and have it clear the invalidation in PictureLayerImpl - move the invalidation_ rect up to LayerImpl and replace the update_rect_ with it entirely. I'm leaning toward the latter. +enne for an opinion/double checking.
I have vague perf worries about proliferating region everywhere. What about intersecting the invalidation region with the update rect? ;)
On 2015/02/27 19:25:27, enne wrote: > I have vague perf worries about proliferating region everywhere. > > What about intersecting the invalidation region with the update rect? ;) Oh that's a good idea. +1
Just to confirm i got this right: Do you mean that the intersection should be calculated and used in the debug_rect_history? On Feb 27, 2015 8:30 PM, <danakj@chromium.org> wrote: > On 2015/02/27 19:25:27, enne wrote: > >> I have vague perf worries about proliferating region everywhere. >> > > What about intersecting the invalidation region with the update rect? ;) >> > > Oh that's a good idea. +1 > > https://codereview.chromium.org/958843004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:609: return invalidation_; you can intersect it here
https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:609: return invalidation_; On 2015/02/27 21:10:48, danakj wrote: > you can intersect it here With a comment explaining why you're doing so based on what we've learnt in this CL :)
On 2015/02/27 21:25:35, danakj wrote: > https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/958843004/diff/40001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl.cc:609: return invalidation_; > On 2015/02/27 21:10:48, danakj wrote: > > you can intersect it here > > With a comment explaining why you're doing so based on what we've learnt in this > CL :) Intersection works great. Comment: Done. let me know if it is fine; hope it's not to excessive... :-)
LGTM w/ a few comments https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... File cc/debug/debug_rect_history.cc (right): https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... cc/debug/debug_rect_history.cc:79: it.has_rect(); this doesn't look like how git cl format would do things, did you run it? (PRESUBMIT warns you when you upload if you haven't) https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... cc/debug/debug_rect_history.cc:81: nit: remove this whitespace https://codereview.chromium.org/958843004/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:609: // PictureLayerImpl::invalidation_ is not cleared in the same was as I think I'd reword this a bit.. "truth" is subjective :) Maybe this: |invalidation_| gives the invalidation contained in the source frame, but is not cleared after drawing from the layer. However, update_rect() is cleared once the invalidation is drawn, which is useful for debugging visualizations. This method intersects the two to give a more exact representation of what was invalidated that is cleared after drawing. https://codereview.chromium.org/958843004/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:617: Region current_invalidation(invalidation_); you can do this whole thing as return IntersectRegions(invalidation_, update_rect());
as always: thanks for helpful comments. i did not know about git cl format yet. https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... File cc/debug/debug_rect_history.cc (right): https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... cc/debug/debug_rect_history.cc:79: it.has_rect(); On 2015/03/02 20:10:53, danakj wrote: > this doesn't look like how git cl format would do things, did you run it? > (PRESUBMIT warns you when you upload if you haven't) no i did not know about it. sweet. https://codereview.chromium.org/958843004/diff/60001/cc/debug/debug_rect_hist... cc/debug/debug_rect_history.cc:81: On 2015/03/02 20:10:53, danakj wrote: > nit: remove this whitespace Acknowledged. https://codereview.chromium.org/958843004/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/958843004/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.cc:617: Region current_invalidation(invalidation_); On 2015/03/02 20:10:53, danakj wrote: > you can do this whole thing as > > return IntersectRegions(invalidation_, update_rect()); Acknowledged.
The CQ bit was checked by daplatz@googlemail.com
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/958843004/#ps80001 (title: "ran git cl format and improved comment")
The CQ bit was unchecked by daplatz@googlemail.com
The CQ bit was checked by daplatz@googlemail.com
The CQ bit was unchecked by daplatz@googlemail.com
The CQ bit was checked by daplatz@googlemail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958843004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/03/02 21:40:42, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) what exactly has gone wrong? tried to understand from the logs but nothing stuck out for me...
From http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...: ** Presubmit Warnings ** daplatz@googlemail.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Once you've done that, let me know.
On 2015/03/02 22:48:06, enne wrote: > From > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...: > > ** Presubmit Warnings ** > mailto:daplatz@googlemail.com is not in AUTHORS file. If you are a new contributor, > please visit > http://www.chromium.org/developers/contributing-code and read the "Legal" > section > If you are a chromite, verify the contributor signed the CLA. > > Once you've done that, let me know. done
On 2015/03/03 19:51:53, daplatz wrote: > On 2015/03/02 22:48:06, enne wrote: > > From > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...: > > > > ** Presubmit Warnings ** > > mailto:daplatz@googlemail.com is not in AUTHORS file. If you are a new > contributor, > > please visit > > http://www.chromium.org/developers/contributing-code and read the "Legal" > > section > > If you are a chromite, verify the contributor signed the CLA. > > > > Once you've done that, let me know. > > done Did you sign the CLA? I don't see your email address as signed: https://cla.developers.google.com/
Ok, now i did. Sorry, should read more careful... On Mar 3, 2015 8:55 PM, <danakj@chromium.org> wrote: > On 2015/03/03 19:51:53, daplatz wrote: > >> On 2015/03/02 22:48:06, enne wrote: >> > From >> > >> > > http://build.chromium.org/p/tryserver.chromium.linux/ > builders/chromium_presubmit/builds/46745/steps/presubmit/logs/stdio: > >> > >> > ** Presubmit Warnings ** >> > mailto:daplatz@googlemail.com is not in AUTHORS file. If you are a new >> contributor, >> > please visit >> > http://www.chromium.org/developers/contributing-code and read the >> "Legal" >> > section >> > If you are a chromite, verify the contributor signed the CLA. >> > >> > Once you've done that, let me know. >> > > done >> > > Did you sign the CLA? I don't see your email address as signed: > https://cla.developers.google.com/ > > https://codereview.chromium.org/958843004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by enne@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/958843004/#ps100001 (title: "added name to AUTHORS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958843004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
On 2015/03/03 20:57:18, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) >> App does not exist: /b/build/slave/ios_dbg_simulator_ninja/build/src/out/Debug-iphonesimulator/sql_unittests.app." This is no problem really related to this issue, or is it?
On 2015/03/04 07:54:43, daplatz wrote: > On 2015/03/03 20:57:18, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > > > >> App does not exist: > /b/build/slave/ios_dbg_simulator_ninja/build/src/out/Debug-iphonesimulator/sql_unittests.app." > This is no problem really related to this issue, or is it? No that's not related.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958843004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958843004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/350219ee4e5d02ca86f2f898172411f7da3d6902 Cr-Commit-Position: refs/heads/master@{#319094} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
