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

Add a call-out to Web Codecs API during decoding #1018

Merged
merged 10 commits into from
May 24, 2021
Merged

Add a call-out to Web Codecs API during decoding #1018

merged 10 commits into from
May 24, 2021

Conversation

surma
Copy link
Collaborator

@surma surma commented May 18, 2021

Added a code path that uses Web Codecs API during decoding.

@surma surma requested a review from jakearchibald May 18, 2021 19:05
@google-cla google-cla bot added the CLA: Yes label May 18, 2021
}
// If none of those work, let’s see if Web Codecs API is available
if (await WebCodecs.isTypeSupported(mimeType)) {
return await abortable(signal, WebCodecs.decode(blob, mimeType));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not returning to the hail-mary path below because Web Codecs API will support all the image formats that built-in canvas supports.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a slight caveat there, we don't support ICO/CUR and SVG presently. SVG requires DOM, so can't be done in a worker context. ICO/CUR function oddly so we've disabled support in the mean time. Either way the isTypeSupported() will give you a valid answer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other differences we should be aware of vs createImageBitmap?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@surma shouldn't this code be rolled into builtinDecode? Since it's using the built-in decoders

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels too early to land this since it's an evolving API (the demos in https://twitter.com/ChromiumDev/status/1255870582442405889 no longer work) and there isn't a spec yet.

Should we make this a draft?

src/client/lazy-app/util/web-codecs/index.ts Show resolved Hide resolved
@surma
Copy link
Collaborator Author

surma commented May 19, 2021

Oh I thought this was stable-ish. Yeah, happy to let this sit for a while then.

@surma surma marked this pull request as draft May 19, 2021 10:34
@dalecurtis
Copy link

dalecurtis commented May 19, 2021

ImageDecoder is now stable (updates were tracked here: https://bugs.chromium.org/p/chromium/issues/detail?id=1182435). I've ping @tomayac about updating the demo. The spec is here https://w3c.github.io/webcodecs/#image-decoding

@tomayac
Copy link
Member

tomayac commented May 19, 2021

ImageDecoder is now stable (updates were tracked here: https://bugs.chromium.org/p/chromium/issues/detail?id=1182435). I've ping @tomayac about updating the demo. The spec is here https://w3c.github.io/webcodecs/#image-decoding

✅ Demo updated: https://imagedecoder.glitch.me/.

@jakearchibald
Copy link
Collaborator

@dalecurtis my bad. Maybe update https://github.com/dalecurtis/image-decoder-api? That page has good SEO 😀 and there's no link to the spec.

@surma surma marked this pull request as ready for review May 20, 2021 08:59
@dalecurtis
Copy link

Ah! I thought I had already done that, thanks for calling that out. Done.

@jakearchibald
Copy link
Collaborator

I've got a vague worry that we're moving from an API where the mimetype doesn't need to be accurate, to one where it does. Like, in Squoosh right now we could remove a bunch of our mime sniffing code, since we only need to sniff for the encoders we ship.

However, with this PR, we can't do that, as Chrome would no longer be able to load a .png that was actually a JPEG.

Maybe it's fine. I'm just worried about adding the complexity for little/no gain.

In future this would let us support animated formats though, which is nice.

@surma
Copy link
Collaborator Author

surma commented May 21, 2021

Would you prefer if we continued to fall through to the hail-mary canvas decoding when WebCodecs fails?

@jakearchibald
Copy link
Collaborator

Yeah let's do that for safety, just in case it turns out our WebP sniffing is wrong for some exotic flavour of WebP or whatever.

We should keep our current set of file sniffing, as we'd need it if we wanted to support animated types.

@jakearchibald
Copy link
Collaborator

Filed w3c/webcodecs#252

@surma
Copy link
Collaborator Author

surma commented May 21, 2021

Done!

@jakearchibald
Copy link
Collaborator

I still think this code should go in builtinDecode, since that's what it is

@surma surma requested a review from jakearchibald May 24, 2021 14:40
@surma
Copy link
Collaborator Author

surma commented May 24, 2021

Done. I added a couple of aborted checks to avoid unnecessary work. Is that the idiomatic way to do that with AbortController?

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got a nice little abortable helper 😄

(I haven't tested these changes)

src/client/lazy-app/util/index.ts Outdated Show resolved Hide resolved
src/client/lazy-app/util/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jakearchibald
Copy link
Collaborator

I've filed #1028 for me to do something better for abortables

@surma surma merged commit 4192c60 into dev May 24, 2021
@dalecurtis
Copy link

dalecurtis commented May 24, 2021

@jakearchibald WDYT about having AbortSignal as a second parameter to decode()? (In the standard) Or as a member in the ImageDecodeOptions dictionary.

@surma
Copy link
Collaborator Author

surma commented May 24, 2021

I know you didn’t ask me, but considering that even decoding can take any amount of time in the context of WebCodecs (as we don’t curate the codecs anymore), that seems like a very good idea to me.

@dalecurtis
Copy link

All opinions welcome :) Filed w3c/webcodecs#254

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 this pull request may close these issues.

4 participants