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

Rendering of "bitmaprenderer"-backed canvas #7833

Open
annevk opened this issue Apr 19, 2022 · 25 comments
Open

Rendering of "bitmaprenderer"-backed canvas #7833

annevk opened this issue Apr 19, 2022 · 25 comments
Labels
interop Implementations are not interoperable with each other topic: canvas

Comments

@annevk
Copy link
Member

annevk commented Apr 19, 2022

Consider this example: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/10227 (HTML reproduced below).

According to the specification the canvas's intrinsic dimensions should end up being those of the image and those should also be used for rendering it as a replaced element. However, the canvas is still sized according to its width and height attributes (or the replaced element defaults if those are absent).

In Chrome/Safari the image appears scaled and in Firefox it's cropped. It's not possible to determine the size of the context I think as that's not exposed. So you cannot reliably tell if the context is actually larger than the canvas or not.

What was the intent here?

(I personally think the Firefox behavior is the most logical behavior here and consistent with how width and height are supposed to function, but it doesn't really match the current specification. However, neither does the Chrome/Safari behavior.)

(This came up in a long discussion over at gpuweb/gpuweb#2416.)

cc @whatwg/canvas


<!DOCTYPE html>
<script>
function start(img) {
  createImageBitmap(img).then(bmp => {
    c = document.querySelector("canvas");
    ctx = c.getContext("bitmaprenderer");

    ctx.transferFromImageBitmap(bmp)
  })
}
</script>
<p><img src=image onload=start(this)>
<p><canvas width=50 height=50></canvas>
@kenrussell
Copy link
Member

Thinking back to discussions from several years ago, and reviewing the specification at https://html.spec.whatwg.org/multipage/canvas.html#the-imagebitmap-rendering-context :

The intent of ImageBitmapRenderingContext was to provide the most efficient way possible to display ImageBitmaps on a page. The primary use case was for rendering on a worker via OffscreenCanvas, transferring those ImageBitmaps back to the main thread, and synchronizing their display with other main-thread rendering.

Per the ImageBitmapRenderingContext specification, transferFromImageBitmap replaces the canvas's contents with the bitmap. The intent was not to force a re-layout at that point, which would happen if the canvas's width or height were changed in response. Instead, the canvas's previous width and height were intended to be then treated as CSS style properties; or, if the CSS width/height had previously been set, those would still be obeyed. Setting the CSS width and height of a canvas is the standard approach for rendering into a canvas at a different resolution than it's displayed on the page.

Given these assumptions I think that Safari's and Chrome's behavior on this test case is correct. It's what would happen if a canvas with a 2D context were resized to the size of the ImageBitmap while using CSS width/height to fit it into its original space on the page, and drawImage were called. Perhaps the ImageBitmapRenderingContext specification should be clarified.

While ImageBitmapRenderingContext doesn't provide a way to query the size of the context, ImageBitmap does. The expectation is that the application would query the ImageBitmap and react to its size if necessary before transferring it into the ImageBitmapRenderingContext.

@Kaiido
Copy link
Member

Kaiido commented Apr 20, 2022

While ImageBitmapRenderingContext doesn't provide a way to query the size of the context [...]

Just a note that creating a new ImageBitmap from the BitmapRenderer's canvas does "apparently" expose the context's size, or at least it exposes the discrepancy between Firefox's behavior and the others: https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=10233

FWIW, I too think Safari and Chrome's behavior is the least surprising here. This way it behaves like an <img>1, which it kind of replaces for a lack of <img>.srcObject able to consume ImageBitmaps.

Footnotes

  1. Except that no UA supports setting object-fit on the canvas...

@annevk
Copy link
Member Author

annevk commented Apr 20, 2022

Thank you for providing that context. I think it's extremely weird that the width and height attributes suddenly become presentational. That goes counter to how they were designed. Note that in the example in OP no CSS style properties have been set.

@kenrussell
Copy link
Member

It was crucial in the design that transferring of an ImageBitmap into the rendering context not be a layout-inducing event, which is why width and height were considered snapshotted into the presentational attributes upon the first transferFromImageBitmap. Additionally, the aim was to replace the canvas's backing store in its entirety with the incoming ImageBitmap. @Kaiido's observation that this behaves like placing the ImageBitmap into an <img> (with a previously specified width and height) was the goal.

Apologies for any weirdness in specification we designers of this API induced. I hope that we can preserve the behavior Safari and Chrome implement - I think it's the preferred semantic from the developer's point of view.

@annevk
Copy link
Member Author

annevk commented Apr 20, 2022

Well, at the moment the weirdness is only in implementations as the specification does require layout changes to occur. Changes to the Rendering section would have to be made to account for this somehow as far as I can tell.

Semantically that also leaves us in a pickle as the intent is for these attributes to not be presentational. I suppose we could try to forbid them on canvas elements that are meant for "bitmaprenderer" contexts, but that's extremely awkward and not something you can realistically enforce in practice.

I'm curious though what others think as at least to me Firefox's behavior is much more consistent with how canvas was supposed to work.

@kainino0x
Copy link

In Chrome/Safari the image appears scaled and in Firefox it's cropped.

Aside: I figured out why I didn't discover this difference when I tested the behavior: my ImageBitmap was smaller that my target canvas. In this case, Chrome/Firefox/Safari all have the same behavior (they zoom in, not "letterbox"). The behavior differs when the ImageBitmap is larger than the target canvas (zoom out vs crop).

@junov
Copy link
Member

junov commented May 5, 2022

FWIW, the object-fit CSS property does work with canvases, but in current implementations, it seems to not intervene in the resizing that maps the bitmap to the size defined by the width and height HTML attributes. The object-fit comes into play when mapping from the intrinsic size to the CSS size.

Example: https://jsfiddle.net/651yzwrc/

I actually think the spec'ed behaviour (which no browsers have implemented) is the sanest because:

  1. It avoids confusion around the concept of "intrinsic size" (i.e. having a unified and simple definition that applies to all context types)
  2. It avoids confusion around the behaviors of CSS properties that affect image rendering (e.g. object-fit, filter, image-rendering). Having a bitmap-to-intrinsic transform, followed by an intrinsic-to-CSS transform results in a two-stage transform, with CSS only intervening on the second stage, which seems confusing and surprising IMHO. The spec'ed behavior avoids this complexity.
  3. It is consistent with OffscrenCanvas commit behavior, where the intrinsic size of the placeholder canvas is determined by the size of the bitmap committed by the OffscreenCanvas.

As Ken pointed out, this has the disadvantage of that calling transferFromImageBitmap may invalidate the page's layout. Is that really a dealbreaker?

@kenrussell
Copy link
Member

As Ken pointed out, this has the disadvantage of that calling transferFromImageBitmap may invalidate the page's layout. Is that really a dealbreaker?

I think it is unless there's an easy application-level workaround to make ImageBitmapRenderingContext.transferFromImageBitmap a guaranteed-cheap operation. Again, the design intent of ImageBitmapRenderingContext was to provide the lightest-weight, application-controlled mechanism possible for getting OffscreenCanvas-rendered frames onto the screen on the main thread.

If the CSS width and height of the destination canvas are explicitly set by the application (for example, to the canvas's width and height), then is transferFromImageBitmap guaranteed to not invalidate layout?

@Kaiido
Copy link
Member

Kaiido commented May 6, 2022

It is consistent with OffscrenCanvas commit behavior, where the intrinsic size of the placeholder canvas is determined by the size of the bitmap committed by the OffscreenCanvas.

FWIW I personally find this behavior quite confusing, at least since .commit() has effectively been replaced by requestAnimationFrame, because this will actually change the size of the placeholder canvas at the next paint refresh, not synchronously. https://jsfiddle.net/Lkjtyrn9/
I'm not entirely sure if bitmap-renderer would update the canvas size synchronously but in that case that would make it not so consistent with OffscreenCanvas.

Also see #7798. OffscreenCanvas actually has a special step where setting the placeholder canvas width and height would throw, Bitmap-renderer doesn't have this step so it's unclear how that should work there, and it's not clear if this special handling is actually a good thing or not.


As an aside it might be worth noting that Safari and Chrome behaviors do differ (as seen in Justin's fiddle). Safari will apparently change the size of the canvas when the ImageBitmap is transferred there before the canvas has been rendered: https://jsfiddle.net/1tuvsk96/
(It does also report ctx.canvas.width to be the intrinsic size of the bitmap, while Chrome reports the attribute value).

@kdashg
Copy link

kdashg commented Aug 11, 2022

I'm happy that canvas's intrinsic width and height become the imagebitmap's width/height, as if we were setting img.src and relying on intrinsic width and height for layout.
Authors who don't want layout should set css width and height, like they must to prevent other canvas resizes from affecting layout.
It sounds like this resizing is not happening though?

It's also good that canvas.width/height are still useful to be able to size an ImageBitmapRenderingContext "placeholder" "bitmap" for the purposes of intrinsic width/height.

I think our two options are:

  1. Have an IBRC.bitmapWidth/Height a la WebGLRenderingContext.drawingBufferWidth/Height, that is got and set by transferTo/FromImageBitmap, or
  2. transferFromImageBitmap should set canvas.width,height to ImageBitmap.width/height.

@greggman
Copy link

greggman commented Aug 25, 2022

I'm happy that canvas's intrinsic width and height become the imagebitmap's width/height, as if we were setting img.src and relying on intrinsic width and height for layout.

I agree. Setting an img.src also changes the width and height (if not set) asychronously. And even if you set img.width, img.height those have no effect on the resolution of the image. In other words

<img src="10x10.png" width="5" height="7">

Will give you a 10x10 pixel backing store for the image, scaled to 5x7 CSS pixels.

After this img.width = 5, img.height = 7, img.naturalWidth = 10, img.naturalHeight = 10

That's how I'd expect bitmaprenderer to work if I never read any docs. And I'd expect if you want the actual size of the bitmap in the bitmap rendered you'd have to get that size some other way (like naturalWidth, naturalHeight on images)

You can't currently get it from ImageBitmap apparently as that says "CSS Pixels" (at least on MDN, that sounds like an error?)

Could add width,height to ImageBitmapRenderingContext similar to WebGLRenderingContext drawingBufferWidth, drawingBufferHeight

ImageBitmap.width and ImageBitmap.height is where you'd get actual resolution

@annevk
Copy link
Member Author

annevk commented Nov 18, 2022

I discussed this with @kkinnunen-apple. There's three (four) sizes in play here:

  • CSS size (as well as display size, but that's a multiple)
  • canvas size
  • backing size

The design for canvas element initially assumed that canvas size and backing size would not always be 1:1, but later we went back on that and for 2d they are now always 1:1.

WebGL however does allow for the backing size to be smaller, although this isn't universally implemented so it's not quite enshrined just yet. And it's not defined what happens when the canvas element is a source of input. E.g., if you use drawImage(), texImage2d(), new VideoFrame(), toDataURL(), etc. and pass the canvas element, do you use the canvas size or backing size? Furthermore, if all you have is a canvas element it would be reasonable to assume that the width and height IDL attributes tell you what kind of result to inspect, but whether that's correct... (Consider a placeholder canvas element for instance or different parts of code controlled by different people.)

Now bitmaprenderer has taken the WebGL approach and has the same problems. In practice it seems that at least some implementations use the backing size when the canvas element is used as a source of input. The big question here is whether we can make changes still or whether there is too much deployed content for that to be reasonable.

I see these options, first two inspired by @kdashg:

  1. We expose the backing size directly on the canvas element through two new readonly getters. We define exactly which of the canvas size or backing size is used in all the places that consume a canvas element. This doesn't impact layout for the canvas element, but it does have significant downstream impact in that the canvas size is no longer the authoritative size.
    1. If all the places that consume a canvas element use the canvas size (and thus we end up scaling the image) we might not have to expose the backing size explicitly. This also doesn't impact layout and it wouldn't really impact downstream if we defined the resizing to happen upon transferFromImageBitmap().
  2. We resize the canvas element upon transferFromImageBitmap(). This does impact layout, but it keeps canvas size and backing size 1:1 and has no downstream impact.
  3. transferFromImageBitmap() throws if the dimensions do not match. Same as 2, but strict.
  4. We resize the canvas element upon transferFromImageBitmap() and if it's the first time the method was called we store the initial width height as internal presentational hints so it keeps the current layout size... This doesn't impact layout and it doesn't impact downstream, but it's odd.

2/3 have my personal preference. 1 is probably the most compatible, but it will be some work defining things properly and going through the long list of canvas-element consuming endpoints and ensuring they do the right thing.

@greggman
Copy link

greggman commented Nov 20, 2022

I'd pick 1 because seems more consistent with the rest of the platform.

Setting an <img>'s src changes its backing size

const img = new Image();
img.src = "200x100.png"

img width and height are now 200x100

img.width = 400
img.height = 200

image is now displayed at 400x200 but its "backingStore" is still 200x100

img.src = "300x150.png"

image width and height are still 400x200 but it's backing store has changed to 300x150

example

image has naturalWidth and naturalHeight to find it's actual backingstore size. It's the size that used when you use the image as a source

const img = new Image();
img.src = "200x100.png"
img.width = 22
img.height = 11

const ctx = document.querySelector('someCanvas').getContext('2d')
ctx.drawImage(img, 0, 0);

The image drawn will be 200x100, not 22x11

example

Image (and Video) both show width and height are mostly detached from both the display size and the backingstore size.

if all you have is a canvas element it would be reasonable to assume that the width and height IDL attributes tell you what kind of result to inspect.

This is an incorrect assumption. The image width and height IDL attributes make this clear it is an incorrect assumption.

Implementations should always use the backing size. Implementations not using the backingsize should arguably be fixed to use the backing size. This would make them match the image tag and therefore do the expected thing. Having the canvas behave differently than the video and image tag seems more confusing to me.

@greggman
Copy link

I'm sure you already know this but, to be a little pedantic because many often get mislead

There's three (four) sizes in play here:

CSS size (as well as display size, but that's a multiple)

It's not quite true that display size is a multiple of CSS size. An example of when this is not true. devicePixeRatio = 1. container/window width = 99px. 2 divs in a flex box each set to be 50% width. Their css size will be 49.5px but at display time one will be 50px and the other 49px. devicePixelContentBoxSize was added to ResizeObserver to let the user find out which one got which size.

@annevk
Copy link
Member Author

annevk commented Nov 21, 2022

I don't think comparing canvas with img works. The width and height attributes of the img element have always set the layout size. That hasn't been the case with canvas at all and it was an explicit design decision to not do that.

@greggman
Copy link

greggman commented Nov 21, 2022

I don't think comparing canvas with img works. The width and height attributes of the img element have always set the layout size. That hasn't been the case with canvas at all and it was an explicit design decision to not do that.

I'm not sure I understand. Setting the width and height of both an image, and a canvas, change their layout size unless the there is CSS overriding them.

To me, an image, a video, and a canvas are the same. They are all rectangles of pixels. Their only difference is the source of the pixels. An image's source is an image file, a videos source is a video file/stream, a canvas's source is JavaScript via one of many context. They should all behave the same.

@junov
Copy link
Member

junov commented Nov 22, 2022

I think having a canvas size and a backing size that are independent from each other adds unnecessary complexity to an already complex data model. Are there use cases that absolutely require HTMLCanvasElement.width/height to be settable when using IBRC? What if, when a canvas's context is an IBRC, the width/height attribute setters threw an InvalidStateError (or did nothing), effectively making the attributes read-only? basically the canvas width/height would reflect the backing size. If authors don't want the backing size to affect layout, or if they want the canvas to have a layout size prior to transferring in the first ImageBitmap, then they can just set the CSS size to something other than "auto".

The only behavior I can think of that is not covered is: what happens when we read the canvas before it receives its first image bitmap? For example, if we upload it to a WebGL texture or call toDataURL or toBlob. These case should generate blank images. But how does one control the size of that blank image? If we think this use case matters, then perhaps width/height are read-only only when the IBRC has a backing, otherwise they are settable and control the dimensions of the default (blank) image.

@Kaiido
Copy link
Member

Kaiido commented Nov 23, 2022

@junov there is still the setAttribute() problem, as outlined in #7798

@annevk
Copy link
Member Author

annevk commented Nov 23, 2022

I think we could essentially ignore the width and height content attributes and have the IDL attributes always return the backing size (and either throw or return early for their setters), but it's somewhat weird that such behavior is context-specific. (And yeah, I guess before the backing size is set it should probably use the canvas size including the 150x300 default to keep everything working as intended.)

However, as @Kaiido points out we already have one context that does something like this, so maybe it's not too bad. (Although we should fix the placeholder context to do something similar. Whereby we ignore the content attributes rather than throw when they are set. Perhaps as we do this we can fix both bitmaprenderer and placeholder together.)

If Chromium is willing to champion that approach I think WebKit would be okay following, but there's probably a fair amount of risk and @kkinnunen-apple was worried that it would make certain optimizations harder as you could no longer be as positive that the layout size would not change upon changing the bitmap.

@junov
Copy link
Member

junov commented Nov 23, 2022

@kkinnunen-apple was worried that it would make certain optimizations harder as you could no longer be as positive that the layout size would not change upon changing the bitmap.

In transferFromImageBitmap, one could have something like this to only recompute layout when necessary:

if ((new_width != old_width && style.width == kAuto) || (new_height != old_height && style.height == kAuto)) {
  InvalidateLayout();
}

Or am I missing something more subtle that could cause layout changes in transferFromImageBitmap?

The main counterargument I see for making transferFromImageBitmap never affect layout would be if we are trying to coerce authors into avoiding re-layout by forcing them to explicitly change the canvas size when that is the desired behaviour.

@annevk
Copy link
Member Author

annevk commented Nov 23, 2022

@junov it gets more complicated with OffscreenCanvas as the placeholder canvas will essentially have to communicate back whether its layout is managed correctly (i.e., via CSS) or not. (To be clear, I'm not trying to argue against your proposal by the way as I think it's as valid as the others I outlined. In the end my main worry here is compat. Just trying to provide some additional information so we can make the best decision.)

@junov
Copy link
Member

junov commented Nov 23, 2022

the placeholder canvas will essentially have to communicate back whether its layout is managed correctly

I don't think the placeholder has to communicate anything back to the OffscreenCanvas. The OffscreenCanvas that produced the frame does not need to know anything about layout: its width and height attributes are always writeable and control the size of the backing, just like a regular canvas with a regular rendering context.

However, it is a bit awkward that the placeholder receives frames asynchronously via regular event loop tasks. Perhaps this could be improved in the event loop processing model. We could have a special task queue just for this that is flushed somewhere predictable, like in the Update the Rendering stage of the event loop. This would not eliminate re-layout, but it would make it saner by synchronizing it with rendering updates, and could potentially eliminate out-of-band layout calculations for some use cases.

@kdashg
Copy link

kdashg commented Sep 13, 2023

We kept discussing this in the WebGL WG. (last discussion 2023-01-12)
I believe the consensus was to try to make the behavior roughly equivalent in effect to:

dstRc.transferFromImageBitmap#1(src) {
  dstRc.canvas.width = src.width;
  dstRc.canvas.height = src.height;
  dstRc.[drawImage](src);
  src.close();
}

For canvases today, width and height are presentational (and can incur relayout) unless overridden by CSS. If you want to prevent relayout, just override with CSS like normal for canvases.

The main alternative is to bring in webgl's concept of readonly drawingBufferWidth,Height, a la:

dstRc.transferFromImageBitmap#2(src) {
  dstRc.drawingBufferWidth = src.width;
  dstRc.drawingBufferHeight = src.height;
  dstRc.[drawImage](src);
  src.close();
}

@annevk
Copy link
Member Author

annevk commented Sep 18, 2023

To be clear, they're not presentational. They establish the size of the rendering context. That's how they were designed. (WebGL does something murky, but that's more of a flaw in WebGL.)

Resizing seems like an acceptable solution though. The only risk there is compatibility. Did the WebGL group discuss how to make this change and who would write tests and such?

@kdashg
Copy link

kdashg commented Sep 18, 2023

(Sure, technically e.g. canvas.width isn't directly presentational, but it changes the natural size of the element in the situation where no other styling applies, which causes presentation changes and reflow/relayout in that situation.)

(I wouldn't call e.g. webgl.drawingBufferWidth a flaw. :) This "murky" thing is for surfacing when callers ask for a size that was too big and we try to limp them along automatically with something smaller. Canvas will generally fallback to software if things get big, and hard-fail on e.g. W>32k sizes, but content doesn't generally run into that. WebGL however was running into e.g. 4k limits on 5k screens for a bit, or things like that. WebGL allocations fail much sooner than canvas2d allocations.)


Yes, we (at least Google and Mozilla) agreed to mutually prototype, and update tests as part of that prototyping.

aosmond added a commit to aosmond/gecko that referenced this issue Oct 14, 2023
…romImageBitmap.

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126
aosmond added a commit to aosmond/gecko that referenced this issue Oct 14, 2023
…romImageBitmap.

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 17, 2023
…romImageBitmap. r=gfx-reviewers,lsalzman

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 17, 2023
…romImageBitmap. r=gfx-reviewers,lsalzman

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 17, 2023
…romImageBitmap. r=gfx-reviewers,lsalzman

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126

UltraBlame original commit: cbd593a4819d1bc653fae6d21f86b25c9b1bdd58
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 17, 2023
…romImageBitmap. r=gfx-reviewers,lsalzman

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126

UltraBlame original commit: cbd593a4819d1bc653fae6d21f86b25c9b1bdd58
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 17, 2023
…romImageBitmap. r=gfx-reviewers,lsalzman

This patch makes it so that when a new ImageBitmap is provided to an
ImageBitmapRenderingContext, we resize its owning canvas rather than
crop or scale the given ImageBitmap to fit inside the canvas dimensions.

See discussion in whatwg/html#7833.

Differential Revision: https://phabricator.services.mozilla.com/D188126

UltraBlame original commit: cbd593a4819d1bc653fae6d21f86b25c9b1bdd58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: canvas
Development

No branches or pull requests

9 participants