Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(197)

Issue 2848223003: Enforce constant size and device scale factor for surfaces (Closed)

Created:
3 years, 7 months ago by Saman Sami
Modified:
3 years, 7 months ago
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Enforce constant size and device scale factor for surfaces Surfaces are expected to have constant size and device scale factor but we don't enforce that. This CL requires a valid SurfaceInfo to be provided at the time of constructing a surface, and all frames that don't match the SurfaceInfo will be skipped. TBR=sadrul@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2848223003 Cr-Commit-Position: refs/heads/master@{#474050} Committed: https://chromium.googlesource.com/chromium/src/+/5ba3188fa05666ae8d6031706c0963532f9d1457

Patch Set 1 #

Total comments: 6

Patch Set 2 : c #

Total comments: 4

Patch Set 3 : c #

Patch Set 4 : c #

Patch Set 5 : c #

Total comments: 6

Patch Set 6 : c #

Total comments: 18

Patch Set 7 : c #

Patch Set 8 : c #

Patch Set 9 : Notify CFSSClient when a frame is rejected #

Total comments: 2

Patch Set 10 : Remove DidReject #

Patch Set 11 : Fixed Android #

Total comments: 13

Patch Set 12 : Fixed dchecks #

Total comments: 2

Patch Set 13 : Fix Android #

Patch Set 14 : Rebase #

Patch Set 15 : Address comments #

Total comments: 4

Patch Set 16 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+245 lines, -158 lines) Patch
M android_webview/browser/hardware_renderer.h View 1 2 8 9 1 chunk +2 lines, -1 line 0 comments Download
M android_webview/browser/hardware_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +14 lines, -8 lines 0 comments Download
M android_webview/browser/surfaces_instance.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -2 lines 0 comments Download
M android_webview/browser/test/rendering_test.cc View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M cc/surfaces/compositor_frame_sink_support.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +3 lines, -4 lines 0 comments Download
M cc/surfaces/compositor_frame_sink_support.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +28 lines, -7 lines 0 comments Download
M cc/surfaces/compositor_frame_sink_support_unittest.cc View 1 2 8 9 3 chunks +0 lines, -6 lines 0 comments Download
M cc/surfaces/direct_compositor_frame_sink.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -1 line 0 comments Download
M cc/surfaces/direct_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +8 lines, -6 lines 0 comments Download
M cc/surfaces/direct_compositor_frame_sink_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -1 line 0 comments Download
M cc/surfaces/display_unittest.cc View 1 2 10 chunks +16 lines, -5 lines 0 comments Download
M cc/surfaces/surface.h View 1 2 3 4 5 6 7 3 chunks +6 lines, -5 lines 0 comments Download
M cc/surfaces/surface.cc View 1 2 3 4 5 6 7 5 chunks +18 lines, -6 lines 0 comments Download
M cc/surfaces/surface_aggregator_unittest.cc View 1 2 6 chunks +3 lines, -12 lines 0 comments Download
M cc/surfaces/surface_manager.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M cc/surfaces/surface_manager.cc View 1 2 chunks +9 lines, -9 lines 0 comments Download
M cc/surfaces/surface_unittest.cc View 1 2 2 chunks +4 lines, -3 lines 0 comments Download
M cc/test/compositor_frame_helpers.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M cc/test/test_compositor_frame_sink.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M cc/test/test_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +12 lines, -9 lines 0 comments Download
M components/viz/frame_sinks/gpu_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -1 line 0 comments Download
M components/viz/frame_sinks/gpu_root_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -1 line 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/delegated_frame_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 35 chunks +45 lines, -40 lines 0 comments Download
M content/renderer/android/synchronous_compositor_frame_sink.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/android/synchronous_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +25 lines, -12 lines 0 comments Download
M ui/android/delegated_frame_host_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -1 line 0 comments Download
M ui/aura/local/compositor_frame_sink_local.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -1 line 0 comments Download
M ui/aura/local/compositor_frame_sink_local.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +11 lines, -10 lines 0 comments Download

Messages

