Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
967733005:
[ozone] Add an about flag to enable ozone overlay testing. (Closed)
|
Created:
5 years, 9 months ago by alexst (slow to review) Modified:
5 years, 9 months ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ozone] Add an about flag to enable ozone overlay testing.
BUG=
Committed: https://crrev.com/59e9a996f54bb238beb5a51dd864eb0febb8728c
Cr-Commit-Position: refs/heads/master@{#318964}
Committed: https://crrev.com/24a14a32b1db77ad2c454a459d0582fe59524c4f
Cr-Commit-Position: refs/heads/master@{#319967}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 26 (9 generated)
alexst@chromium.org changed reviewers: + achaulk@chromium.org
alexst@chromium.org changed reviewers: + achaulk@chromium.org
Patchset #1 (id:1) has been deleted
alexst@chromium.org changed reviewers: + ccameron@chromium.org
+ccameron for gpu_process_transport_factory.cc owners.
lgtm
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... content/browser/compositor/gpu_process_transport_factory.cc:127: (command_line->HasSwitch(switches::kEnableHardwareOverlays) || This appears to be the only use of kEnableHardwareOverlays. Why do we need a 2nd switch?
On 2015/03/03 19:52:09, danakj wrote: > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > content/browser/compositor/gpu_process_transport_factory.cc:127: > (command_line->HasSwitch(switches::kEnableHardwareOverlays) || > This appears to be the only use of kEnableHardwareOverlays. Why do we need a 2nd > switch? One enables hardware overlays in general, one enables non-validated single overlay for testing. kEnableHardwareOverlays = general case, validate fully with the platform. kOzoneTestSingleOverlaySupport = always allow one overlay. This flag exists because the underlying kernel api's for proper validation are not stable yet, but we can make useful progress in other parts of the stack.
On 2015/03/03 19:59:02, alexst wrote: > On 2015/03/03 19:52:09, danakj wrote: > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > content/browser/compositor/gpu_process_transport_factory.cc:127: > > (command_line->HasSwitch(switches::kEnableHardwareOverlays) || > > This appears to be the only use of kEnableHardwareOverlays. Why do we need a > 2nd > > switch? > > One enables hardware overlays in general, one enables non-validated single > overlay for testing. > > kEnableHardwareOverlays = general case, validate fully with the platform. > kOzoneTestSingleOverlaySupport = always allow one overlay. This flag exists > because the underlying kernel api's for proper validation are not stable yet, > but we can make useful progress in other parts of the stack. So you're saying this will allow you to make kEnableHardwareOverlays do more things? Cuz right now, general case == this one overlay case?
On 2015/03/03 20:00:33, danakj wrote: > On 2015/03/03 19:59:02, alexst wrote: > > On 2015/03/03 19:52:09, danakj wrote: > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > > > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > content/browser/compositor/gpu_process_transport_factory.cc:127: > > > (command_line->HasSwitch(switches::kEnableHardwareOverlays) || > > > This appears to be the only use of kEnableHardwareOverlays. Why do we need a > > 2nd > > > switch? > > > > One enables hardware overlays in general, one enables non-validated single > > overlay for testing. > > > > kEnableHardwareOverlays = general case, validate fully with the platform. > > kOzoneTestSingleOverlaySupport = always allow one overlay. This flag exists > > because the underlying kernel api's for proper validation are not stable yet, > > but we can make useful progress in other parts of the stack. > > So you're saying this will allow you to make kEnableHardwareOverlays do more > things? Cuz right now, general case == this one overlay case? You could consider making --enable-hardware-overlays=single do this too, so we have fewer flags?
On 2015/03/03 20:00:33, danakj wrote: > On 2015/03/03 19:59:02, alexst wrote: > > On 2015/03/03 19:52:09, danakj wrote: > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > > > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > content/browser/compositor/gpu_process_transport_factory.cc:127: > > > (command_line->HasSwitch(switches::kEnableHardwareOverlays) || > > > This appears to be the only use of kEnableHardwareOverlays. Why do we need a > > 2nd > > > switch? > > > > One enables hardware overlays in general, one enables non-validated single > > overlay for testing. > > > > kEnableHardwareOverlays = general case, validate fully with the platform. > > kOzoneTestSingleOverlaySupport = always allow one overlay. This flag exists > > because the underlying kernel api's for proper validation are not stable yet, > > but we can make useful progress in other parts of the stack. > > So you're saying this will allow you to make kEnableHardwareOverlays do more > things? Cuz right now, general case == this one overlay case? Ozone allows people to live out of tree, so in theory there may already exist an ozone platform, written by someone else we don't know about that uses this flag, fully validates overlays (hence not needing kOzoneTestSingleOverlaySupport) and uses kEnableHardwareOverlays to turn things on and off globally. We are not that advanced yet, so we added a special case kOzoneTestSingleOverlaySupport that lets us make progress. Eventually, when will not need kOzoneTestSingleOverlaySupport, so it'll get removed and only kEnableHardwareOverlays will be left.
On 2015/03/03 20:05:32, alexst wrote: > On 2015/03/03 20:00:33, danakj wrote: > > On 2015/03/03 19:59:02, alexst wrote: > > > On 2015/03/03 19:52:09, danakj wrote: > > > > > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/967733005/diff/20001/content/browser/composit... > > > > content/browser/compositor/gpu_process_transport_factory.cc:127: > > > > (command_line->HasSwitch(switches::kEnableHardwareOverlays) || > > > > This appears to be the only use of kEnableHardwareOverlays. Why do we need > a > > > 2nd > > > > switch? > > > > > > One enables hardware overlays in general, one enables non-validated single > > > overlay for testing. > > > > > > kEnableHardwareOverlays = general case, validate fully with the platform. > > > kOzoneTestSingleOverlaySupport = always allow one overlay. This flag exists > > > because the underlying kernel api's for proper validation are not stable > yet, > > > but we can make useful progress in other parts of the stack. > > > > So you're saying this will allow you to make kEnableHardwareOverlays do more > > things? Cuz right now, general case == this one overlay case? > > Ozone allows people to live out of tree, so in theory there may already exist an > ozone platform, written by someone else we don't know about that uses this flag, > fully validates overlays (hence not needing kOzoneTestSingleOverlaySupport) and > uses kEnableHardwareOverlays to turn things on and off globally. > > We are not that advanced yet, so we added a special case > kOzoneTestSingleOverlaySupport that lets us make progress. Eventually, when will > not need kOzoneTestSingleOverlaySupport, so it'll get removed and only > kEnableHardwareOverlays will be left. Oh interesting... ok thanks for the explanation. LGTM :)
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967733005/20001
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/59e9a996f54bb238beb5a51dd864eb0febb8728c Cr-Commit-Position: refs/heads/master@{#318964}
alexst@chromium.org changed reviewers: + asvitkine@chromium.org
Reland. +asvitkine for tools/metrics/histograms/histograms.xml
lgtm
The CQ bit was checked by alexst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, ccameron@chromium.org Link to the patchset: https://codereview.chromium.org/967733005/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967733005/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/24a14a32b1db77ad2c454a459d0582fe59524c4f Cr-Commit-Position: refs/heads/master@{#319967} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
