Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restore ability to disable color correct rendering #15898

Merged
merged 1 commit into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@poiru
Copy link
Member

commented Nov 30, 2018

In Electron 2.0, --disable-features=ColorCorrectRendering could be
used to make the app use the display color space (e.g. P3 on Macs)
instead of color correcting to sRGB. Because color correct rendering is
always enabled on Chromium 62 and later and because
--force-color-profile has no effect on macOS, apps that need e.g. P3
colors are currently stuck on Electron 2.0.

This restores the functionality removed in
https://chromium-review.googlesource.com/698347 in the form of the
--disable-color-correct-rendering switch.

This can be removed once web content (including WebGL) learn how
to deal with color spaces. That is being tracked at
https://crbug.com/634542 and https://crbug.com/711107.

As an example of a widely used app using
--disable-features=ColorCorrectRendering, see VSCode:
https://github.com/Microsoft/vscode/blob/3f33ef2593d3efe6eca5230f3d34d3406fb73498/src/main.js#L138-L139

Notes: Add --disable-color-correct-rendering switch

@poiru poiru requested a review from nornagon Nov 30, 2018

@poiru poiru requested a review from as a code owner Nov 30, 2018

@poiru poiru force-pushed the disable-color-correct-rendering branch from 7c77850 to 8a88557 Nov 30, 2018

@poiru

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I confirmed that #FF0000 with disable-color-correct-rendering are more saturated on a Mac with a P3 display. I checked colors generated with WebGL, CSS, and a PNG image. Here's the code:

main.js:

const { app, BrowserWindow } = require('electron');

app.commandLine.appendSwitch('disable-color-correct-rendering');

let win = null;

app.once('ready', () => {
  win = new BrowserWindow({
    width: 300,
    height: 600,
    webPreferences: {
      nodeIntegration: false
    }
  });

  win.loadURL(`file://${__dirname}/test.html`);
});

test.html:

<html>
<head>
  <meta http-equiv="Content-Security-Policy" content="default-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'; img-src 'self' data:">
  <style>
  body, html {
    width: 100%;
    height: 100%;
    font-family: sans-serif;
    font-size: 10px;
  }
  canvas, div, img {
    width: 100px;
    height: 100px;
    border: 2px solid black;
  }
  </style>
</head>
<body>
  <p>webgl canvas</p>
  <canvas></canvas>
  <p>css</p>
  <div style="background-color: red"></div>
  <p>img</p>
  <img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEX/AAAZ4gk3AAAACklEQVR4nGNiAAAABgADNjd8qAAAAABJRU5ErkJggg=="/>
  <p id="ua"></p>
</body>
<script>
window.addEventListener('load', () => {
  var canvas = document.querySelector('canvas');
  var gl = canvas.getContext('webgl');
  gl.viewport(0, 0, gl.drawingBufferWidth, gl.drawingBufferHeight);
  gl.clearColor(1.0, 0.0, 0.0, 1.0);
  gl.clear(gl.COLOR_BUFFER_BIT);

  document.querySelector('#ua').innerHTML = navigator.userAgent;
}, false);
</script>

@poiru poiru force-pushed the disable-color-correct-rendering branch from 8a88557 to fd56b87 Dec 3, 2018

@nornagon
Copy link
Contributor

left a comment

It is far from clear to me that this patches all the necessary places, and I'm concerned about continuing support for this flag in the face of future upstream refactors in Chrome's rendering code, but I'm satisfied that at least for this particular version of Chrome that this patch does not change behaviour unless you pass the flag, so I'm approving anyway.

Please be prepared for this flag to get disabled in a future chrome upgrade if you're not around to help port it :)

RendererSettings renderer_settings;
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+ renderer_settings.enable_color_correct_rendering =
+ !command_line->HasSwitch("disable-color-correct-rendering");

This comment has been minimized.

Copy link
@nornagon

nornagon Dec 10, 2018

Contributor

please add a string constant kDisableColorCorrectRendering so that typos will be a compile error :)

This comment has been minimized.

Copy link
@poiru

poiru Dec 11, 2018

Author Member

Done!

fix: restore ability to disable color correct rendering
In Electron 2.0, `--disable-features=ColorCorrectRendering` could be
used to make the app use the display color space (e.g. P3 on Macs)
instead of color correcting to sRGB. Because color correct rendering is
always enabled on Chromium 62 and later and because
`--force-color-profile` has no effect on macOS, apps that need e.g. P3
colors are currently stuck on Electron 2.0.

This restores the functionality removed in
https://chromium-review.googlesource.com/698347 in the form of the
`--disable-color-correct-rendering` switch.

This can be removed once web content (including WebGL) learn how
to deal with color spaces. That is being tracked at
https://crbug.com/634542 and https://crbug.com/711107.

As an example of a widely used app using
`--disable-features=ColorCorrectRendering`, see VSCode:
https://github.com/Microsoft/vscode/blob/3f33ef2593d3efe6eca5230f3d34d3406fb73498/src/main.js#L138-L139

Notes: Add `--disable-color-correct-rendering` switch

@poiru poiru force-pushed the disable-color-correct-rendering branch from fd56b87 to 3af9274 Dec 11, 2018

@deepak1556 deepak1556 merged commit e383aa3 into master Dec 11, 2018

26 of 29 checks passed

Backportable? - 3-1-x Backport Failed
Details
Backportable? - 4-0-x Backport Failed
Details
electron-mas-testing Build #20181211.17 has test failures
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181211.10 succeeded
Details
electron-arm64-testing Build #20181211.10 succeeded
Details
electron-lint Build #20181211.8 succeeded
Details
electron-osx-testing Build #20181211.18 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Dec 11, 2018

Release Notes Persisted

Add --disable-color-correct-rendering switch

@deepak1556 deepak1556 deleted the disable-color-correct-rendering branch Dec 11, 2018

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

I was unable to backport this PR to "3-1-x" cleanly;
you will need to perform this backport manually.

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

poiru added a commit that referenced this pull request Dec 11, 2018

fix: restore ability to disable color correct rendering (backport: 4-…
…0-x)

Backport of #15898

See that PR for details.

Notes: Add `--disable-color-correct-rendering` switch

ckerr added a commit that referenced this pull request Dec 11, 2018

fix: restore ability to disable color correct rendering (backport: 4-…
…0-x) (#16020)

Backport of #15898

See that PR for details.

Notes: Add `--disable-color-correct-rendering` switch
@bpasero

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@poiru did this ever land on 3-0-x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.