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

Introduce blob's #1845

Open
jimmywarting opened this issue Jul 7, 2021 · 9 comments · May be fixed by #2127
Open

Introduce blob's #1845

jimmywarting opened this issue Jul 7, 2021 · 9 comments · May be fixed by #2127
Labels

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented Jul 7, 2021

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
Copy link
Contributor Author

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

@LinusU
Copy link
Collaborator

LinusU commented Jul 7, 2021

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
Copy link
Contributor Author

jimmywarting commented Jul 7, 2021

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

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

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jul 7, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

LinusU commented Jul 7, 2021

(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
Copy link
Contributor Author

jimmywarting commented Jul 7, 2021

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
Copy link

Novivy commented Nov 12, 2022

is .toBlob gonna be implemented..?

@jimmywarting jimmywarting linked a pull request Nov 12, 2022 that will close this issue
1 task
@jimmywarting
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants