whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.1k stars 2.66k forks source link

ImageData alpha premultiplication #5365

Open adroitwhiz opened 4 years ago

adroitwhiz commented 4 years ago

This is outdated! See #5371 for current proposal

Currently, the ImageData object contains pixel data with un-premultiplied/unassociated alpha, meaning that the red, green, and blue values of each pixel are not multiplied by that pixel's alpha value.

This causes a couple of issues:

It would be helpful to add a premultiplied property to ImageData objects:

interface ImageData {
  constructor(unsigned long sw, unsigned long sh, optional boolean premultiplied);
  constructor(Uint8ClampedArray data, unsigned long sw, optional unsigned long sh, optional boolean premultiplied);

  readonly attribute unsigned long width;
  readonly attribute unsigned long height;
  readonly attribute Uint8ClampedArray data;
  attribute boolean premultiplied; // (default false)
};
interface mixin CanvasImageData {
  // pixel manipulation
  ImageData createImageData(long sw, long sh, optional boolean premultiplied);
  ImageData createImageData(ImageData imagedata);
  ImageData getImageData(long sx, long sy, long sw, long sh, optional boolean premultiplied);
  void putImageData(ImageData imagedata, long dx, long dy);
  void putImageData(ImageData imagedata, long dx, long dy, long dirtyX, long dirtyY, long dirtyWidth, long dirtyHeight);
};

The way I could see this behaving is:

These APIs are quite rough, but hopefully they convey the intended functionality.

domenic commented 4 years ago

/cc @whatwg/canvas

fserb commented 4 years ago

One other request we've also been consistently getting is the ability to pass an existing ImageData to getImageData to be updated instead of creating a new one.

adroitwhiz commented 4 years ago

I've created a quick-and-dirty Firefox patch for those who want to play around with the API. It's rough around the edges but might be helpful for prototyping.

kdashg commented 4 years ago

It's an interesting choice for premultiplied to be non-readonly, but I think that might be fine. Overall this is a good idea.

domenic commented 4 years ago

API design critique: avoid boolean arguments and use options dictionaries instead.

Is there a strong motivation to allow changing premultiplied after the fact? Otherwise keeping ImageData objects immutable would be better.

adroitwhiz commented 4 years ago

I suppose it would be nicer to make the premultiplication state immutable. If a developer needs to change it, they can always create a new ImageData with a different premultiplication state that shares the same underlying buffer.

Maybe an options dictionary could look something like:

enum AlphaAssociation { "straight", "premultiplied" };

dictionary ImageDataOptions {
  AlphaAssociation alphaAssociation = "straight";
};

Not so sure about the name-- would simply alpha be better? This could also open the door to an "opaque" AlphaAssociation, which I could see representing data laid out as RGBRGB... with no alpha values (although reinterpreting that buffer as alpha-containing data would be a whole different can of worms).

domenic commented 4 years ago

Sorry, to be clear a boolean inside an options dictionary is fine. (Although maybe an enum is nicer, I dunno, that's up to you.) The principle is to avoid new ImageData(10, 10, true) and instead have new ImageData(10, 10, { premultiplied: true }). new ImageData(10, 10, { alphaAssociation: "premultiplied" }) may also be nice; I don't have a preference between those two.

kdashg commented 4 years ago

I think a bool is better than alphaAssociation. It's less clear to me now than it was before. We should use existing terms of art.

fserb commented 4 years ago

I agree the bool is better.

adroitwhiz commented 4 years ago

So the consensus seems to be on something like:

dictionary ImageDataOptions {
    boolean premultiplied = false;
};

interface ImageData {
  constructor(unsigned long sw, unsigned long sh, optional ImageDataOptions options = {});
  constructor(Uint8ClampedArray data, unsigned long sw, optional unsigned long sh, optional ImageDataOptions options = {});

  readonly attribute unsigned long width;
  readonly attribute unsigned long height;
  readonly attribute Uint8ClampedArray data;
  attribute boolean premultiplied; // (default false)
};
interface mixin CanvasImageData {
  // pixel manipulation
  ImageData createImageData(long sw, long sh, optional ImageDataOptions options = {});
  ImageData createImageData(ImageData imagedata);
  ImageData getImageData(long sx, long sy, long sw, long sh, optional ImageDataOptions options = {});
  void putImageData(ImageData imagedata, long dx, long dy);
  void putImageData(ImageData imagedata, long dx, long dy, long dirtyX, long dirtyY, long dirtyWidth, long dirtyHeight);
};

Thoughts on this?

kdashg commented 4 years ago

I like the ImageDataOptions and ImageData, but CanvasImageData should be a different proposal.

IE think I see what you're wanting to do with CanvasImageData instead of using a canvas2d context, but you'll need to compel the usecase for this better.

kdashg commented 4 years ago

@kenrussell ^

adroitwhiz commented 4 years ago

@jdashg The CanvasImageData interface already exists-- I only modified the createImageData and getImageData signatures to use the new ImageDataOptions.

kdashg commented 4 years ago

Oh sorry, I forgot about that weird canvas mixin thing. (I find its name confusing) Sounds good to me.

kenrussell commented 4 years ago

Nice proposal. The fact that the new attribute is visible on the ImageData interface means that this is easily feature-detectable and won't run into the same problems as #4248 .

Two comments:

1) It would be ideal if the name "premultipliedAlpha" could be used in order to have parity with the same-named WebGL context creation attribute: https://www.khronos.org/registry/webgl/specs/latest/1.0/#WEBGLCONTEXTATTRIBUTES .

