Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307083003:
Send the ICC profile to the compositor under aura. (Closed)
|
Created:
4 years, 3 months ago by hubbe Modified:
4 years, 3 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend the ICC profile to the compositor under aura.
Also make sure that we remember the color space even if the compositor has not
been created yet.
BUG=622133
Committed: https://crrev.com/bf9fe707d077f33dee601719f647a8122e5cf04e
Cr-Commit-Position: refs/heads/master@{#418690}
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments addressed #
Total comments: 2
Patch Set 3 : comments addressed #
Total comments: 4
Patch Set 4 : comment updates #
Total comments: 5
Patch Set 5 : move to window_tree_host #
Total comments: 3
Patch Set 6 : add todo #Patch Set 7 : merged #
Messages
Total messages: 61 (34 generated)
The CQ bit was checked by hubbe@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.
hubbe@chromium.org changed reviewers: + ccameron@chromium.org
ccameron@chromium.org changed reviewers: + danakj@chromium.org
Adding danakj to comment on the GPTF bit https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:687: const gfx::ColorSpace& color_space) { danakj, could you verify the correctness of this -- IIRC you mentioned a need to do something to handle this case before I left, but now I forget what the details were. https://codereview.chromium.org/2307083003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: if (!(icc_profile == gfx::ICCProfile())) { We don't need to check icc_profile == gfx::ICCProfile() here -- the call to .GetColorSpace will still succeed, and the result will be the previously-existing value.
https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:690: data = CreatePerCompositorData(compositor); I don't think you want to do this. Instead just store it in the ui::Compositor and set it on the GPTF when you make a new output surface. Take a look at how visible is done.
https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:687: const gfx::ColorSpace& color_space) { On 2016/09/02 21:10:32, ccameron wrote: > danakj, could you verify the correctness of this -- IIRC you mentioned a need to > do something to handle this case before I left, but now I forget what the > details were. Acknowledged. https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:690: data = CreatePerCompositorData(compositor); On 2016/09/02 21:11:45, danakj wrote: > I don't think you want to do this. Instead just store it in the ui::Compositor > and set it on the GPTF when you make a new output surface. Take a look at how > visible is done. Added it to ui:Compositor. Does that mean we don't need PerCompositorData::color_space anymore? https://codereview.chromium.org/2307083003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: if (!(icc_profile == gfx::ICCProfile())) { On 2016/09/02 21:10:32, ccameron wrote: > We don't need to check icc_profile == gfx::ICCProfile() here -- the call to > .GetColorSpace will still succeed, and the result will be the > previously-existing value. Done.
The CQ bit was checked by hubbe@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...
https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:690: data = CreatePerCompositorData(compositor); On 2016/09/02 21:35:49, hubbe wrote: > On 2016/09/02 21:11:45, danakj wrote: > > I don't think you want to do this. Instead just store it in the ui::Compositor > > and set it on the GPTF when you make a new output surface. Take a look at how > > visible is done. > > Added it to ui:Compositor. > Does that mean we don't need PerCompositorData::color_space anymore? Ya that'd be ideal.
https://codereview.chromium.org/2307083003/diff/20001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/2307083003/diff/20001/ui/compositor/composito... ui/compositor/compositor.cc:345: context_factory_->SetDisplayColorSpace(this, color_space_); Can you leave a comment like in SetVisible, and also on the ContextFactory interface
The CQ bit was checked by hubbe@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...
https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/1/content/browser/compositor/... content/browser/compositor/gpu_process_transport_factory.cc:690: data = CreatePerCompositorData(compositor); On 2016/09/02 21:36:47, danakj wrote: > On 2016/09/02 21:35:49, hubbe wrote: > > On 2016/09/02 21:11:45, danakj wrote: > > > I don't think you want to do this. Instead just store it in the > ui::Compositor > > > and set it on the GPTF when you make a new output surface. Take a look at > how > > > visible is done. > > > > Added it to ui:Compositor. > > Does that mean we don't need PerCompositorData::color_space anymore? > > Ya that'd be ideal. Gone. https://codereview.chromium.org/2307083003/diff/20001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/2307083003/diff/20001/ui/compositor/composito... ui/compositor/compositor.cc:345: context_factory_->SetDisplayColorSpace(this, color_space_); On 2016/09/02 21:38:28, danakj wrote: > Can you leave a comment like in SetVisible, and also on the ContextFactory > interface Done.
LGTM https://codereview.chromium.org/2307083003/diff/40001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/40001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:691: if (data->display) Can you add a comment here like in SetDisplayVisible as well https://codereview.chromium.org/2307083003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/2307083003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:271: // Visibility is reset when the output surface is lost, so update it to match Visibility => Visibility and color space. Or "Display properties are reset" or something.
lgtm2
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2307083003/diff/40001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2307083003/diff/40001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:691: if (data->display) On 2016/09/02 23:03:32, danakj wrote: > Can you add a comment here like in SetDisplayVisible as well Done. https://codereview.chromium.org/2307083003/diff/40001/ui/compositor/composito... File ui/compositor/compositor.cc (right): https://codereview.chromium.org/2307083003/diff/40001/ui/compositor/composito... ui/compositor/compositor.cc:271: // Visibility is reset when the output surface is lost, so update it to match On 2016/09/02 23:03:32, danakj wrote: > Visibility => Visibility and color space. Or "Display properties are reset" or > something. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
piman@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); Drive-by: why is this done here, it's odd... This function has to do with when a tab is added/removed from a window, on a per-renderer basis. Should a window without a renderer not have a color profile? Do we need to reset this color profile every time a new tab gets created, or gets moved to another window? I seems unrelated. Couldn't we just do this in the ui::Compositor constructor if we're going to pick an arbitrary monitor anyway?
https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/02 23:23:38, piman wrote: > Drive-by: why is this done here, it's odd... > > This function has to do with when a tab is added/removed from a window, on a > per-renderer basis. > Should a window without a renderer not have a color profile? > Do we need to reset this color profile every time a new tab gets created, or > gets moved to another window? I seems unrelated. > > Couldn't we just do this in the ui::Compositor constructor if we're going to > pick an arbitrary monitor anyway? I'll let ccameron@ answer, I put it here because he suggested it..
Sorry, I inappropriately cargo-culted that from Mac https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/02 23:23:38, piman wrote: > Drive-by: why is this done here, it's odd... > > This function has to do with when a tab is added/removed from a window, on a > per-renderer basis. > Should a window without a renderer not have a color profile? > Do we need to reset this color profile every time a new tab gets created, or > gets moved to another window? I seems unrelated. Sorry, you're right. On Mac we send this from the RWHVMac (and we do support multi-monitor there). Aura will need to be different. A more reasonable place, for now, may be at: https://cs.chromium.org/chromium/src/ui/aura/window_tree_host.cc?rcl=14728439... > Couldn't we just do this in the ui::Compositor constructor if we're going to > pick an arbitrary monitor anyway? Eventually it will need to be updated when: - The Aura window moves to a different monitor - The system's color changes (on OSes that support notifications of this ... Mac is good, not sure about the others). When we're at that point, we'll probably be having gfx::ICCProfile be a member of display::Display
https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/02 23:41:48, ccameron wrote: > On 2016/09/02 23:23:38, piman wrote: > > Drive-by: why is this done here, it's odd... > > > > This function has to do with when a tab is added/removed from a window, on a > > per-renderer basis. > > Should a window without a renderer not have a color profile? > > Do we need to reset this color profile every time a new tab gets created, or > > gets moved to another window? I seems unrelated. > > Sorry, you're right. On Mac we send this from the RWHVMac (and we do support > multi-monitor there). Aura will need to be different. > > A more reasonable place, for now, may be at: > https://cs.chromium.org/chromium/src/ui/aura/window_tree_host.cc?rcl=14728439... Ok, yes, that makes sense, thanks! > > Couldn't we just do this in the ui::Compositor constructor if we're going to > > pick an arbitrary monitor anyway? > > Eventually it will need to be updated when: > - The Aura window moves to a different monitor > - The system's color changes (on OSes that support notifications of this ... > Mac is good, not sure about the others). > > When we're at that point, we'll probably be having gfx::ICCProfile be a member > of display::Display Ok, yes, I think is reasonable, this would probably flow from the WindowTreeHost implementation (e.g. DesktopWindowTreeHost*), so moving this logic to WindowTreeHost seems like the right place.
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 hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2307083003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2676: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/02 23:56:34, piman wrote: > On 2016/09/02 23:41:48, ccameron wrote: > > On 2016/09/02 23:23:38, piman wrote: > > > Drive-by: why is this done here, it's odd... > > > > > > This function has to do with when a tab is added/removed from a window, on a > > > per-renderer basis. > > > Should a window without a renderer not have a color profile? > > > Do we need to reset this color profile every time a new tab gets created, or > > > gets moved to another window? I seems unrelated. > > > > Sorry, you're right. On Mac we send this from the RWHVMac (and we do support > > multi-monitor there). Aura will need to be different. > > > > A more reasonable place, for now, may be at: > > > https://cs.chromium.org/chromium/src/ui/aura/window_tree_host.cc?rcl=14728439... > > Ok, yes, that makes sense, thanks! > > > > Couldn't we just do this in the ui::Compositor constructor if we're going to > > > pick an arbitrary monitor anyway? > > > > Eventually it will need to be updated when: > > - The Aura window moves to a different monitor > > - The system's color changes (on OSes that support notifications of this ... > > Mac is good, not sure about the others). > > > > When we're at that point, we'll probably be having gfx::ICCProfile be a member > > of display::Display > > Ok, yes, I think is reasonable, this would probably flow from the WindowTreeHost > implementation (e.g. DesktopWindowTreeHost*), so moving this logic to > WindowTreeHost seems like the right place. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
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 hubbe@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/2307083003/#ps80001 (title: "move to window_tree_host")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hubbe@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/aura/window_tree_host.cc
https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host.cc:73: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); What is FromBestMonitor? Shouldn't you pick the profile based on the monitor the WindowTreeHost is going to be used on, and change it if moved to a different monitor?
hubbe@google.com changed reviewers: + hubbe@google.com
https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host.cc:73: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/07 20:15:58, sky wrote: > What is FromBestMonitor? Shouldn't you pick the profile based on the monitor the > WindowTreeHost is going to be used on, and change it if moved to a different > monitor? Yes we should, but we don't have the ability yet. It's a TODO at this point. (I can add an actual TODO if you like)
LGTM with a TODO https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... File ui/aura/window_tree_host.cc (right): https://codereview.chromium.org/2307083003/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host.cc:73: gfx::ICCProfile::FromBestMonitor().GetColorSpace()); On 2016/09/07 20:29:25, hubbe1 wrote: > On 2016/09/07 20:15:58, sky wrote: > > What is FromBestMonitor? Shouldn't you pick the profile based on the monitor > the > > WindowTreeHost is going to be used on, and change it if moved to a different > > monitor? > > Yes we should, but we don't have the ability yet. > It's a TODO at this point. (I can add an actual TODO if you like) Yes please.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, sky@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2307083003/#ps100001 (title: "add todo")
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
Failed to apply patch for ui/compositor/compositor.cc:
While running git apply --index -3 -p1;
error: patch failed: ui/compositor/compositor.cc:268
Falling back to three-way merge...
Applied patch to 'ui/compositor/compositor.cc' with conflicts.
U ui/compositor/compositor.cc
Patch: ui/compositor/compositor.cc
Index: ui/compositor/compositor.cc
diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index
a08204f612aeabd79e75fb50544840835788f396..7e5b88be7e51640d9de66ecb44310502e9ba2ca9
100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -268,9 +268,10 @@ void Compositor::SetOutputSurface(
std::unique_ptr<cc::OutputSurface> output_surface) {
output_surface_requested_ = false;
host_->SetOutputSurface(std::move(output_surface));
- // Visibility is reset when the output surface is lost, so update it to match
- // the Compositor's.
+ // Display properties are reset when the output surface is lost, so update it
+ // to match the Compositor's.
context_factory_->SetDisplayVisible(this, host_->visible());
+ context_factory_->SetDisplayColorSpace(this, color_space_);
}
void Compositor::ScheduleDraw() {
@@ -340,7 +341,10 @@ void Compositor::SetScaleAndSize(float scale, const
gfx::Size& size_in_pixel) {
}
void Compositor::SetDisplayColorSpace(const gfx::ColorSpace& color_space) {
- context_factory_->SetDisplayColorSpace(this, color_space);
+ color_space_ = color_space;
+ // Color space is reset when the output surface is lost, so this must also be
+ // updated then.
+ context_factory_->SetDisplayColorSpace(this, color_space_);
}
void Compositor::SetBackgroundColor(SkColor color) {
The CQ bit was checked by hubbe@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 hubbe@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, sky@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2307083003/#ps120001 (title: "merged")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Send the ICC profile to the compositor under aura. Also make sure that we remember the color space even if the compositor has not been created yet. BUG=622133 ========== to ========== Send the ICC profile to the compositor under aura. Also make sure that we remember the color space even if the compositor has not been created yet. BUG=622133 Committed: https://crrev.com/bf9fe707d077f33dee601719f647a8122e5cf04e Cr-Commit-Position: refs/heads/master@{#418690} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bf9fe707d077f33dee601719f647a8122e5cf04e Cr-Commit-Position: refs/heads/master@{#418690} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
