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

Adds wide-gamut support #3882

Closed
wants to merge 4 commits into from
Closed

Adds wide-gamut support #3882

wants to merge 4 commits into from

Conversation

fserb
Copy link
Contributor

@fserb fserb commented Aug 3, 2018

This is pulling a part of: https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpaceProposal.md into the spec.

This includes Context 2D creation params and adding a new convertToBlob function to canvas and offscreenCanvas.

@domenic @whatwg/canvas


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:58 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Command failed: /home/noderunner/wattsi/bin/wattsi /tmp/upload_0c739482382be255156fa8e57febe1a0 (sha not provided) rupbfr97o1k default /tmp/upload_23a1dd449af8b88e382686a6d3de6c7a

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@fserb
Copy link
Contributor Author

fserb commented Aug 3, 2018

@smfr
Copy link

smfr commented Aug 4, 2018

^gamut

@fserb fserb changed the title Adds wide-gammut support Adds wide-gamut support Aug 4, 2018
@fserb
Copy link
Contributor Author

fserb commented Aug 4, 2018

ops. :) thanks.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this happening! This will also fix an open issue (haven't looked for it yet).

I think this would be easier to review if you could split the pixelFormat and convertToBlob changes into separate PRs. Would that be too much of a hassle?

Is there a plan for tests?

source Outdated
@@ -60069,6 +60082,15 @@ interface <dfn>Path2D</dfn> {

</dd>

<dt><var>context</var> = <var>canvas</var> . <code data-x="dom-canvas-getContext">getContext</code>('2d' [, { [ <code data-x="dom-CanvasRenderingContext2DSettings-pixelFormat">pixelFormat</code>: "float16" ] } ] )</dt>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little weird if we don't also document the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this really is stressing the limits our domintro pseudo-language.

I would suggest rolling this into the previous dt/dd, as they are not mutually exclusive. My suggestion is either one of

context = canvas . getContext('2d' [, { [ alpha: false ] [, pixelFormat: "float16" ] } ] )

or give up on documenting all optionality/non-defaults and do

context = canvas . getContext('2d' [, { alpha, pixelFormat } ] )

@annevk
Copy link
Member

annevk commented Aug 8, 2018

Pinging @whatwg/canvas separately from OP as it doesn't work there due to bot reasons...

@fserb
Copy link
Contributor Author

fserb commented Aug 15, 2018

Left pixelFormat here and moving convertToBlob to a different change.
PTAL.

@annevk annevk added addition/proposal New features or enhancements needs tests Moving the issue forward requires someone to write tests impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation labels Aug 16, 2018
@annevk
Copy link
Member

annevk commented Aug 16, 2018

Comments:

  • If we document float16, we should also document 8-8-8-8 (maybe int8 would be more aligned?).
  • It seems to me we should also describe how this impacts getImageData(). In this PR it doesn't seem to be defined at all what this new color space actually means for an implementation.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem integrated into https://html.spec.whatwg.org/#offscreen-2d-context-creation-algorithm or https://html.spec.whatwg.org/#imagebitmaprenderingcontext-creation-algorithm . Is that intentional, or should those support pixelFormat too?

@@ -59318,6 +59318,17 @@ dictionary <dfn>AssignedNodesOptions</dfn> {
<dd w-nodev>
<pre><code class="idl" data-x="">typedef (<span>CanvasRenderingContext2D</span> or <span>ImageBitmapRenderingContext</span> or <span>WebGLRenderingContext</span>) <dfn>RenderingContext</dfn>;

enum <dfn>CanvasPixelFormat</dfn> {
"8-8-8-8", // default
"float16",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No trailing comma

source Outdated
@@ -60069,6 +60082,15 @@ interface <dfn>Path2D</dfn> {

</dd>

<dt><var>context</var> = <var>canvas</var> . <code data-x="dom-canvas-getContext">getContext</code>('2d' [, { [ <code data-x="dom-CanvasRenderingContext2DSettings-pixelFormat">pixelFormat</code>: "float16" ] } ] )</dt>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this really is stressing the limits our domintro pseudo-language.

I would suggest rolling this into the previous dt/dd, as they are not mutually exclusive. My suggestion is either one of

context = canvas . getContext('2d' [, { [ alpha: false ] [, pixelFormat: "float16" ] } ] )

or give up on documenting all optionality/non-defaults and do

context = canvas . getContext('2d' [, { alpha, pixelFormat } ] )


<dl>
<dt><dfn><code data-x="dom-CanvasRenderingContext2DSettings-pixelFormat">pixelFormat</code></dfn></dt>
<dd>Decides the backing storage for the <code data-x="dom-context-2d-canvas">canvas</code>'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a description, but doesn't make any statements requiring the user agent to do anything. Some questions:

  • What does this impact, observably? Does it change the numeric values returned from any canvas or pixel-reading APIs?
  • Is "backing storage" the same as the CanvasRenderingContext2D's output bitmap?
  • What does "optional" mean? Does it mean fall back to 8-8-8, or throw an exception, or...?
  • Where is this information stored? What parts of the spec later use it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fserb looks like this is waiting for you to address change requests from @domenic

@@ -59318,6 +59318,17 @@ dictionary <dfn>AssignedNodesOptions</dfn> {
<dd w-nodev>
<pre><code class="idl" data-x="">typedef (<span>CanvasRenderingContext2D</span> or <span>ImageBitmapRenderingContext</span> or <span>WebGLRenderingContext</span>) <dfn>RenderingContext</dfn>;

enum <dfn>CanvasPixelFormat</dfn> {
"8-8-8-8", // default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume you're using this pattern (which looks kinda odd to me, and doesn't match the structure of the other option) instead of something like int8 because we want to be open to things like 10-10-10-2 or whatever in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly.

@foolip
Copy link
Member

foolip commented Oct 30, 2018

There's an Intent to Ship: Canvas Color Management on blink-dev now, pointing back to this PR and #3926. @annevk @domenic, do either of you have the bandwidth to keep reviewing this?

@fserb, I see the "needs tests" label, do the tests in https://wpt.fyi/results/2dcontext/wide-gamut-canvas fully cover the changes in this PR, or are more needed?

@chrisdavidmills
Copy link

Documentation need recorded on https://trello.com/c/N0hJJV95/129-2d-canvas-api

@annevk
Copy link
Member

annevk commented Nov 6, 2018

@foolip to be clear, this PR has a number of unaddressed outstanding comments already. And I believe it also depends on #3926 which isn't moving forward either.

@svgeesus
Copy link

The Chrome intent to ship says

Is this feature fully tested by web-platform-tests?
Yes.

but I couldn't find the tests. A link would be helpful. I'm mainly wanting to read the tests as worked examples to help my review of the spec.

<dd>Decides the backing storage for the <code data-x="dom-context-2d-canvas">canvas</code>'s
context. Support for "8-8-8-8" is mandatory. All other formats are optional. When using "8-8-8-8",
one byte is used for each color channel and 1 byte used for alpha. When using float
formats, values outside of [0, 1] range can be used to represent colors outside of the color

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires the transfer function to be fully specified outside [0,1].

Could I have a pointer to where that is defined?

@foolip
Copy link
Member

foolip commented Dec 3, 2018

@fserb @zakerinasab, can you point to the existing tests in WPT? Presumably they need to be updated at least a little for this change?

@fserb
Copy link
Contributor Author

fserb commented Dec 3, 2018

@foolip: this is tests we have so far:
https://github.com/web-platform-tests/wpt/tree/master/2dcontext/wide-gamut-canvas

@foolip
Copy link
Member

foolip commented Dec 3, 2018

@svgeesus, hope that helps!

@svgeesus
Copy link

@fserb perhaps the questions from @domenic were missed?

@svgeesus
Copy link

@domenic it seems the issues you raised are still unaddressed? @foolip @annevk how should this be moved forward?

@annevk
Copy link
Member

annevk commented Oct 31, 2019

Someone(s) with time would need to define the underlying model and processing model on top, as well as write the relevant tests. We have an API sketch, but none of the remainder, from a quick look and from what I remember when we last went through this.

Base automatically changed from master to main January 15, 2021 07:57
@annevk
Copy link
Member

annevk commented Apr 28, 2021

#6562 lays some groundwork for this. Closing this as it's outdated and needs a lot more work as per the above comments.

@annevk annevk closed this Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs tests Moving the issue forward requires someone to write tests topic: canvas
Development

Successfully merging this pull request may close these issues.

8 participants