Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Remove image codecs from Canvaskit Chromium #40309

Merged
auto-submit merged 9 commits into from
Mar 17, 2023
Merged

[web] Remove image codecs from Canvaskit Chromium #40309

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
08e97bf
[web] Remove image codecs from Canvaskit Chromium
mdebbar Mar 15, 2023
ac14b78
Merge branch 'main' into image_codecs
mdebbar Mar 15, 2023
453f300
update browser detection
mdebbar Mar 15, 2023
e875ac0
tests for browser detection
mdebbar Mar 15, 2023
f24f10d
review comments
mdebbar Mar 15, 2023
79c6490
Merge branch 'main' into image_codecs
mdebbar Mar 16, 2023
56dccbe
make it a getter to fix tests
mdebbar Mar 16, 2023
828331f
Merge branch 'main' into image_codecs
mdebbar Mar 17, 2023
43aae6a
renames
mdebbar Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/browser_detection.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,5 +269,5 @@ int _detectWebGLVersion() {
}

/// Whether the current browser supports the Chromium variant of CanvasKit.
final bool browserSupportsCanvaskitChromium =
bool get browserSupportsCanvaskitChromium =>
browserSupportsImageDecoder && domIntl.v8BreakIterator != null;
17 changes: 13 additions & 4 deletions lib/web_ui/lib/src/engine/safe_browser_api.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,17 +217,26 @@ const bool _browserImageDecodingEnabled = bool.fromEnvironment(
);

/// Whether the current browser supports `ImageDecoder`.
bool browserSupportsImageDecoder = _defaultBrowserSupportsImageDecoder;
bool browserSupportsImageDecoder = _kDefaultBrowserSupportsImageDecoder;

/// Sets the value of [browserSupportsImageDecoder] to its default value.
void debugResetBrowserSupportsImageDecoder() {
browserSupportsImageDecoder = _defaultBrowserSupportsImageDecoder;
browserSupportsImageDecoder = _kDefaultBrowserSupportsImageDecoder;
}

bool _defaultBrowserSupportsImageDecoder =
bool get _kDefaultBrowserSupportsImageDecoder =>
_browserImageDecodingEnabled &&
_imageDecoderConstructor != null &&
browserEngine == BrowserEngine.blink;
_kProtectFromImageDecoderIncompatibilities;

// TODO(yjbanov): https://github.com/flutter/flutter/issues/122761
// Frequently, when a browser launches an API that other browsers already
// support, there are subtle incompatibilities that may cause apps to crash if,
// we blindly adopt the new implementation. This variable prevents us from
// picking up potentially incompatible implementations of ImagdeDecoder API.
// Instead, when a new browser engine launches the API, we'll evaluate it and
// enable it explicitly.
bool get _kProtectFromImageDecoderIncompatibilities => browserEngine == BrowserEngine.blink;
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire Mar 17, 2023

Choose a reason for hiding this comment

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

Complete nitpick, but I don't like the name of this variable. On its face, the name seems to imply the opposite of what it does. If something called ProtectFromImageDecoderIncompatibilities is true, I would think that means that you are protecting from incompatibilities, and if it's false, I would think you would not be protecting from incompatibilities. In general, either way, the name is a bit confusing as a name of a boolean. Might I suggest something like _kImageDecoderIsStable or _kImageDecoderIsBlessed or something like that?

Copy link
Contributor Author

@mdebbar mdebbar Mar 17, 2023

Choose a reason for hiding this comment

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

Agreed. I also removed the k prefix since these are getters and not consts anymore.


/// The signature of the function passed to the constructor of JavaScript `Promise`.
typedef JsPromiseCallback = void Function(void Function(Object? value) resolve, void Function(Object? error) reject);
Expand Down
5 changes: 3 additions & 2 deletions third_party/canvaskit/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ group("canvaskit_group") {
# This toolchain is only to be used by canvaskit_chromium_group below.
wasm_toolchain("canvaskit_chromium") {
extra_toolchain_args = {
# Remove ICU data.
# In Chromium browsers, we can use the browser's APIs to get the necessary
# ICU data.
skia_use_icu = false
skia_use_client_icu = true
skia_icu_bidi_third_party_dir = "//flutter/third_party/canvaskit/icu_bidi"

# Remove image codecs.
# In Chromium browsers, we can use the browser's built-in codecs.
skia_use_libjpeg_turbo_decode = false
skia_use_libpng_decode = false
skia_use_libwebp_decode = false
Expand Down
2 changes: 2 additions & 0 deletions tools/gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,8 @@ def to_gn_wasm_args(args, gn_args):
gn_args['skia_use_webgpu'] = False
gn_args['skia_use_libheif'] = False
gn_args['skia_use_libjpeg_turbo_encode'] = False
# TODO(yjbanov): https://github.com/flutter/flutter/issues/122759
# Remove this and implement it through Canvas2d.
gn_args['skia_use_libpng_encode'] = True
Copy link
Contributor

@yjbanov yjbanov Mar 15, 2023

Choose a reason for hiding this comment

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

Can you please file an issue to remove this as well? We can implement this by piping the image through a 2D canvas.

gn_args['skia_use_libwebp_encode'] = False
gn_args['skia_use_lua'] = False
Expand Down