GetImageData for OffscreenCanvas without an associated canvas element returns blank pixels

[jdm]

Code:

• servo/components/script/dom/canvasrenderingcontext2d.rs

  Lines 281 to 284 in 4fe8238

  	// FIXME(nox): This is probably wrong when this is a context for an
  	// offscreen canvas.
  	let canvas_size = canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size());
  	assert!(Rect::from_size(canvas_size).contains_rect(&rect));

• servo/components/script/dom/canvasrenderingcontext2d.rs

  Lines 281 to 284 in 4fe8238

  	// FIXME(nox): This is probably wrong when this is a context for an
  	// offscreen canvas.
  	let canvas_size = canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size());
  	assert!(Rect::from_size(canvas_size).contains_rect(&rect));

• servo/components/script/dom/canvasrenderingcontext2d.rs

  Lines 1056 to 1058 in 4fe8238

  	// FIXME(nox): This is probably wrong when this is a context for an
  	// offscreen canvas.
  	let canvas_size = canvas.as_ref().map_or(Size2D::zero(), |c| c.get_size());

In cases like OffscreenCanvasRenderingContext2D::GetImageData when there is no associated canvas element (eg. https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/offscreen-canvas/drawing-rectangles-to-the-canvas/2d.clearRect.clip.html), we need to provide a real canvas size. I propose we replace the optional canvas element argument with a canvas size argument instead, and modify the callers to pass it in as appropriate.

We should get a bunch of tests with better results when running ./mach test-wpt tests/wpt/web-platform-tests/offscreen-canvas.

[rasviitanen]

@highfive: assign me

[highfive]

Hey @rasviitanen! Thanks for your interest in working on this issue. It's now assigned to you!

[bblanke]

Hi! I'm with an NC State group that's been assigned to work on this as a part of a school project. @rasviitanen, have you made progress on this? How can I help?

[rasviitanen]

@bblanke Hey fellow student. I have had some major hardware issues, which has been hindering me from resolving this issue and running the tests properly. I have however, made some progress on this, and created an implementation that for the most part resolves this issue.
The problem I am facing now, is that some tests regarding reseting the canvas is failing, that are supposed to pass (they are marked as pass in the test metadata). I believe that this can be an effect of someone marking the test as "pass" mistakenly, and that the failing tests is a result of something else other than my implementation.

You could help me investigate this issue. I'll to upload the patch shortly and give a more detailed explanation. I'll make sure to let you know once I upload it.

[bblanke]

Awesome! I'm excited to help

[jdm]

@rasviitanen Ah, so the reason the test is "passing" right now is that the existing implementation to compare pixel values relies on GetImageData. If the passing condition for the test is transparent black pixels, the incorrect GetImageData implementation will make the test look like it's passing because the expected pixel data is found. When the bug is fixed, it exposes that the existing canvas implementation that's actually being tested is faulty.

[bblanke]

Which tests are you all using to validate this? When I look at the metadata in tests/wpt/metadata/offscreen-canvas all of the tests are set to FAIL

[jdm]

The absence of an ini file for https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/offscreen-canvas/the-offscreen-canvas/initial.reset.path.html means the test hardness defaults to expecting it to pass.

[rasviitanen]

@bblanke sorry for it taking time, you can now see my pull-request #24464

[bblanke]

Thank you! I'm looking through it now; nice to have a good reference to learn from.