feat(ext/web): add ImageData Web API #21183

jamsinclair · 2023-11-13T17:35:02Z

Fixes #19288

Adds the ImageData Web API.

This would be beneficial to projects using ImageData as a convenient transport layer for pixel data. This is common in Web Assembly projects that manipulate images. Having this global available in Deno would improve compatibility of existing JS libraries.

References

CLAassistant · 2023-11-13T17:35:09Z

All committers have signed the CLA.

cli/tsc/dts/lib.deno.window.d.ts

jamsinclair · 2023-11-13T17:39:05Z

tools/wpt/expectation.json

@@ -6908,6 +6908,9 @@
 }
 },
 "embedded-content": {
+ "the-canvas-element": {
+ "imagedata.html": true


This test case does not run with Deno's current wpt set up. However, by hacking the wpt.ts script I got the test running and it appears to pass.

https://github.com/web-platform-tests/wpt/blob/360d99ddc756540791ed1e1e97df51fa950aa38d/html/semantics/embedded-content/the-canvas-element/imagedata.html:

/html/semantics/embedded-content/the-canvas-element/imagedata.html test ImageData(w, h), width cannot be 0 ... ok test ImageData(w, h), height cannot be 0 ... ok test ImageData(w, h), exposed attributes check ... ok test ImageData(buffer, w), the buffer size must be a multiple of 4 ... ok test ImageData(buffer, w), buffer size must be a multiple of the image width ... ok test ImageData(buffer, w, h), buffer.length == 4 * w * h must be true ... ok test ImageData(buffer, w, opt h), Uint8ClampedArray argument type check ... ok test ImageData(buffer, w, opt h), exposed attributes check ... ok file result: ok. 8 passed; 0 failed; 0 expected failure; total 8 (2681ms)

I believe we already have a few hacks in place for WPT. What is specifically failing?

the file name is the issue @mmastrac.

To fix this, an upstream PR to WPT should be made to update the naming convention for this specific testing module.

I believe we already have a few hacks in place for WPT. What is specifically failing?

When running ./tools/wpt.ts run the test case is not discovered and not run by the script.

To fix this, an upstream PR to WPT should be made to update the naming convention for this specific testing module

Is this really the case? I might be misunderstanding the situation, but the test and name seems valid to me. The test is the "With HTML boilerplate" type according to the documentation. It appears that these types of tests have not been used yet by Deno.

Do we need to refactor the discoverTestsToRun to support them?

I've added the commit 28347cf which I believe resolves the issue.

I see, you are correct. in previous cases we have gone upstream to update the HTML tests to js tests where applicable

Ah I see! Well I have no strong feelings, I'll follow whatever your team prefers 🙇

jamsinclair · 2023-11-13T17:43:48Z

ext/web/16_image_data.js

+ sourceHeight = typeof arg1 !== "undefined" ? parseInt(arg1, 10) : undefined;
+ settings = arg2;
+
+ if (Number.isNaN(sourceWidth) || sourceWidth < 1) {


There is some repeated logic between both overloads, but I think the class is simpler to understand with the duplication.

Please let me know there are better or more preferred ways to refactor.

ext/web/16_image_data.js

0f-0b · 2023-11-14T15:45:06Z

ext/web/16_image_data.js

+ let settings;
+
+ // Overload: new ImageData(data, sw [, sh [, settings ] ])
+ if (webidl.type(arg0) === "Object") {


Suggested change

if (webidl.type(arg0) === "Object") {

if (

arguments.length > 3 ||

TypedArrayPrototypeGetSymbolToStringTag(arg0) === "Uint8ClampedArray"

) {

As per the overload resolution algorithm. WPT will fail as expected.

@crowlKats this is the reason that ImageData(buffer, w, opt h), Uint8ClampedArray argument type check has to fail. By obeying the overload resolution algorithm we can't handle this case.

All major browsers will fail the test as well. You can test it yourself by visiting https://wpt.live/html/semantics/embedded-content/the-canvas-element/imagedata.html.

Ah I see, thanks for clarifying. probably at some point someone should fix the test (I'll take a look at some point), because its kinda ridiculous if everyone fails it with intent...

ext/web/16_image_data.js

jamsinclair · 2023-11-22T06:06:59Z

The CI is failing for the Web Platform Tests on the ci / test release ubuntu-x86_64 job.

Test Result Output:

2023-11-21T14:15:29.8055289Z expected test failures that passed:
2023-11-21T14:15:29.8055848Z 
2023-11-21T14:15:29.8057091Z         "/workers/semantics/interface-objects/001.worker.html - The ImageData interface object should be exposed."
2023-11-21T14:15:29.8058431Z 
2023-11-21T14:15:29.8059149Z final result: failed. 2153 passed; 1 failed; 97 expected failure; total 2154 (449030ms)

I'll try look into this over the weekend - ~~likely a simple misconfiguration of the test expectations.~~

Edit: It turns out we were expecting ImageData not to be exposed in the workers/semantics/interface-objects/001.worker.html, we can now remove it as it's implemented in this PR 🎉 .

0f-0b · 2023-11-26T22:53:11Z

I'd like to point out some outstanding problems.

There are subtle differences from browser implementations.
- Conversion of the settings argument should be done before the actual constructor steps, maybe using a dictionary converter (example).
- The access to data.length should be unobservable (use the TypedArrayPrototypeGetLength primordial).
- The check that data.length is a non-zero multiple of 4 should be done before validation of sw and sh.
The differences are illustrated by this code snippet.
```
Object.defineProperty(Uint8ClampedArray.prototype, "length", {
  get() {
    console.log("get data.length");
    return 0;
  },
});

new ImageData(
  new Uint8ClampedArray(0),
  {
    valueOf() {
      console.log("call sw.valueOf");
      return 1;
    },
  },
  {
    valueOf() {
      console.log("call sh.valueOf");
      return 1;
    },
  },
  {
    get colorSpace() {
      console.log("get settings.colorSpace");
      return {
        toString() {
          console.log("call settings.colorSpace.toString");
          return "display-p3";
        },
      };
    },
  },
);
```
In a browser (that supports setting colorSpace), it logs the following messages to the console.
- call sw.valueOf.
- call sh.valueOf.
- get settings.colorSpace.
- call settings.colorSpace.toString.
- An uncaught "InvalidStateError" DOMException.
While in this implementation, it currently prints these messages.
- call sw.valueOf.
- call sh.valueOf.
- get data.length.
- get data.length.
- get data.length.
- An uncaught "IndexSizeError" DOMException.
Type declarations should live in lib.deno_web.d.ts instead of lib.deno.window.d.ts, since ImageData is available in both the main thread and workers.

jamsinclair · 2023-11-27T13:18:07Z

Thanks for the thorough review @0f-0b! I think I've addressed those changes, let me know if I misunderstood anything.

Running the script in your comment with the debug build now outputs the following

call sw.valueOf
call sh.valueOf
get settings.colorSpace
call settings.colorSpace.toString
An uncaught InvalidStateError DOMException.

I also additionally

Added a check for zero data elements
Added a quick test to assert that ImageData works inside a Worker

crowlKats · 2023-12-03T09:13:45Z

tools/wpt/expectation.json

@@ -7009,6 +7009,11 @@
 }
 },
 "embedded-content": {
+ "the-canvas-element": {
+ "imagedata.html": [
+ "ImageData(buffer, w, opt h), Uint8ClampedArray argument type check"


why is this test failing?

Great question, I've tagged you in a comment above that explains why this has to fail – #21183 (comment).

crowlKats

This looks good to me! Thanks a lot for adding this

crowlKats · 2023-12-03T16:51:00Z

@lucacasonato could you take a look at the WPT changes?

lucacasonato · 2023-12-03T17:26:42Z

They look fine

feat(ext/web): add ImageData Web API

66504e5

jamsinclair commented Nov 13, 2023

View reviewed changes

jamsinclair changed the title ~~feat(ext/web): add ImageData Web API~~ [WIP] feat(ext/web): add ImageData Web API Nov 13, 2023

mmastrac reviewed Nov 13, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

crowlKats self-requested a review November 13, 2023 17:54

crowlKats reviewed Nov 13, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

crowlKats reviewed Nov 13, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

crowlKats reviewed Nov 13, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

jamsinclair added 8 commits November 14, 2023 17:59

refactor: use constructor parameters instead of destructuring

4e0f99a

refactor: use webidl branding

216ad5b

refactor: use webidl enum converter for PredefinedColorSpace

263e881

refactor: use webidl.type over typeof

31e7e80

refactor: formatting fixes

1e8d0e2

refactor: ensure constructor has an argument length of 2

9438975

refactor: add ImageData typings to lib.deno.window.d.ts

953231b

fix: allow HTML boilerplate tests to be run by wpt runner

28347cf

jamsinclair marked this pull request as ready for review November 14, 2023 13:25

jamsinclair changed the title ~~[WIP] feat(ext/web): add ImageData Web API~~ feat(ext/web): add ImageData Web API Nov 14, 2023

refactor: fix typos and grammar

5615acd

crowlKats reviewed Nov 14, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

crowlKats reviewed Nov 14, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

crowlKats reviewed Nov 14, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

0f-0b reviewed Nov 14, 2023

View reviewed changes

jamsinclair added 5 commits November 15, 2023 00:54

refactor: use webidl and primordial solutions instead

22c0bb6

refactor: use Uint8ClampedArray primordial

3ea3ab2

refactor: add expected wpt test failure

2aefa52

refactor: use webidl unsigned long converter

0c625b4

refactor: remove unnecessary NumberIsNaN

9125d25

crowlKats reviewed Nov 14, 2023

View reviewed changes

ext/web/16_image_data.js Outdated Show resolved Hide resolved

ext/web/16_image_data.js Outdated Show resolved Hide resolved

ext/web/16_image_data.js Outdated Show resolved Hide resolved

refactor: use if/else for readability

1aad2cb

petamoriken reviewed Nov 15, 2023

View reviewed changes

ext/web/16_image_data.js Show resolved Hide resolved

jamsinclair and others added 4 commits November 16, 2023 20:37

refactor: add Deno.privateCustomInspect method

159e5bd

refactor: use ImageDataPrototype var

6978412

refactor: pass inspectOptions

5374106

Merge branch 'main' into feat/add-image-data-api

8803987

test: remove expected failure for ImageData interface in worker test

10182a7

jamsinclair added 7 commits November 27, 2023 22:00

refactor: move ImageData typings to lib.deno_web.d.ts

d7d8380

refactor: use unobservable length check

a81b871

refactor: move image data length mutiple of 4 check to earlier position

30d17f3

refactor: add missing zero elements check

3894813

refactor: use ImageDataSettings dictionary converter

66be9cd

test: add additional test for ImageData usage inside a worker

8dcee10

chore: formatting fixes

0b97d3c

bartlomieju requested a review from crowlKats December 3, 2023 00:05

crowlKats reviewed Dec 3, 2023

View reviewed changes

crowlKats approved these changes Dec 3, 2023

View reviewed changes

crowlKats merged commit 8c0fb90 into denoland:main Dec 6, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ext/web): add ImageData Web API #21183

feat(ext/web): add ImageData Web API #21183

jamsinclair commented Nov 13, 2023 •

edited

Loading

CLAassistant commented Nov 13, 2023 •

edited

Loading

jamsinclair Nov 13, 2023

mmastrac Nov 13, 2023 •

edited

Loading

crowlKats Nov 13, 2023

jamsinclair Nov 14, 2023 •

edited

Loading

jamsinclair Nov 14, 2023

crowlKats Nov 14, 2023

jamsinclair Nov 14, 2023

jamsinclair Nov 13, 2023

0f-0b Nov 14, 2023

jamsinclair Dec 3, 2023

crowlKats Dec 3, 2023

jamsinclair commented Nov 22, 2023 •

edited

Loading

0f-0b commented Nov 26, 2023

jamsinclair commented Nov 27, 2023 •

edited

Loading

crowlKats Dec 3, 2023

jamsinclair Dec 3, 2023

crowlKats left a comment

crowlKats commented Dec 3, 2023

lucacasonato commented Dec 3, 2023

- if (webidl.type(arg0) === "Object") {
+ if (
+ arguments.length > 3 ||
+ TypedArrayPrototypeGetSymbolToStringTag(arg0) === "Uint8ClampedArray"
+ ) {

feat(ext/web): add ImageData Web API #21183

feat(ext/web): add ImageData Web API #21183

Conversation

jamsinclair commented Nov 13, 2023 • edited Loading

CLAassistant commented Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

mmastrac Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamsinclair Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamsinclair commented Nov 22, 2023 • edited Loading

0f-0b commented Nov 26, 2023

jamsinclair commented Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crowlKats left a comment

Choose a reason for hiding this comment

crowlKats commented Dec 3, 2023

lucacasonato commented Dec 3, 2023

jamsinclair commented Nov 13, 2023 •

edited

Loading

CLAassistant commented Nov 13, 2023 •

edited

Loading

mmastrac Nov 13, 2023 •

edited

Loading

jamsinclair Nov 14, 2023 •

edited

Loading

jamsinclair commented Nov 22, 2023 •

edited

Loading

jamsinclair commented Nov 27, 2023 •

edited

Loading