feross / feross/capture-frame

Some feedback

Open
#10 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
115
Forks
10
PR merge metrics
No merged PRs in 30d

Description

# Formats

I tested chrome, ff & safari on this page:

https://kangax.github.io/jstests/toDataUrl_mime_type_test/

Apparently
- Safari supports png, jpeg, gif, bmp, and tiff
- Chrome supports png, jpeg and webp
- Firefox supports png, jpeg and bmp

(don't have IE/Edge on mac)

So i think this check should be removed and just allow whatever format gets thrown at it.
Other browser & format will be added at some point.
If it isn't supported then it will fallback to png. there is no harm in testing other unsupported formats.

https://github.com/feross/capture-frame/blob/947bb9ac0a0b03d0a418208666e1a324b027cc03/index.js#L18-L20

Since it will default to png then there is no need to check for format either

https://github.com/feross/capture-frame/blob/947bb9ac0a0b03d0a418208666e1a324b027cc03/index.js#L14-L16

since `image/undefined` will be a png anyway

But if you insist on keeping it, then use default parameter `function n(format = png)`

# Output

Right now it returns a Buffer and makes it a dependent on Buffer. I think that it could as well return a Uint8Array instead. Uint8Array and Buffer are both instances of Uint8Array.
That would make this package a lot smaller when using browserify.
Node are now also able to write any TypedArray with `fs` and are not restricted to only Buffer. So there is that...

Using `canvas.toBlob` i think is also better. Which means it will be async...
So returning a new promise would be something worth considering?

then you can use the FileReader or Response to turn a blob into a arrayBuffer and then resolve the promise with a Uint8Array

```js
// Response
new Promise(rs =>
canvas.toBlob(blob => {
rs(new Response(blob).arrayBuffer().then(ab => new Uint8Array(ab)))
}, 'image/' + format)
)
```

```js
// FileReader
new Promise(rs =>
canvas.toBlob(blob => {
const reader = new FileReader()
reader.onload = () => {
rs(new Uint8Array(reader.result))
}
reader.readAsArrayBuffer(blob)
}, 'image/' + format)
)
```

---

But i almost think it should return a Blob instead of a Buffer or TypedArray.
then you are able to check if the format was what you wished for by checking if the `blob.type` matches.
You are then also able to create a ObjectURL or append the blob to a FormData. Wish I think could be more useful.
it's also more RAM friendlier to have a blob that might point to some place on the HDD instead of having the hole buffer in the RAM.
...There is no easy way to guess the mime type of a buffer and do the opposite thing.

If I used webp in firefox it would return a png (with buffer). So if i assumed it worked and save it as `.webp` then it would have the wrong extension type

# Quality

Think you missing the quality parameter as well... 0-1 (default is 0.8)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.