Use a BMP decoder when resizing an image #18918

calixteman · 2024-10-18T17:57:11Z

The image decoding won't block the main thread any more.
For now, it isn't enabled for Chrome because issue6741.pdf leads to a crash.

The image decoding won't block the main thread any more. For now, it isn't enabled for Chrome because issue6741.pdf leads to a crash.

calixteman · 2024-10-18T17:57:48Z

/botio browsertest

moz-tools-bot · 2024-10-18T17:57:50Z

From: Bot.io (Linux m4)

Received

Command cmd_browsertest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/89dce42e9880aed/output.txt

moz-tools-bot · 2024-10-18T17:57:50Z

From: Bot.io (Windows)

Received

Command cmd_browsertest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/623d99422ac60d8/output.txt

moz-tools-bot · 2024-10-18T18:19:06Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/89dce42e9880aed/output.txt

Total script time: 21.25 mins

Regression tests: FAILED

  errors: 120
  different ref/snapshot: 16
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/89dce42e9880aed/reftest-analyzer.html#web=eq.log

moz-tools-bot · 2024-10-18T18:27:10Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/623d99422ac60d8/output.txt

Total script time: 29.31 mins

Regression tests: FAILED

  different ref/snapshot: 2396

Image differences available at: http://54.193.163.58:8877/623d99422ac60d8/reftest-analyzer.html#web=eq.log

Snuffleupagus

Do you have benchmarking results that show (clear) improvements with this patch?

Have you reported the Chrome-crash to the browser developers?

Snuffleupagus · 2024-10-21T08:33:50Z

src/shared/util.js

      return shadow(this, "platform", {
+        isChrome: !isFirefox && !!window.chrome,


Unfortunately this isn't safe, since code in src/shared/ will run on both the main- and worker-threads (and in Node.js) so you cannot access window here.

Can we please just revert the changes in this file (I'll make a few other suggestions)?

Snuffleupagus · 2024-10-21T08:34:10Z

web/app.js

@@ -1045,6 +1045,7 @@ const PDFViewerApplication = {
    const loadingTask = getDocument({
      ...apiParams,
      ...args,
+      isChrome: FeatureTest.platform.isChrome,


Please revert this.

Suggested change

isChrome: FeatureTest.platform.isChrome,

Snuffleupagus · 2024-10-21T08:36:10Z

src/display/api.js

@@ -177,6 +177,7 @@ const DefaultStandardFontDataFactory =
 *   `OffscreenCanvas` in the worker. Primarily used to improve performance of
 *   image conversion/rendering.
 *   The default value is `true` in web environments and `false` in Node.js.
+ * @property {boolean} [isChrome] - Determines if we can use bmp ImageDecoder.


Suggested change

* @property {boolean} [isChrome] - Determines if we can use bmp ImageDecoder.

* @property {boolean} [isChrome] - Determines if we can use bmp ImageDecoder.

* NOTE: Temporary option until [link to Chromium bug report] is fixed.

Snuffleupagus · 2024-10-21T08:42:51Z

src/display/api.js

@@ -281,6 +282,7 @@ function getDocument(src = {}) {
    typeof src.isOffscreenCanvasSupported === "boolean"
      ? src.isOffscreenCanvasSupported
      : !isNodeJS;
+  const isChrome = src.isChrome === true;


To avoid issues, and to support this properly when running tests, can we please change it as follows?

Suggested change

const isChrome = src.isChrome === true;

const isChrome =

typeof src.isChrome === "boolean"

? src.isChrome

: (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) &&

!FeatureTest.platform.isFirefox &&

typeof window !== "undefined" &&

!!window?.chrome;

Snuffleupagus · 2024-10-21T08:45:34Z

src/core/image_resizer.js

+    let decoder, imagePromise;
+
+    // TODO: remove the isChrome, once Chrome isn't crashing anymore with
+    // isssue6741.pdf.


Suggested change

// isssue6741.pdf.

// issue6741.pdf.

Snuffleupagus · 2024-10-21T08:47:14Z

src/core/image_resizer.js

+    // TODO: remove the isChrome, once Chrome isn't crashing anymore with
+    // isssue6741.pdf.
+    if (
+      (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) ||


Unfortunately I don't believe that enabling this unconditionally in Firefox is safe, since this can be disabled with a preference and that preference is only enabled by default in Nightly: https://searchfox.org/mozilla-central/rev/ec342a3d481d9ac3324d1041e05eefa6b61392d2/modules/libpref/init/StaticPrefList.yaml#3241-3245

Use a BMP decoder when resizing an image

4a69d96

The image decoding won't block the main thread any more. For now, it isn't enabled for Chrome because issue6741.pdf leads to a crash.

timvandermeij added core image-conversion labels Oct 19, 2024

This was referenced Oct 20, 2024

Update dependencies and translations to the most recent versions #18921

Merged

Investigate test differences on the bots #18928

Open

Snuffleupagus requested changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a BMP decoder when resizing an image #18918

Use a BMP decoder when resizing an image #18918

calixteman commented Oct 18, 2024

calixteman commented Oct 18, 2024

moz-tools-bot commented Oct 18, 2024

moz-tools-bot commented Oct 18, 2024

moz-tools-bot commented Oct 18, 2024

moz-tools-bot commented Oct 18, 2024

Snuffleupagus left a comment

Snuffleupagus Oct 21, 2024

Snuffleupagus Oct 21, 2024

Snuffleupagus Oct 21, 2024

Snuffleupagus Oct 21, 2024 •

edited

Loading

Snuffleupagus Oct 21, 2024

Snuffleupagus Oct 21, 2024

		return shadow(this, "platform", {
		isChrome: !isFirefox && !!window.chrome,

	* @property {boolean} [isChrome] - Determines if we can use bmp ImageDecoder.
	* @property {boolean} [isChrome] - Determines if we can use bmp ImageDecoder.
	* NOTE: Temporary option until [link to Chromium bug report] is fixed.

-  const isChrome = src.isChrome === true;
+  const isChrome =
+    typeof src.isChrome === "boolean"
+      ? src.isChrome
+      : (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) &&
+        !FeatureTest.platform.isFirefox &&
+        typeof window !== "undefined" &&
+        !!window?.chrome;

Use a BMP decoder when resizing an image #18918

Are you sure you want to change the base?

Use a BMP decoder when resizing an image #18918

Conversation

calixteman commented Oct 18, 2024

calixteman commented Oct 18, 2024

moz-tools-bot commented Oct 18, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Oct 18, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Oct 18, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Oct 18, 2024

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024

Choose a reason for hiding this comment

Snuffleupagus Oct 21, 2024 •

edited

Loading