Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892073002:
cc: Correct inverted directions in occlusion tracker background filter logic (Closed)
Descriptioncc: Correct inverted directions in occlusion tracker background filter logic
The filter's bounds for asymmetric filters (ex: background filter) are
relative to the target surface, which moves the pixels from outside of the
clip to the filtered surface. As a result, |affected_area| needs to expand.
Since we are concerned with the target surface, we need to swap the outsets
before applying them to the filtered surface bounds.
BUG=600821
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2892073002
Cr-Commit-Position: refs/heads/master@{#474291}
Committed: https://chromium.googlesource.com/chromium/src/+/92ee490845d92f5159028b4ed80b4e4fe5db545f
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 1
Patch Set 3 : correct wording #
Total comments: 1
Patch Set 4 : nits #Messages
Total messages: 32 (23 generated)
Description was changed from ========== fix asymmetric effect BUG=680636 ========== to ========== fix asymmetric effect BUG=680636 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== fix asymmetric effect BUG=680636 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Correct inverted directions in occlusion tracker background filter logic After both the logic of adding asymmetric filter and the affected area expansion are corrected in cl https://codereview.chromium.org/2119043002, we failed to shrink the affected occlusion rect by expanding the non-opaque pixels outside the rect: we shrink the left edge of the affected occlusion by the left outset instead of using the right outset. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Description was changed from ========== cc: Correct inverted directions in occlusion tracker background filter logic After both the logic of adding asymmetric filter and the affected area expansion are corrected in cl https://codereview.chromium.org/2119043002, we failed to shrink the affected occlusion rect by expanding the non-opaque pixels outside the rect: we shrink the left edge of the affected occlusion by the left outset instead of using the right outset. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Correct inverted directions in occlusion tracker background filter logic After both the logic of adding asymmetric filter and the affected area expansion are corrected in cl https://codereview.chromium.org/2119043002, we failed to shrink the affected occlusion rect by expanding the non-opaque pixels outside the rect: we shrink the left edge of the affected occlusion by the left outset instead of using the right outset and similar with other edges. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
yiyix@chromium.org changed reviewers: + danakj@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@danakj: could you please review this change? thank you very much
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Did you confirm that with this your patch will pass that test still? https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... File cc/trees/occlusion_tracker.cc (right): https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... cc/trees/occlusion_tracker.cc:226: // is not the affected area, but rather stuff slightly outside it. Thus the I think this comment doesn't explain what we have learnt very well. In my words I'd say that the filter's bounds for a background filter are relative to the target surface here rather than to the filtered surface, which means we have to invert the outsets to apply them to the filtered surface's bounds. wdyt?
On 2017/05/19 19:28:54, danakj wrote: > Did you confirm that with this your patch will pass that test still? > > https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... > File cc/trees/occlusion_tracker.cc (right): > > https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... > cc/trees/occlusion_tracker.cc:226: // is not the affected area, but rather stuff > slightly outside it. Thus the > I think this comment doesn't explain what we have learnt very well. In my words > I'd say that the filter's bounds for a background filter are relative to the > target surface here rather than to the filtered surface, which means we have to > invert the outsets to apply them to the filtered surface's bounds. wdyt? After i learned what it is, the comment becomes clearer to me..... but yea, you are right, after all the learning we need to do after reading this comment the first time, I think i should improve it....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/19 19:28:54, danakj wrote: > Did you confirm that with this your patch will pass that test still? > > https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... > File cc/trees/occlusion_tracker.cc (right): > > https://codereview.chromium.org/2892073002/diff/20001/cc/trees/occlusion_trac... > cc/trees/occlusion_tracker.cc:226: // is not the affected area, but rather stuff > slightly outside it. Thus the > I think this comment doesn't explain what we have learnt very well. In my words > I'd say that the filter's bounds for a background filter are relative to the > target surface here rather than to the filtered surface, which means we have to > invert the outsets to apply them to the filtered surface's bounds. wdyt? I have confirmed that my patch would pass that test case. Could you please review this patch? I have corrected the comments.
Description was changed from ========== cc: Correct inverted directions in occlusion tracker background filter logic After both the logic of adding asymmetric filter and the affected area expansion are corrected in cl https://codereview.chromium.org/2119043002, we failed to shrink the affected occlusion rect by expanding the non-opaque pixels outside the rect: we shrink the left edge of the affected occlusion by the left outset instead of using the right outset and similar with other edges. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Correct inverted directions in occlusion tracker background filter logic The filter's bounds for asymmetric filters (ex: background filter) are relative to the target surface, which moves the pixels from outside of the clip to the filtered surface. As a result, |affected_area| needs to expand. Since we are concerned with the target surface, we need to swap the outsets before applying them to the filtered surface bounds. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
LGTM https://codereview.chromium.org/2892073002/diff/40001/cc/trees/occlusion_trac... File cc/trees/occlusion_tracker.cc (right): https://codereview.chromium.org/2892073002/diff/40001/cc/trees/occlusion_trac... cc/trees/occlusion_tracker.cc:224: // The filter's bounds for asymmetric filters (ex: background filter) are asymmetric background filters (ex: drop shadow)
The CQ bit was checked by yiyix@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/2892073002/#ps60001 (title: "nits")
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@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": 60001, "attempt_start_ts": 1495638857392020,
"parent_rev": "f4397de900797d1909f8a895429483b05cdd4ebb", "commit_rev":
"92ee490845d92f5159028b4ed80b4e4fe5db545f"}
Message was sent while issue was closed.
Description was changed from ========== cc: Correct inverted directions in occlusion tracker background filter logic The filter's bounds for asymmetric filters (ex: background filter) are relative to the target surface, which moves the pixels from outside of the clip to the filtered surface. As a result, |affected_area| needs to expand. Since we are concerned with the target surface, we need to swap the outsets before applying them to the filtered surface bounds. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Correct inverted directions in occlusion tracker background filter logic The filter's bounds for asymmetric filters (ex: background filter) are relative to the target surface, which moves the pixels from outside of the clip to the filtered surface. As a result, |affected_area| needs to expand. Since we are concerned with the target surface, we need to swap the outsets before applying them to the filtered surface bounds. BUG=600821 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2892073002 Cr-Commit-Position: refs/heads/master@{#474291} Committed: https://chromium.googlesource.com/chromium/src/+/92ee490845d92f5159028b4ed80b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/92ee490845d92f5159028b4ed80b... |
