New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 735658 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

color: WCG color support on Android

Project Member Reported by ccameron@chromium.org, Jun 21 2017

Issue description

A few work items for color correct rendering on Android

1. We need to somewhere find out what color mode the device is in (sRGB or P3 IIUC for now).

2. Plumb this into display::Display, and send to the renderer via  WebContentsViewAndroid::GetScreenInfo.

3. Plumb this to cc::Display::SetColorSpace in content::CompositorImpl.

4. Populate cc::RendererSettings flags in content::CompositorImpl correctly based on features.

5. Specify the color space of the output surface (haven't found that one yet).

IIUC this is all that's missing. To be proven wrong.
 
For 5, that can be specified by EGL_KHR_gl_colorspace.

We'll need to plumb it down through to
  gl::NativeViewGLSurfaceEGL, from
  ImageTransportSurface::CreateNativeSurface, from
  gpu::GpuCommandBufferStub/InProcessCommandBuffer::Initialize, probably from
  ContextCreationAttribHelper, maybe from
  GetCompositorContextAttributes in compositor_impl_android.cc
  ... and I think that's about as far back as we need to go.

Whatever signal it is that we use in this location needs to be the same signal that we use in DisplayAndroidManager::UpdateDisplay. Fortunately they're both in the browser process, so life is less complicated than it could be.
And, re #1, we can just pull the color space out of display::Display in compositor_impl_android.

The details of webview, however, are still unclear.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdf3733167497d23d93981188c0599f6d51bda5b

commit fdf3733167497d23d93981188c0599f6d51bda5b
Author: ccameron <ccameron@chromium.org>
Date: Sun Jun 25 07:18:32 2017

android WCG support: Add missing plumbing

Add a stub in populating the display::Display, where we will ultimately
decide between P3 and sRGB. The code to detect which is appropriate is
not present yet.

Populate the cc::RendererSettings::color_correct_rendering and
ScreenInfo::color_space fields correctly.

TBR=hubbe
BUG=735658

Review-Url: https://codereview.chromium.org/2953393002
Cr-Commit-Position: refs/heads/master@{#482169}

[modify] https://crrev.com/fdf3733167497d23d93981188c0599f6d51bda5b/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/fdf3733167497d23d93981188c0599f6d51bda5b/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/fdf3733167497d23d93981188c0599f6d51bda5b/ui/android/display_android_manager.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c78e51eefd44cc3514c50fce94d180df184fea9

commit 8c78e51eefd44cc3514c50fce94d180df184fea9
Author: Christopher Cameron <ccameron@chromium.org>
Date: Wed Jul 12 01:41:08 2017

color: Plumb android color space support through to EGL

We need to specify a color space eglCreateWindowSurface based on the
display properties.

We populate the display::Display::color_space as sRGB or P3 in
DisplayAndroidManager.

Convert this gfx::ColorSpace to a ContextCreationAttribHelper attribute
in compositor_impl_android.cc's helper functions.

Convert the attribute to a property on GLSurfaceFormat in
GpuCommandbufferStub.

Use the property to specify the value for EGL_GL_COLORSPACE_KHR for
eglCreateWindowSurface in GLSurfaceEGL (if the required extensions are
present).

Bug: 735658
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I579f37b7e940ef5d7a4e93ec2db551265c92c5bc
Reviewed-on: https://chromium-review.googlesource.com/550951
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Bauman <jbauman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485778}
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/content/browser/renderer_host/compositor_impl_android.h
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/gpu/command_buffer/common/gles2_cmd_utils.h
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/gpu/ipc/common/gpu_command_buffer_traits_multi.h
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/gpu/ipc/service/gpu_command_buffer_stub.cc
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/ui/gfx/color_space.cc
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/ui/gfx/color_space.h
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/ui/gl/gl_surface_egl.cc
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/ui/gl/gl_surface_format.cc
[modify] https://crrev.com/8c78e51eefd44cc3514c50fce94d180df184fea9/ui/gl/gl_surface_format.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc1ae6d669603f0b75213e7557c15cef80416989

commit bc1ae6d669603f0b75213e7557c15cef80416989
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Jul 18 05:20:46 2017

color: Detect Android wide color gamut displays

There are two variables that determine if Chrome should render P3
content:
  * Configuration.isScreenWideColorGamut reports whether or not the
    system compositor will perform color correction for EGL surfaces
    that are tagged with a color space.
  * Display.isWideColorGamut reports whether or not this particular
    display has a wide color gamut.

The Configuration.isScreenWideColorGamut variable must be read from the
WindowAndroid's context.

Update both of these variables, and plumb their conjunction down into
the native code, where it is used to select P3 or sRGB.

This mechanism is only used by Chrome, not WebView. For WebView, add hooks
in WebContentsViewDelegate to override the color space that is passed to
the renderer (instead of just using the one from the display::Display).

R=boliu
TBR=mthiesse

Bug: 735658
Change-Id: I7781b4a1068a57a480123a58302fe484273565fd
Reviewed-on: https://chromium-review.googlesource.com/571053
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487391}
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/android_webview/browser/aw_web_contents_view_delegate.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/android_webview/browser/aw_web_contents_view_delegate.h
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/content/public/browser/web_contents_view_delegate.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/content/public/browser/web_contents_view_delegate.h
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/display_android_manager.cc
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/display_android_manager.h
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/java/src/org/chromium/ui/base/WindowAndroid.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/java/src/org/chromium/ui/display/DisplayAndroidManager.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/java/src/org/chromium/ui/display/PhysicalDisplayAndroid.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/android/java/src/org/chromium/ui/display/VirtualDisplayAndroid.java
[modify] https://crrev.com/bc1ae6d669603f0b75213e7557c15cef80416989/ui/display/display_list.cc

Hmm, this doesn't work in Canary for me, but does work in Chromium. Something is off.

Comment 7 by aelias@chromium.org, Jul 18 2017

Cc: ntfschr@chromium.org
Sounds like some kind of SDK roll/API level type issue.  +ntfschr who sent out last post about this: https://groups.google.com/a/google.com/forum/#!topic/clank-team/YavyQTwBUs4 .  Would it help to try the next-* Canary, for instance?
What build of Android is the device flashed to?
Pardon. Facepalm.

The Canary build I'm trying is 61.0.3160.0 aka r487322.

The fix above is r487391.

So, this should not work in Canary.

Root cause: version numbers are too high for me to count on my fingers.
This is long-finished and then un-shipped in issue 771740.
Owner: ericrk@chromium.org
->ericrk if we're going to ship this again

Sign in to add a comment