Adds convertToBlob to Canvas #3926

fserb · 2018-08-15T19:12:30Z

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

This moves the convertToBlob function from offscreenCanvas into 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

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

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 · 2018-08-15T19:12:44Z

@whatwg/canvas (In case the original didn't work)

annevk · 2018-08-16T06:29:47Z

Couple of comments:

Since HTMLCanvasElement comes first, we should introduce ImageEncodeOptions there.
I would introduce a new subsection (inc. heading) for this method. Perhaps as a previous sibling to "The OffscreenCanvas interface" (basically where you have it now, but with a heading)?
You cannot talk about the Canvas object since there's no such thing. In the non-normative description you can refer to canvas since you already introduced that variable. In the normative description I'd stick to "this object".

Hope that helps.

fserb · 2018-08-21T18:53:28Z

I did the ImageEncodeOptions change on the other PR. :/ Should I do it here too?

annevk · 2018-08-22T09:58:47Z

You have to pick an order and build the PRs on each other, I think. Either we extend ImageEncodeOptions first and then move it around for convertToBlob() or the other way around. The current setup doesn't lead to logical commits on master.

annevk · 2018-08-22T09:59:20Z

(The other PR is #3882 by the way, for those wondering.)

domenic · 2018-08-23T17:20:55Z

@annevk's comments generally make sense. Another point to watch out for is how you've changed things compared to the existing algorithm at https://html.spec.whatwg.org/#dom-offscreencanvas-converttoblob. Are all of these changes intentional?

The method now works even on detached offscreen canvases.
The method no longer creates a copy of the bitmap before going in parallel. That seems very bad and likely to lead to race conditions.

fserb · 2018-10-09T18:32:04Z

Addressed @annevk's original comment and reverted the unintended issues pointed by @domenic.

foolip · 2018-10-30T15:40:41Z

There's an Intent to Ship: Canvas Color Management on blink-dev now, pointing back to this PR and #3882. @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?

foolip · 2018-10-30T15:41:58Z

I'll answer my own question about tests, looks like the only tests involving convertToBlob are for offscreen canvas, so more needed. @fserb, are you up for writing a test PR matching these spec changes?

chrisdavidmills · 2018-10-31T09:20:50Z

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

annevk · 2018-10-31T10:59:24Z

source

+dictionary <dfn>ImageEncodeOptions</dfn> {
+  DOMString <span data-x="image-encode-options-type">type</span> = "image/png";
+  unrestricted double <span data-x="image-encode-options-quality">quality</span> = 1.0;
+  <span>ImageEncodePixelFormat</span> <span data-x="image-encode-options-pixelformat">pixelFormat</span> = "uint8";


This option isn't really explained in sufficient detail. Also, I thought we'd postpone adding it to a subsequent PR and only deal with convertToBlob() here?

Okey, I'll leave just the convertToBlob part.

annevk · 2018-10-31T11:02:42Z

@foolip I'm not sure we have other reviewers available? I think the main problem I have at times is the lag between reviews and updates and that probably goes both ways. Not sure what we can do to make that better. Perhaps at times synchronously communicate?

foolip · 2018-10-31T13:50:52Z

@annevk, that was my way of asking if I should spend time reviewing this, without obviously volunteering :)

@fserb is in UTC-5, so timezones seem OK for all involved.

fserb · 2018-11-01T15:43:29Z

@foolip, we do have canvas convertToBlob tests too at 2dcontext/wide-gamut-canvas/canvas-colorManaged-convertToBlob-roundtrip.html. Is that what you were talking about?

foolip · 2018-11-01T16:02:02Z

Oh, there it is. I must have made a typo when grepping.

fserb · 2018-11-06T15:04:10Z

Trying to make those PRs smaller. This one now just moves convertToBlob to also include Canvas.

annevk

Are there tests for this?

The one other problem here that was already a problem is that we don't parse the MIME type argument and we don't state how we set the MIME type for the resulting Blob object. It would be good to make that use the MIME type infrastructure of the web platform and be clear if any unknown parameters get passed through, result in rejection, etc.

source

annevk · 2018-11-06T15:15:32Z

I also wonder if we should preserve the old ID for convertToBlob(). I guess it might be okay given that OffscreenCanvas is still relatively new.

fserb · 2018-11-07T16:08:41Z

Done all. PTAL.

annevk · 2018-11-08T14:19:28Z

(You also didn't reply to a bunch of the non-code questions/comments?)

fserb · 2018-11-09T18:28:05Z

Regarding the WPT tests, we have a canvas-colorManaged-convertToBlob-roundtrip.html But we could probably add more tests.

I agree that given that convertToBlob() is kinda new, we can stick to the new ID.

I'll work on additional resolution steps for the MIME type.

fserb · 2018-11-09T20:45:36Z

@annevk is that what you had in mind?

Put convertToBlob/toDataURL/toBlob together in a "Serializing canvas bitmaps" section, and merged that with the former "Serializing bitmaps to a file" section

Can't link to specific concepts since they're polymorphic

domenic · 2018-11-09T21:24:56Z

I'm pushing some last fixes. That will include reverting the MIME type work because I think it's too involved to get in to now. In particular we need to treat the three conversion functions the same way, which we currently are not. It's better to leave it as-is.

domenic

With my latest pushes this LGTM. @annevk for a final look?

Are there other implementers from @whatwg/canvas interested in allowing convertToBlob() on <canvas> elements, not just OffscreenCanvases?

annevk

The reordering makes this hard to review, but I guess it's okay? And yeah, it seems like what's still lacking is impl interest and tests. A follow-up issue for MIME types might be good too.

source

foolip · 2018-12-05T09:51:39Z

@annevk @domenic is this good to merge now?

foolip · 2018-12-05T09:52:27Z

Scratch that question, the answer is: "seems like what's still lacking is impl interest and tests"

@fserb, will you be adding more tests for this?

foolip · 2018-12-05T09:53:45Z

On impl interest, @smfr commented in #4167 (comment) with support.

annevk · 2018-12-05T10:58:55Z

Right, I understand there's interest from Google, but we need two implementers per https://whatwg.org/working-mode#changes.

foolip · 2018-12-05T12:04:21Z

@smfr doesn't work for Google :)

annevk · 2018-12-10T18:39:31Z

That's true, though that comment wasn't very specific (seemed to be mostly about the float16 backing store). In any event, we also seem to be waiting on tests as you noted earlier.

annevk · 2021-04-28T14:00:13Z

What's the status here? Has this been implemented/shipped/abandoned?

fserb · 2021-04-28T14:23:49Z

This has been implemented on Chrome but not shipped.

It's one of those things that people will need once other color management feature gets traction (as will there be no way to export a canvas with a specified color space). Which I think is why @foolip suggested that support for color management kinda implies support for this.

It's also a better API, and already implemented for OffscreenCanvas. Could we get a +1 from other impl?

annevk · 2021-04-28T14:25:11Z

Let's ping @whatwg/canvas once more.

fserb force-pushed the colormgmt-convertToBlob branch from c71458e to 1ac158a Compare October 9, 2018 17:21

foolip mentioned this pull request Oct 30, 2018

Adds wide-gamut support #3882

Closed

fserb mentioned this pull request Oct 30, 2018

Wide gamut support for Canvas/OffscreenCanvas/ImageBitmap w3ctag/design-reviews#315

Closed

5 tasks

annevk reviewed Oct 31, 2018

View reviewed changes

fserb added 4 commits November 6, 2018 09:44

convertToBlob

edf3818

header

8463a67

added format and address canvas object

4a94d8d

Fixed bitmap

d33ff1b

fserb force-pushed the colormgmt-convertToBlob branch from d8ead47 to d33ff1b Compare November 6, 2018 14:44

Removed pixelFormat from this

bc01ebc

annevk reviewed Nov 6, 2018

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

annevk review

2d86f11

move convertToBlob up

a885986

Added MIME type parsing

676b6f3

domenic added 2 commits November 9, 2018 16:15

Rearrange sections

9a1b83c

Put convertToBlob/toDataURL/toBlob together in a "Serializing canvas bitmaps" section, and merged that with the former "Serializing bitmaps to a file" section

Make these steps more generic

f622fd6

Can't link to specific concepts since they're polymorphic

Don't fix MIME types for now

11f5d6f

domenic approved these changes Nov 9, 2018

View reviewed changes

domenic mentioned this pull request Nov 9, 2018

Give toDataURL() and toBlob() default parameter values #3477

Merged

annevk approved these changes Nov 13, 2018

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

Add commas

c49e006

svgeesus mentioned this pull request Oct 31, 2019

[css-color-4] extend rgb() for HDR w3c/csswg-drafts#3249

Closed

svgeesus mentioned this pull request Dec 9, 2019

Is this still active? WICG/canvas-color-space#26

Closed

Base automatically changed from master to main January 15, 2021 07:57

domenic force-pushed the main branch from 0947339 to 1b5099f Compare July 27, 2021 17:07

Adds convertToBlob to Canvas #3926

Are you sure you want to change the base?

Adds convertToBlob to Canvas #3926

Conversation

fserb commented Aug 15, 2018 • edited by pr-preview bot Loading

💥 Error: Wattsi server error 💥

fserb commented Aug 15, 2018

annevk commented Aug 16, 2018

fserb commented Aug 21, 2018

annevk commented Aug 22, 2018

annevk commented Aug 22, 2018

domenic commented Aug 23, 2018

fserb commented Oct 9, 2018

foolip commented Oct 30, 2018

foolip commented Oct 30, 2018

chrisdavidmills commented Oct 31, 2018

annevk Oct 31, 2018

Choose a reason for hiding this comment

fserb Nov 6, 2018

Choose a reason for hiding this comment

annevk commented Oct 31, 2018

foolip commented Oct 31, 2018

fserb commented Nov 1, 2018

foolip commented Nov 1, 2018

fserb commented Nov 6, 2018

annevk left a comment

Choose a reason for hiding this comment

annevk commented Nov 6, 2018

fserb commented Nov 7, 2018

annevk commented Nov 8, 2018

fserb commented Nov 9, 2018

fserb commented Nov 9, 2018

domenic commented Nov 9, 2018

domenic left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

foolip commented Dec 5, 2018

foolip commented Dec 5, 2018

foolip commented Dec 5, 2018

annevk commented Dec 5, 2018

foolip commented Dec 5, 2018

annevk commented Dec 10, 2018

annevk commented Apr 28, 2021

fserb commented Apr 28, 2021

annevk commented Apr 28, 2021

fserb commented Aug 15, 2018 •

edited by pr-preview bot

Loading