Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844483004:
Reset the state of the previous frame if overlay processing is skipped. (Closed)
|
Created:
3 years, 8 months ago by AndyWu Modified:
3 years, 7 months ago CC:
reveman, aelias_OOO_until_Jul13, ajuma, brianderson, cc-bugs_chromium.org, ccameron, chromium-reviews, danakj, enne (OOO), ericrk, piman, Sami, sunnyps, vmpstr, Ian Vollick, weiliangc Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset the state of the previous frame if hardware overlay processing is skipped.
If hardware overlay processing is skipped for a frame, there's no
way to be sure of the state of the previous frame. Reset
|previous_frame_underlay_rect_| for the next frame.
BUG=Internal b/37644269
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2844483004
Cr-Commit-Position: refs/heads/master@{#467754}
Committed: https://chromium.googlesource.com/chromium/src/+/3a24b86207d99af61810b6607b08432303646c3f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Reset the state of the previous frame if overlay processing is skipped. #Patch Set 3 : Reset the state of the previous frame if overlay processing is skipped. #
Total comments: 1
Patch Set 4 : Unit test #Patch Set 5 : Fix comment #
Total comments: 3
Patch Set 6 : Revise comments #
Total comments: 4
Patch Set 7 : Reset the state of the previous frame if hardware overlay processing is skipped. #
Messages
Total messages: 33 (15 generated)
Description was changed from ========== Reset the state of the previous frame if overlay processing is skipped. If overlay processing was skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 ========== to ========== Reset the state of the previous frame if overlay processing is skipped. If overlay processing was skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Reset the state of the previous frame if overlay processing is skipped. If overlay processing was skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Reset the state of the previous frame if overlay processing is skipped. If overlay processing was skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
tsunghung@chromium.org changed reviewers: + halliwell@chromium.org, liberato@chromium.org, sanfin@chromium.org, watk@chromium.org
tsunghung@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
thank you very much for putting this together! disclaimer on my comment is that i'm not an owner of this file :) thanks -fl https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor.cc File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor... cc/output/overlay_processor.cc:146: previous_frame_underlay_rect_ = gfx::Rect(); clearing it seems to be the default (and safe) action. to avoid having this happen the next time somebody adds an early-out, maybe do this instead near the top of this fn: gfx::Rect previous_frame_underlay_rect = previous_frame_underlay_rect_; previous_frame_underlay_rect_ = gfx::Rect(); then, pass the copy as an argument to UpdateDamageRect, rather than having it look at the original. it can update |previous...rect_| if it wants to. at least, i think that @198 is the only place where we update this to a non-empty value.
On 2017/04/26 18:34:08, AndyWu wrote: can you add a test? (in overlay_unittest.cc)
https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor.cc File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor... cc/output/overlay_processor.cc:146: previous_frame_underlay_rect_ = gfx::Rect(); On 2017/04/26 18:40:31, liberato wrote: > clearing it seems to be the default (and safe) action. to avoid having this > happen the next time somebody adds an early-out, maybe do this instead near the > top of this fn: > > gfx::Rect previous_frame_underlay_rect = previous_frame_underlay_rect_; > previous_frame_underlay_rect_ = gfx::Rect(); > > then, pass the copy as an argument to UpdateDamageRect, rather than having it > look at the original. it can update |previous...rect_| if it wants to. > > at least, i think that @198 is the only place where we update this to a > non-empty value. +1
On 2017/04/26 18:40:36, halliwell wrote: > On 2017/04/26 18:34:08, AndyWu wrote: > > can you add a test? (in overlay_unittest.cc) Will do.
https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor.cc File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/1/cc/output/overlay_processor... cc/output/overlay_processor.cc:146: previous_frame_underlay_rect_ = gfx::Rect(); On 2017/04/26 18:40:31, liberato wrote: > clearing it seems to be the default (and safe) action. to avoid having this > happen the next time somebody adds an early-out, maybe do this instead near the > top of this fn: > > gfx::Rect previous_frame_underlay_rect = previous_frame_underlay_rect_; > previous_frame_underlay_rect_ = gfx::Rect(); > > then, pass the copy as an argument to UpdateDamageRect, rather than having it > look at the original. it can update |previous...rect_| if it wants to. > > at least, i think that @198 is the only place where we update this to a > non-empty value. Done, thanks for your suggestion.
looks correct to me, thanks https://codereview.chromium.org/2844483004/diff/40001/cc/output/overlay_proce... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/40001/cc/output/overlay_proce... cc/output/overlay_processor.cc:131: // If overlay processing was skipped for a frame there's no way to be sure of s/was/is now. "was" made sense when we knew we were skipping overlays, but now we reset it just in case we skip overlays this frame.
non-owner lg*m % watk's comment. thanks -fl
tsunghung@chromium.org changed reviewers: + ajuma@chromium.org, brettw@chromium.org
https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... cc/output/overlay_processor.cc:135: previous_frame_underlay_rect_ = gfx::Rect(); Doesn't this just mean every frame previous_frame_underlay_rect_ = an empty rect, so we never subtract damage? The comment says "if overlay processing is skipped" but this is running when ProcessFor*Layers() is called so, I'm not understanding what it means to say.
tsunghung@chromium.org changed reviewers: - ajuma@chromium.org, brettw@chromium.org, danakj@chromium.org, enne@chromium.org
Not sure who is the owner of cc/output/overlay_processor.cc, please advise, thanks. https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... cc/output/overlay_processor.cc:135: previous_frame_underlay_rect_ = gfx::Rect(); On 2017/04/27 15:32:00, danakj wrote: > Doesn't this just mean every frame previous_frame_underlay_rect_ = an empty > rect, so we never subtract damage? > > The comment says "if overlay processing is skipped" but this is running when > ProcessFor*Layers() is called so, I'm not understanding what it means to say. The thing we care about is UpdateDamageRect() being called or not. I am not familiar with these terms. Maybe I should say "if hardware overlay processing is skipped"? Or maybe I should directly say "if UpdateDamageRect() is not called"?
Description was changed from ========== Reset the state of the previous frame if overlay processing is skipped. If overlay processing was skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Reset the state of the previous frame if hardware overlay processing is skipped. If hardware overlay processing is skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
reveman@chromium.org changed reviewers: + dcastagna@chromium.org, reveman@chromium.org
+dcastagna who have been making a lot of changes to overlay processing lately https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... cc/output/overlay_processor.cc:133: const gfx::Rect previous_frame_underlay_rect = previous_frame_underlay_rect_; nit: don't think we typically use "const" for local variables just because they are not going to be modified in cc/
danakj@chromium.org changed reviewers: + danakj@chromium.org
LGTM https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/80001/cc/output/overlay_proce... cc/output/overlay_processor.cc:135: previous_frame_underlay_rect_ = gfx::Rect(); On 2017/04/27 15:53:01, AndyWu wrote: > On 2017/04/27 15:32:00, danakj wrote: > > Doesn't this just mean every frame previous_frame_underlay_rect_ = an empty > > rect, so we never subtract damage? > > > > The comment says "if overlay processing is skipped" but this is running when > > ProcessFor*Layers() is called so, I'm not understanding what it means to say. > > The thing we care about is UpdateDamageRect() being called or not. I am not > familiar with these terms. Maybe I should say "if hardware overlay processing is > skipped"? Or maybe I should directly say "if UpdateDamageRect() is not called"? I see, UpdateDamageRect() is setting this to something not empty. OK Thanks. https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... cc/output/overlay_processor.cc:131: // Reset |previous_frame_underlay_rect_| in case |UpdateDamageRect| not being nit: I don't think |style| is used for functions, just variables. "in case UpdateDamageRect()" would work
https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... cc/output/overlay_processor.cc:133: const gfx::Rect previous_frame_underlay_rect = previous_frame_underlay_rect_; On 2017/04/27 16:15:09, reveman wrote: > nit: don't think we typically use "const" for local variables just because they > are not going to be modified in cc/ Well, I prefer to use "const" here. It says that the local variable is the read-only version of the member variable. If someone tries to modify the local variable instead of the member variable by mistake, the complier can catch that.
https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... File cc/output/overlay_processor.cc (right): https://codereview.chromium.org/2844483004/diff/100001/cc/output/overlay_proc... cc/output/overlay_processor.cc:131: // Reset |previous_frame_underlay_rect_| in case |UpdateDamageRect| not being On 2017/04/27 16:26:19, danakj wrote: > nit: I don't think |style| is used for functions, just variables. "in case > UpdateDamageRect()" would work Done, thanks for the correction.
The CQ bit was checked by tsunghung@chromium.org
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/2844483004/#ps120001 (title: "Reset the state of the previous frame if hardware overlay processing is skipped.")
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tsunghung@chromium.org
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": 1493315755340940,
"parent_rev": "5928f440f84065ab45f848dcd135e3c2f52ca5bb", "commit_rev":
"3a24b86207d99af61810b6607b08432303646c3f"}
Message was sent while issue was closed.
Description was changed from ========== Reset the state of the previous frame if hardware overlay processing is skipped. If hardware overlay processing is skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Reset the state of the previous frame if hardware overlay processing is skipped. If hardware overlay processing is skipped for a frame, there's no way to be sure of the state of the previous frame. Reset |previous_frame_underlay_rect_| for the next frame. BUG=Internal b/37644269 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2844483004 Cr-Commit-Position: refs/heads/master@{#467754} Committed: https://chromium.googlesource.com/chromium/src/+/3a24b86207d99af61810b6607b08... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3a24b86207d99af61810b6607b08... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