2) It would be preferred to have the premultipliedAlpha attribute be immutable on the ImageData. Simply reinterpreting the contents of the Uint8Array will almost never work correctly if alpha != 255. This means that changing the attribute from false to true or vice versa essentially requires updating the contents of the Uint8Array, and operations like this should be restricted to the construction of a new ImageData. Further, it should be specified that constructing an ImageData from another one where the premultipliedAlpha creation attribute differs is a lossy operation.

adroitwhiz commented 4 years ago

@kenrussell

Further, it should be specified that constructing an ImageData from another one where the premultipliedAlpha creation attribute differs is a lossy operation.

How can such an operation occur? Passing an existing ImageData object to createImageData returns a new ImageData filled with transparent black, and creating an ImageData object which shares its buffer with another should, in my opinion, simply reinterpret the data.

kenrussell commented 4 years ago

@kenrussell

Further, it should be specified that constructing an ImageData from another one where the premultipliedAlpha creation attribute differs is a lossy operation.

How can such an operation occur? Passing an existing ImageData object to createImageData returns a new ImageData filled with transparent black, and creating an ImageData object which shares its buffer with another should, in my opinion, simply reinterpret the data.

Sorry, my mistake, I didn't remember how createImageData(ImageData) worked. You're right, since the spec says that the only way to create an ImageData initially containing some externally-produced data is by passing in Uint8ClampedArray to the ImageData constructor, it won't / can't modify the data.

Making the premultipliedAlpha attribute read-only still makes this proposal easier to reason about in my opinion. Reinterpreting a previously non-premultipliedAlpha ImageData as premultipledAlpha is confusing.

kdashg commented 4 years ago

I sort of like that it's not readonly, since that lets you "recast" it without having to copy or something. It's lesser usecase, but I think it's real.

adroitwhiz commented 4 years ago

I sort of like that it's not readonly, since that lets you "recast" it without having to copy or something. It's lesser usecase, but I think it's real.

Even if premultipliedAlpha is immutable, you can still do something like:

const premultiplied = new ImageData(1000, 1000, {premultipliedAlpha: true});

const nonPremultiplied = new ImageData(
  premultiplied.data,
  premultiplied.width,
  premultiplied.height,
  {premultipliedAlpha: false}
);

to "recast" the data.

adroitwhiz commented 9 months ago

Is there still any interest in this API? I let it lay dormant for a while because it seemed like there was a lot of discussion happening with the canvas-color-space proposal at the time, and I didn't want to step on their toes, especially if they were going to implement the same feature. It seems like the discussion has now shifted over to the Color on the Web group at W3C and their HDR canvas proposal, minus anything about alpha.

In addition, an ImageDataSettings parameter has been added to the spec now! Adding premultipliedAlpha to that settings dictionary should be fairly straightforward.

I don't think this would conflict at all with canvas color space or bit depth proposals. Premultiplication is completely orthogonal to both of those.

If there's still interest in this, I can try and update my Firefox patch and try and get this in behind a feature flag. The only concern I can see with regards to FF specifically is that they haven't yet implemented canvas color space support (they seem to still be working on wide-gamut support), and I could conceive of a developer feature-detecting it by checking whether they can pass an object into the ImageData constructor.

brianosman commented 8 months ago

I haven't discussed it with other folks here to get an official position, but I just happened to be looking at other proposals, and was reminded of this one. I'd definitely like to see this proposal move forward.

kdashg commented 6 months ago

I think it is valuable. IIRC people rely on WebGL sometimes just to be able to pull non-premult data out of e.g. PNGs. Feature-testing new additions to options dictionaries is awkward, but considered acceptably doable for devs. Ideally devs could just test for the presence of ImageData.premultipliedAlpha or something, which ought to be implemented in lockstep with recognizing this in ImageDataSettings.

adroitwhiz commented 6 months ago

My concern is moreso that this would be the first appearance of the options dictionary in the first place, at least in FF. I briefly looked into your prototype WebGL color space implementation and applying it to canvas elements, so that the dictionary could support both colorSpace and the premul stuff, but that's way outside my wheelhouse as someone who's only skimmed the FF codebase.

I'll try to throw together a quick patch containing just the premultiplication stuff, but my thought was that it might be better to defer that until after you've finished refactoring color management.

kdashg commented 6 months ago

Refactoring color management will take about a year, so no reason to wait.