[canvaskit] Fix image decoding in CPU-only mode#180706
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with image decoding in CPU-only mode by using MakeImageFromCanvasImageSource as a fallback, which relies on a 2D canvas. The changes are logical and correctly implemented across the affected files, including the addition of a new test file to verify the fix. I have one suggestion to improve performance in image_web_codecs.dart by removing what appears to be an unnecessary intermediate canvas when handling video frames.
| final int width = frame.displayWidth.toInt(); | ||
| final int height = frame.displayHeight.toInt(); | ||
| final DomHTMLCanvasElement canvas = createDomCanvasElement(width: width, height: height); | ||
| final DomCanvasRenderingContext2D ctx = canvas.context2D; | ||
| ctx.drawImage(frame, 0, 0); | ||
| skImage = canvasKit.MakeImageFromCanvasImageSource(canvas); |
There was a problem hiding this comment.
In CPU-only mode, you're creating an intermediate canvas to draw the VideoFrame onto, and then passing that canvas to MakeImageFromCanvasImageSource. Since VideoFrame is a CanvasImageSource, you should be able to pass it directly to MakeImageFromCanvasImageSource. This would avoid the overhead of creating and drawing to an intermediate canvas, improving performance and simplifying the code.
| final int width = frame.displayWidth.toInt(); | |
| final int height = frame.displayHeight.toInt(); | |
| final DomHTMLCanvasElement canvas = createDomCanvasElement(width: width, height: height); | |
| final DomCanvasRenderingContext2D ctx = canvas.context2D; | |
| ctx.drawImage(frame, 0, 0); | |
| skImage = canvasKit.MakeImageFromCanvasImageSource(canvas); | |
| skImage = canvasKit.MakeImageFromCanvasImageSource(frame); |
There was a problem hiding this comment.
@gemini-code-assist I tried this at first but it crashed in the CanvasKit JS bindings. I'll try it again to see if it works but I think this workaround is needed
There was a problem hiding this comment.
Thanks for the context, @harryterkelsen. It's good to know you already explored that option. The direct approach of passing VideoFrame to MakeImageFromCanvasImageSource would indeed be more performant and simpler, so it's unfortunate that it led to a crash in the CanvasKit JS bindings.
If you have any more details about the crash (e.g., error messages, stack traces, or specific scenarios where it occurs), I'd be happy to take a look and see if there's a way to resolve it. Otherwise, if the intermediate canvas is indeed a necessary workaround due to a CanvasKit limitation, then the current implementation is understandable.
There was a problem hiding this comment.
@gemini-code-assist It crashes when I call MakeImageFromCanvasImageSource directly with the VideoFrame with the error:
Error: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': Value is not of type 'long'.
org-dartlang-sdk:///lib/_internal/js_shared/lib/js_util_patch.dart 149:10 Object._callMethodUnchecked1
../../../../flutter/lib/web_ui/lib/src/engine/canvaskit/image_web_codecs.dart 52:27 CkBrowserImageDecoder.generateImageFromVideoFrame
...
I suspect because the CanvasKit implementation gets the width of the image source directly using source.width and source.height, which isn't defined for a VideoFrame.
There was a problem hiding this comment.
Thanks for the additional details, @harryterkelsen. Your analysis regarding the VideoFrame not directly exposing width and height properties in a way that CanvasKit's MakeImageFromCanvasImageSource expects is very insightful and likely the root cause of the crash. It makes perfect sense that drawing the VideoFrame to an intermediate HTMLCanvasElement first would resolve this, as the HTMLCanvasElement would then provide the expected width and height attributes. Given this limitation in the CanvasKit JS bindings, your current workaround with the intermediate canvas is indeed necessary and the correct approach to ensure functionality.
There was a problem hiding this comment.
I suspect because the CanvasKit implementation gets the width of the image source directly using
source.widthandsource.height, which isn't defined for aVideoFrame.
If VideoFrame is a legit CanvasImageSource, then this sounds like a bug in CanvasKit?
There was a problem hiding this comment.
Yeah, the CanvasKit bindings shouldn't assume that .width and .height are available. They should probably allow passing the width and height as optional parameters
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| final int width = frame.displayWidth.toInt(); | ||
| final int height = frame.displayHeight.toInt(); | ||
| final DomHTMLCanvasElement canvas = createDomCanvasElement(width: width, height: height); | ||
| final DomCanvasRenderingContext2D ctx = canvas.context2D; | ||
| ctx.drawImage(frame, 0, 0); | ||
| skImage = canvasKit.MakeImageFromCanvasImageSource(canvas); |
There was a problem hiding this comment.
I suspect because the CanvasKit implementation gets the width of the image source directly using
source.widthandsource.height, which isn't defined for aVideoFrame.
If VideoFrame is a legit CanvasImageSource, then this sounds like a bug in CanvasKit?
| ctx.drawImage(frame, 0, 0); | ||
| skImage = canvasKit.MakeImageFromCanvasImageSource(canvas); | ||
| } else { | ||
| skImage = canvasKit.MakeLazyImageFromTextureSourceWithInfo( |
There was a problem hiding this comment.
To make this easier to catch in the future, should we add an assert to MakeLazyImageFromTextureSourceWithInfo, something like:
assert(!CanvasKitRenderer.instance.isSoftware, 'Cannot use `MakeLazyImageFromTextureSourceWithInfo` in CPU-only mode.');There was a problem hiding this comment.
Good idea, I'll add it
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
In CPU-only rendering mode, we were using
MakeLazyImageFromTextureSourcewhich only works if a WebGL context is available (which it isn't in CPU-only mode). This fixes the problem by usingMakeImageFromCanvasImageSourcewhich uses a 2d canvas when we have fallen back to software rendering.Fixes #175423
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.