Repository navigation
[web] Unify image decoding and codecs on CanvasKit and Skwasm - #188573
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Flutter Web Engine's image decoding architecture by introducing a unified EngineCodec that delegates to specific sub-codecs (_BrowserEngineCodec, _StaticEngineCodec, and _SkiaEngineCodec) and abstracting backend-specific animated image decoders under a new BackendAnimatedImage contract. It removes legacy codecs, updates the CanvasKit and Skwasm renderers to implement the new interfaces, and optimizes native resource handling and stream processing. The review feedback highlights several critical opportunities to prevent native resource leaks (such as VideoFrame, DomImageBitmap, and ImageSource) by wrapping asynchronous decoding, scaling, and texture upload operations in try-catch or try-finally blocks. Additionally, suggestions were made to improve exception safety during decoder disposal and to avoid unnecessary stream-teeing overhead when the content length is unknown.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the web engine's image decoding architecture by introducing unified EngineCodec and BackendAnimatedImage abstractions to consolidate the CanvasKit and Skwasm rendering pipelines. It replaces legacy HTML-based codecs with native browser decoders (BrowserImageDecoder) and backend-specific fallback decoders, while optimizing texture uploads and stream-based progress tracking. Feedback suggests wrapping the decoder initialization in a try-catch block to ensure decoder.dispose() is called on failure, preventing potential resource leaks from the duplicated progress stream reader.
96eef46 to
33e5155
Compare
|
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. |
|
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. |
| ui_web.ImageCodecChunkCallback? chunkCallback, | ||
| }) async { | ||
| final url = uri.toString(); | ||
| final DomResponse response = await rawHttpGet(url); |
There was a problem hiding this comment.
consider httpFetch instead of rawHttpGet
rawHttpGet is a bare fetch. Line 448 handles bad status codes, but a request that
never completes, meaning CORS blocked, DNS failure, or offline, rejects the promise and
now surfaces as TypeError: Failed to fetch.
There was a problem hiding this comment.
Done. I also added a bit of API to HttpFetchResponse to support getting the headers
| EngineCodec._(); | ||
| } | ||
|
|
||
| /// An [EngineCodec] that decodes frames using the browser's [BrowserImageDecoder]. |
There was a problem hiding this comment.
Doc comment is duplicated verbatim.
| BrowserImageDecoder browserDecoder, { | ||
| this.targetWidth, | ||
| this.targetHeight, | ||
| this.allowUpscaling = true, |
There was a problem hiding this comment.
_BrowserEngineCodec accepts allowUpscaling but never reads it, so a direct instantiateImageCodec(..., allowUpscaling: false) no longer caps dimensions. Honor the flag or note it's intentional, and add a test?
There was a problem hiding this comment.
Good catch! I added support back for allowUpscaling and added a test
|
Thanks, @flutter-zl! PTAL |
|
autosubmit label was removed for flutter/flutter/188573, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/188573, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/188573, because - The status or check suite Mac_arm64_verify_binaries has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/188573, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…12406) Manual roll Flutter from e52f01c920ad to b766512c65d8 (42 revisions) Manual roll requested by stuartmorgan@google.com flutter/flutter@e52f01c...b766512 2026-08-04 engine-flutter-autoroll@skia.org Roll Dart SDK from 2a799a2404e9 to 9859c0a39adb (4 revisions) (flutter/flutter#190521) 2026-08-04 154381524+flutteractionsbot@users.noreply.github.com Revert: iOS: Eliminate use of IOSContextNoop in platform view tests (flutter/flutter#190501) 2026-08-04 125822178+guszxtavo@users.noreply.github.com [Impeller] Enable ETC2/ASTC LDR/BC texture compression features at Vulkan device creation (flutter/flutter#189303) 2026-08-03 30870216+gaaclarke@users.noreply.github.com Remove openglessdf from impeller_unittests. (flutter/flutter#190469) 2026-08-03 1961493+harryterkelsen@users.noreply.github.com [web] Unify image decoding and codecs on CanvasKit and Skwasm (flutter/flutter#188573) 2026-08-03 chris@bracken.jp iOS: Eliminate use of IOSContextNoop in platform view tests (flutter/flutter#190419) 2026-08-03 evanwall@buffalo.edu Add path rendering benchmarks (flutter/flutter#188654) 2026-08-03 97480502+b-luk@users.noreply.github.com Add windows platform support for primitive_shape_test integration test (flutter/flutter#190464) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from 958c1c1921a1 to a08d918ebd6a (3 revisions) (flutter/flutter#190467) 2026-08-03 chris@bracken.jp tests: add --ios-runtime param (flutter/flutter#190414) 2026-08-03 chris@bracken.jp iOS: Remove the synchronous first-frame wait (flutter/flutter#190432) 2026-08-03 chris@bracken.jp iOS: Eliminate the Impeller/Skia backend selection params (flutter/flutter#190416) 2026-08-03 chris@bracken.jp iOS,macOS: Use @autoclosure in Logger (flutter/flutter#190417) 2026-08-03 chris@bracken.jp tools: Support FLUTTER_HOST_ARCH in update_dart_sdk scripts (flutter/flutter#190421) 2026-08-03 chris@bracken.jp iOS: Hardcode rendering API to Metal in tests (no-op) (flutter/flutter#190422) 2026-08-03 chris@bracken.jp a11y: Map disabled/read-only semantics to AX node restriction (flutter/flutter#190353) 2026-08-03 kevmoo@users.noreply.github.com [Infra] Replace defunct umbrella template with Wasm issue form (flutter/flutter#190471) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from abecb0dc02c1 to 958c1c1921a1 (4 revisions) (flutter/flutter#190459) 2026-08-03 engine-flutter-autoroll@skia.org Roll Dart SDK from 65b163be2485 to 2a799a2404e9 (3 revisions) (flutter/flutter#190454) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from 68efb3f2ad16 to abecb0dc02c1 (1 revision) (flutter/flutter#190443) 2026-08-03 engine-flutter-autoroll@skia.org Roll Packages from 5351d8c to ac87e65 (4 revisions) (flutter/flutter#190441) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from 5a761eb826c1 to 68efb3f2ad16 (1 revision) (flutter/flutter#190440) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from 4c9f8b4805e2 to 5a761eb826c1 (1 revision) (flutter/flutter#190437) 2026-08-03 ellie@edencrew.com [macOS] Resume app lifecycle on becomeActive to avoid frozen UI after occlusion (flutter/flutter#188772) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from 39cda9d6d7d2 to 4c9f8b4805e2 (6 revisions) (flutter/flutter#190426) 2026-08-03 engine-flutter-autoroll@skia.org Roll Skia from df13bfb5a54e to 39cda9d6d7d2 (2 revisions) (flutter/flutter#190425) 2026-08-02 chris@bracken.jp iOS: Serialise CADisplayLink access in VSyncClient tests (flutter/flutter#190335) 2026-08-02 engine-flutter-autoroll@skia.org Roll Skia from 32329e5643b5 to df13bfb5a54e (1 revision) (flutter/flutter#190394) 2026-08-02 bdero@google.com [Impeller] Skip binding dead-code-eliminated resources on Metal (flutter/flutter#190040) 2026-08-01 bdero@google.com [Flutter GPU] Raise Dart errors for invalid render pipelines and memoize per-draw pipeline state (flutter/flutter#189899) 2026-08-01 41930132+hellohuanlin@users.noreply.github.com Revert "Improve non rect platform view rendering (#182662)" (flutter/flutter#190003) 2026-08-01 engine-flutter-autoroll@skia.org Roll Skia from ebf50520d720 to 32329e5643b5 (1 revision) (flutter/flutter#190389) 2026-08-01 engine-flutter-autoroll@skia.org Roll Skia from f73c4510d12d to ebf50520d720 (6 revisions) (flutter/flutter#190376) 2026-07-31 97480502+b-luk@users.noreply.github.com Primitive shape integration test (flutter/flutter#190368) 2026-07-31 97480502+b-luk@users.noreply.github.com Eliminate some early returns in uber_sdf.frag to fix broken UberSDF AA on Windows (flutter/flutter#190260) 2026-07-31 codefu@google.com chore: swiftshader mirrored + llvm16 (flutter/flutter#181225) 2026-07-31 1961493+harryterkelsen@users.noreply.github.com [web] Remove in-repo agent documentation (flutter/flutter#190326) 2026-07-31 30870216+gaaclarke@users.noreply.github.com [windows]: Uses offscreen MSAA when implicit msaa isn't available. (flutter/flutter#190256) 2026-07-31 srawlins@google.com flutter_tools: Use new FileSystemExtension from devtools (flutter/flutter#190360) 2026-07-31 engine-flutter-autoroll@skia.org Roll Dart SDK from c3acfc2479f6 to 65b163be2485 (1 revision) (flutter/flutter#190358) 2026-07-31 magder@google.com Use devicectl for screenshots on Xcode 27, remove idevicescreenshot artifact (flutter/flutter#189091) 2026-07-31 engine-flutter-autoroll@skia.org Roll Skia from 7ef86a5b0eb9 to f73c4510d12d (1 revision) (flutter/flutter#190352) If this roll has caused a breakage, revert this CL and stop the roller ...
…n Chrome 0ec9641 hat dieses Repo auf 3.47.1 gehoben, "in lockstep with both apps". Das war die falsche Richtung: 3.47 hört auf Firefox und Safari (iPhone eingeschlossen) still auf, Bilder zu zeichnen — ein Thumbnail rendert einmal und bleibt bei jedem späteren Repaint leer, bis die Seite neu geladen wird. CachedFileImage ist die Stelle, an der es auftaucht, also erben es beide Apps, und nichts in diesem Repo kann es reparieren. Also fordern die Pubspecs hier wieder `^3.44.0`. Das ist der Punkt und nicht bloß Kosmetik: solange sie ^3.47.0 fordern, bricht der NÄCHSTE Pin-Bump den Build jeder App, die auf 3.44 zurück ist — federfall entkam nur, weil sein Pin älter ist als 0ec9641, und eiermann entkam gar nicht. Vier Stellen, alle zusammen: die zwei Pubspecs, CI, und die Versionszeilen in README und CLAUDE.md. Dazu zurück, was 0ec9641 am Code geändert hatte: auf 3.44 sind die Animationsfutures nicht `@awaitNotRequired`, also greift wieder `discarded_futures` und die vier `unawaited(...)` müssen stehen (mit `dart:async` in offline_notice.dart). Gemessen: ohne sie meldet `flutter analyze` genau diese vier Stellen. Die AUSFÜHRLICHE Analyse des Fehlers in CLAUDE.md — skwasm gegen CanvasKit, flutter.js' wasmAllowList, flutter/flutter#186032 und der Fix in #188573 — stammt aus einer parallelen Sitzung. Ihr Commit ging bei einem Reset in diesem Arbeitsbaum verloren; der Text ist aus dem noch vorhandenen Objekt wiederhergestellt und steht hier unverändert, bis auf den Schlussabsatz: die Falle, die er beschreibt ("die nächste Pin-Bump bricht federfalls Build"), ist genau die, die dieser Commit zumacht. Verifiziert auf 3.44.3: analyze sauber, 127 + 68 + 50 + 48 Tests grün, `dart format --set-exit-if-changed` sauber. Ziel bleibt 3.48 (flutter/flutter#188573). Auf 3.47 wird nicht gewartet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit 7da167f. 3.47 silently loses images on Firefox and Safari — iPhone included. A thumbnail paints once and then draws nothing on any later repaint, until a full page reload. Chrome is unaffected, which is what made it easy to miss. Two gates decide it, and Firefox/Safari fail both. `--wasm` ships skwasm plus a CanvasKit fallback, and flutter.js's default wasmAllowList is {blink:true, gecko:false, webkit:false} — so only Chrome runs skwasm, which copies decoded bytes eagerly into an ImageBitmap and owns them. Everything else falls back to CanvasKit, where an image decoded from bytes is a LAZY SkImage over the <img> element the codec made, re-uploading its texture on every paint. flutter/flutter#186032 then taught the engine to reclaim that element aggressively — `ImageElementImageSource._doClose()` went from a deliberate no-op ("let the browser garbage collect it") to `imageElement.src = ''`. Its stated purpose was to stop iOS Safari crashing on many large images, so the regression lands squarely in the case the change was written for. Confirmed by A/B, not by reading: the same commit built `--wasm --release` against the same backend, served with the same COOP/COEP headers, breaks on 3.47.1 and works on 3.44.3. Three app-side workarounds were tried and all failed — clamping the decode below the 200x200 thumb, raising the ImageCache ceilings, and waiting out the new 30s decode timeout — so this is not something federfall can hold at arm's length. The exact defect is still unpinned; a lazy texture whose source is freed on refcount-zero is the suspect, but ImageBitmapImageSource frees its source the same way and is fine on both versions, so that story is incomplete. 3.48 fixes it. flutter/flutter#188573 moves image decoding and lifetime onto the shared frontend skwasm already used, and 3.48.0-0.2.pre renders correctly in Firefox on the same harness. It is a refactor, not a targeted fix, so a 3.47.2 backport is not worth waiting for — and 3.47 offered nothing this app wanted anyway, only higher Java and iOS floors. So: back to 3.44.3, and go straight to 3.48 when it is stable (the cadence puts that around November). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Consolidates all image decoding, tiered network routing, progressive stream duplication, and DOM-based resizing logic into a unified shared frontend coordinator (EngineCodec). Both CanvasKit and Skwasm backends are reduced to dumb native adapters, and garbage-collection finalization is centralized exclusively in the frontend wrappers.
More work towards CanvasKit/Skwasm unification: #175630
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
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.