Automattic / node-canvas

Node canvas is a Cairo backed Canvas implementation for NodeJS.
10.13k stars 1.17k forks source link

Introduce blob's #1845

Open jimmywarting opened 3 years ago

jimmywarting commented 3 years ago

NodeJS recently introduced Blobs into core

think this is grate cuz now we can have canvas.toBlob(cb)

we could also introduce a new fn that can create a ImageBitmap out of a more web/standarlized way with createImageBitmap(blob) instead of using the abnormal way with loadImage(...) or new Image(...) (that i think should be deprecated/removed)

I also think createCanvas should go away for a web-worker related thing which is the OffscreenCanvas cunstructor

so that would also mean: we would use OffscreenCanvas.convertToBlob instead of canvas.toBlob

This was what i did to feel more at home:

const {
    // More correct naming convention
    createCanvas: OffscreenCanvas,
    loadImage: createImageBitmap
} = NodeCanvas;

export async function cropImage(...args) {
    /** @type {ImageBitmap} */
    const bitmap = await createImageBitmap(imagePath);
    const canvas = OffscreenCanvas(width, height);

(still a tiny bit node specific - would like it to work 100% like a Web Worker would handle everything for cross browser coding) fetch-blob is a good candidate for implementing support for it already

An example with using fetch-blob with createImageBitmap() would look like

import { createImageBitmap } from 'canvas'
import { blobFrom } from 'fetch-blob/from.js'

const blob = await blobFrom('./sample.jpg')
const bitmap = await createImageBitmap(blob)
console.log(bitmap.width, bitmap.height, bitmap.close)

// or simply: 
const bitmap = await blobFrom('./sample.jpg').then(createImageBitmap)

This would be the correct way to get a buffer:

const blob = await canvas.convertToBlob()
const buf = await blob.arrayBuffer().then(Buffer.from)

Close #1845, #1705, #1735, #1758, #1802 in favor of the new way to load images with createImageBitmap(blob)? deprecate new Image() and loadImage()? use FinalizationRegistry and ImageBitmap.close to release the memory in native binding

jimmywarting commented 3 years ago

This would also help to quickly upload/download things using (node-)fetch and do other things with it

LinusU commented 3 years ago

I think that it would be awesome to "transition" node-canvas to be an implementation of OffscreenCanvas instead of Canvas, which is more accurately what it is current anyways. Maybe even change import { createCanvas } from 'canvas' to import { OffscreenCanvas } from 'canvas' (but keep the old function as well for compatibility)?

Anything that makes us more compatible I'm 👍

jimmywarting commented 3 years ago

(but keep the old function as well for compatibility)?

yea, sounds good! but OffscreenCanvas is a constructor doe... need to use new

jimmywarting commented 3 years ago

ctx.drawImage would have to accept a ImageBitmap class instead/also, But it maybe already do? saw some stuff in the readme that i missed and that u have util like createBitmap

jimmywarting commented 3 years ago

fyi, fetch-blob is a ESM only package, so to load it from commonjs you would have to use the async import() instead cuz esm are async b/c of top level await

but that would be fine either way. createImageBitmap don't need to import something to know that it's a blob and can be read. convertToBlob is async either way also. so can just lazy import it when needed instead

LinusU commented 3 years ago

(but keep the old function as well for compatibility)?

yea, sounds good! but OffscreenCanvas is a constructor doe...

Yeah, we could export an OffscreenCanvas class and have createCanvas return new OffscreenCanvas(...)

fyi, fetch-blob is a ESM only package

With the next major canvas version I think we should go ESM only as well

jimmywarting commented 3 years ago

Maybe this is a good boilerplate to start of from:

import { Bitmap } from './bindings.js'
const unlock = Symbol('Illegal constructor')

class ImageBitmap {
  #width = 0
  #height = 0
  #close

  constructor(key, width, height, #close) {
    if (key !== unlock) {
      throw new Error('Illegal constructor')
    }
  }

  get width () {
    return this.#width
  }

  get height () {
    return this.#height
  }

  close() {
    this.#close && this.#close()
    this.#close = null
  }
}

const registry = new FinalizationRegistry(bitmap => {
  bitmap.close()
});

/**
 * @param {Blob} image
 */
async function createImageBitmap(image, sx, sy, sw, sh, options = {}) {
  const ab = await blob.arrayBuffer()
  const data = new Bitmap(new Uint8Array(ab))
  const imageBitmap = new ImageBitmap(unlock, data.width, data.height, data.close.bind(data))
  registry.register(imageBitmap, data)
  return imageBitmap
}
Novivy commented 1 year ago

is .toBlob gonna be implemented..?

jimmywarting commented 1 year ago

b/c node-canvas is more like a OffscreenCanvas then i have already made a PR that adds the async convertToBlob

Kuboczoch commented 1 month ago

This issue may be fixed by #2127, which @jimmywarting has done (it is even approved).

Can we merge it into the main branch this year?