Bug 177185 - [WPE][GTK] Add support for ICC color management
Summary: [WPE][GTK] Add support for ICC color management
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 223442
  Show dependency treegraph
 
Reported: 2017-09-19 13:27 PDT by Alex
Modified: 2021-03-18 06:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (28.13 KB, patch)
2021-03-10 06:28 PST, Carlos Garcia Campos
aperez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex 2017-09-19 13:27:16 PDT
Reporting bug to Webkit as GNOME Web browser developer suggested:
https://bugzilla.gnome.org/show_bug.cgi?id=787876

Safari and Chrome have color management and are Webkit based. Can WebkitGTK+ based GNOME Web have it to?
Comment 1 Alberto Garcia 2019-04-24 03:53:00 PDT
More details here:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=880697

In short, WebKitGTK does not show this image with the right colors (compare it with other browsers to see the difference):

http://okgo.net/build/wp-content/uploads/2016/11/Email-Blast-Photo.jpg
Comment 2 Michael Catanzaro 2019-05-16 08:11:19 PDT
So a quick internet search suggests we need this for BMP, JPEG, JPEG 2000, PNG, and WebP. But not for GIF or ICO.
Comment 3 Alberto Garcia 2020-09-15 14:36:46 PDT
Still happens with 2.30.0
Comment 4 Carlos Garcia Campos 2021-03-10 06:28:56 PST
Created attachment 422825 [details]
Patch
Comment 5 Alberto Garcia 2021-03-10 07:54:09 PST
(In reply to Carlos Garcia Campos from comment #4)
> Created attachment 422825 [details]
> Patch

I tested it with WebKitGTK 2.30.5 and I confirm that it solves the problem, thanks!!
Comment 6 Adrian Perez 2021-03-10 08:45:21 PST
Comment on attachment 422825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422825&action=review

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:278
> +{

It would be nice here to fall-back to asking colord what's the color profile for
the screen. That would work also for Wayland-based compositors and desktop
environments which populate colord's view of the available screens (at least
GNOME Shell and Weston with the cms-colord plug-in do this).

Returning a hardwired RGB profile would be in my mind the last of resorts
after trying colord.

By the way, I think it's fine to skip adding support for colord in this patch,
but if you agree that it would be nice to consider it, let's create a new bug
to track that and maybe add a TODO/FIXME in the code ^_^

> Source/cmake/FindLCMS2.cmake:70
> +

I would rather prefer to use an imported target in this CMake find-module.
We have been transitioning towards using imported targets, for example
FindManette.cmake uses an imported target and also does the dance of
regex-matching the version from the headers as a fallback so I think it
would be a good one to use as template here :)
Comment 7 Michael Catanzaro 2021-03-10 11:00:44 PST
Comment on attachment 422825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422825&action=review

>> Source/WebCore/platform/graphics/PlatformDisplay.cpp:278
>> +{
> 
> It would be nice here to fall-back to asking colord what's the color profile for
> the screen. That would work also for Wayland-based compositors and desktop
> environments which populate colord's view of the available screens (at least
> GNOME Shell and Weston with the cms-colord plug-in do this).
> 
> Returning a hardwired RGB profile would be in my mind the last of resorts
> after trying colord.
> 
> By the way, I think it's fine to skip adding support for colord in this patch,
> but if you agree that it would be nice to consider it, let's create a new bug
> to track that and maybe add a TODO/FIXME in the code ^_^

Beware: talking to colord will only work if you do it in the UI process.

It might seem to work during development, because I know cgarcia has the web process sandbox disabled. It won't work for users, though. :P
Comment 8 Adrian Perez 2021-03-11 02:51:54 PST
Comment on attachment 422825 [details]
Patch

Changed to r+, after some discussion with Carlos Garcia, we agreed on
landing this as it covers common cases (as most consumer displays are
sRGB anyway), and we will file a separate bug regarding colord. The
CMake find-module can be rewritten later on as a follow-up, too.

:)
Comment 9 Carlos Garcia Campos 2021-03-11 03:00:00 PST
Committed r274273 (235170@main): <https://commits.webkit.org/235170@main>