Total messages: 216 (183 generated)
Fady Samuel
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newcode60 cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { Return resources and call the ...
3 years, 7 months ago (2017-05-01 20:36:54 UTC) #3
Fady Samuel
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newcode56 cc/surfaces/surface.cc:56: if (!frame.render_pass_list.empty()) { Not necessary any more right? https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newcode60 ...
3 years, 7 months ago (2017-05-07 18:16:52 UTC) #4
Fady Samuel
https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface.cc#newcode68 cc/surfaces/surface.cc:68: callback.Run(); Return resources too? https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_info.h File cc/surfaces/surface_info.h (right): https://codereview.chromium.org/2848223003/diff/60001/cc/surfaces/surface_info.h#newcode32 ...
3 years, 7 months ago (2017-05-08 20:21:20 UTC) #15
Saman Sami
https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newcode56 cc/surfaces/surface.cc:56: if (!frame.render_pass_list.empty()) { On 2017/05/07 18:16:51, Fady Samuel wrote: ...
3 years, 7 months ago (2017-05-08 20:40:33 UTC) #16
Saman Sami
PTAL https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/1/cc/surfaces/surface.cc#newcode60 cc/surfaces/surface.cc:60: device_scale_factor != surface_info_.device_scale_factor()) { On 2017/05/08 20:40:32, Saman ...
3 years, 7 months ago (2017-05-16 18:55:46 UTC) #94
Fady Samuel
https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor_frame_sink_support.cc#newcode135 cc/surfaces/compositor_frame_sink_support.cc:135: current_surface_->RunDrawCallback(); Could you please add a comment why we ...
3 years, 7 months ago (2017-05-17 00:40:50 UTC) #97
Saman Sami
https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/500001/cc/surfaces/compositor_frame_sink_support.cc#newcode135 cc/surfaces/compositor_frame_sink_support.cc:135: current_surface_->RunDrawCallback(); On 2017/05/17 00:40:50, Fady Samuel wrote: > Could ...
3 years, 7 months ago (2017-05-17 00:55:17 UTC) #98
Saman Sami
danakj, please review all files.
3 years, 7 months ago (2017-05-17 14:44:37 UTC) #102
Fady Samuel
Non-OWNER LGTM for the approach
3 years, 7 months ago (2017-05-17 16:16:27 UTC) #108
danakj
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; How will this happen in practice? ...
3 years, 7 months ago (2017-05-17 20:49:36 UTC) #111
Saman Sami
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 20:49:36, danakj wrote: > ...
3 years, 7 months ago (2017-05-17 20:52:09 UTC) #112
danakj
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 20:52:08, Saman Sami wrote: ...
3 years, 7 months ago (2017-05-17 21:03:28 UTC) #113
Saman Sami
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:03:28, danakj wrote: > ...
3 years, 7 months ago (2017-05-17 21:22:50 UTC) #114
danakj
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:22:50, Saman Sami wrote: ...
3 years, 7 months ago (2017-05-17 21:25:28 UTC) #115
Saman Sami
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:25:28, danakj wrote: > ...
3 years, 7 months ago (2017-05-17 21:39:26 UTC) #116
Fady Samuel
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc File cc/surfaces/surface.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/surface.cc#newcode69 cc/surfaces/surface.cc:69: reject_frame = true; On 2017/05/17 21:39:26, Saman Sami wrote: ...
3 years, 7 months ago (2017-05-18 11:45:26 UTC) #117
danakj
LGTM https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor_frame_sink_support.cc#newcode133 cc/surfaces/compositor_frame_sink_support.cc:133: DLOG(ERROR) << "Cannot create a surface with invalid ...
3 years, 7 months ago (2017-05-18 15:15:54 UTC) #118
Saman Sami
https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/surfaces/compositor_frame_sink_support.cc#newcode133 cc/surfaces/compositor_frame_sink_support.cc:133: DLOG(ERROR) << "Cannot create a surface with invalid SurfaceInfo."; ...
3 years, 7 months ago (2017-05-18 20:44:37 UTC) #128
Fady Samuel
https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor_frame_sink_support.cc#oldcode139 cc/surfaces/compositor_frame_sink_support.cc:139: std::move(frame), Shouldn't we be returning resources here if this ...
3 years, 7 months ago (2017-05-18 20:51:32 UTC) #129
Saman Sami
https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (left): https://codereview.chromium.org/2848223003/diff/580001/cc/surfaces/compositor_frame_sink_support.cc#oldcode139 cc/surfaces/compositor_frame_sink_support.cc:139: std::move(frame), On 2017/05/18 20:51:32, Fady Samuel wrote: > Shouldn't ...
3 years, 7 months ago (2017-05-19 18:28:17 UTC) #146
danakj
https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc#newcode25 cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/18 20:44:37, Saman Sami wrote: ...
3 years, 7 months ago (2017-05-19 19:00:59 UTC) #159
Saman Sami
https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc#newcode25 cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/19 19:00:59, danakj wrote: > ...
3 years, 7 months ago (2017-05-19 19:26:47 UTC) #162
danakj
LGTM https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc File cc/test/compositor_frame_helpers.cc (right): https://codereview.chromium.org/2848223003/diff/520001/cc/test/compositor_frame_helpers.cc#newcode25 cc/test/compositor_frame_helpers.cc:25: frame.metadata.begin_frame_ack.has_damage = true; On 2017/05/19 19:26:46, Saman Sami ...
3 years, 7 months ago (2017-05-19 19:31:40 UTC) #163
Saman Sami
boliu, please review android. piman, please review content.
3 years, 7 months ago (2017-05-23 16:16:11 UTC) #188
Fady Samuel
https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc#newcode148 cc/surfaces/compositor_frame_sink_support.cc:148: return true; Why are we returning true here?
3 years, 7 months ago (2017-05-23 16:40:47 UTC) #189
boliu
lgtm
3 years, 7 months ago (2017-05-23 16:43:31 UTC) #190
Saman Sami
https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc#newcode148 cc/surfaces/compositor_frame_sink_support.cc:148: return true; On 2017/05/23 16:40:47, Fady Samuel wrote: > ...
3 years, 7 months ago (2017-05-23 16:48:11 UTC) #191
Fady Samuel
On 2017/05/23 16:48:11, Saman Sami wrote: > https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc > File cc/surfaces/compositor_frame_sink_support.cc (right): > > https://codereview.chromium.org/2848223003/diff/780001/cc/surfaces/compositor_frame_sink_support.cc#newcode148 ...
3 years, 7 months ago (2017-05-23 16:49:19 UTC) #192
piman
https://codereview.chromium.org/2848223003/diff/780001/content/renderer/android/synchronous_compositor_frame_sink.h File content/renderer/android/synchronous_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/780001/content/renderer/android/synchronous_compositor_frame_sink.h#newcode161 content/renderer/android/synchronous_compositor_frame_sink.h:161: float device_scale_factor_; new POD field needs new initializer
3 years, 7 months ago (2017-05-23 18:03:17 UTC) #193
Saman Sami
https://codereview.chromium.org/2848223003/diff/780001/content/renderer/android/synchronous_compositor_frame_sink.h File content/renderer/android/synchronous_compositor_frame_sink.h (right): https://codereview.chromium.org/2848223003/diff/780001/content/renderer/android/synchronous_compositor_frame_sink.h#newcode161 content/renderer/android/synchronous_compositor_frame_sink.h:161: float device_scale_factor_; On 2017/05/23 18:03:17, piman wrote: > new ...
3 years, 7 months ago (2017-05-23 18:31:08 UTC) #201
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2848223003/800001
3 years, 7 months ago (2017-05-23 21:03:22 UTC) #212
commit-bot: I haz the power
Committed patchset #16 (id:800001) as https://chromium.googlesource.com/chromium/src/+/5ba3188fa05666ae8d6031706c0963532f9d1457
3 years, 7 months ago (2017-05-23 21:10:07 UTC) #215
nednguyen
3 years, 6 months ago (2017-05-24 14:15:34 UTC) #216
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:800001) has been created in
https://codereview.chromium.org/2905523003/ by nednguyen@google.com.

The reason for reverting is: Suspect this breaks SurfaceAggregatorPerfTest 

BUG=725840.

Powered by Google App Engine
This is Rietveld 408576698