Fix rendering broken on some browsers

[DonikaV]

Fixes rendering broken on some browsers by adding willReadFrequently: true to canvas.getContext()

Closes #1010

See mozilla/pdf.js#14641 (comment)

[1Jesper1]

Nice, maybe we could do a browser check? Chrome and Edge have the problem, Firefox doesn't.

[1Jesper1]

If merged, could this also be merged into the v5 branch?

[1Jesper1]

I tested it, seems to work!

[1Jesper1]

Workaround till merge: Use https://www.npmjs.com/package/patch-package
react-pdf version "5.7.2"

EDIT: See #1207 (comment)

diff --git a/node_modules/react-pdf/dist/esm/Page/PageCanvas.js b/node_modules/react-pdf/dist/esm/Page/PageCanvas.js
index 3fbdbc7..b9b01a8 100644
--- a/node_modules/react-pdf/dist/esm/Page/PageCanvas.js
+++ b/node_modules/react-pdf/dist/esm/Page/PageCanvas.js
@@ -80,7 +80,7 @@ export var PageCanvasInternal = /*#__PURE__*/function (_PureComponent) {
         annotationMode: renderForms ? ANNOTATION_MODE.ENABLE_FORMS : ANNOTATION_MODE.ENABLE,
 
         get canvasContext() {
-          return canvas.getContext('2d');
+					return canvas.getContext('2d', { willReadFrequently: true });
         },
 
         viewport: renderViewport
diff --git a/node_modules/react-pdf/dist/umd/Page/PageCanvas.js b/node_modules/react-pdf/dist/umd/Page/PageCanvas.js
index 3915fe0..d08bb59 100644
--- a/node_modules/react-pdf/dist/umd/Page/PageCanvas.js
+++ b/node_modules/react-pdf/dist/umd/Page/PageCanvas.js
@@ -108,7 +108,7 @@ var PageCanvasInternal = /*#__PURE__*/function (_PureComponent) {
         annotationMode: renderForms ? ANNOTATION_MODE.ENABLE_FORMS : ANNOTATION_MODE.ENABLE,
 
         get canvasContext() {
-          return canvas.getContext('2d');
+					return canvas.getContext('2d', { willReadFrequently: true });
         },
 
         viewport: renderViewport
diff --git a/node_modules/react-pdf/src/Page/PageCanvas.jsx b/node_modules/react-pdf/src/Page/PageCanvas.jsx
index 5384ee2..b741500 100644
--- a/node_modules/react-pdf/src/Page/PageCanvas.jsx
+++ b/node_modules/react-pdf/src/Page/PageCanvas.jsx
@@ -109,8 +109,8 @@ export class PageCanvasInternal extends PureComponent {
     const renderContext = {
       annotationMode: renderForms ? ANNOTATION_MODE.ENABLE_FORMS : ANNOTATION_MODE.ENABLE,
       get canvasContext() {
-        return canvas.getContext('2d');
-      },
+        return canvas.getContext('2d', { willReadFrequently: true });
+			},
       viewport: renderViewport,
     };
     if (canvasBackground) {

[callabr1]

Hi sorry to spam all threads @DonikaV & @1Jesper1; I got a dev to add this change to one of our test environments and I gave it a try.

This MR fixed some documents, but not all. I created a bunch of text documents with various fonts and converted to PDF, those are all now fixed - however many of the PDFs that clients are using are still seeing the issue. Seems if there's a lot of images on page it will render with the issue, but it's difficult to find the exact trigger...

Here are some sample documents that were fixed with these changes:
donquijote.pdf
Test Fonts PDF.pdf
pdf-sample.pdf

And here's some sample documents where the issue persists after the changes were applied:
Sample - 1.pdf
uglyDoc.pdf

Also I have this file, where the top rendered better with the fix but the bottom still has the issue:
differentRender.pdf

Here's some screenshots:
Sample - 1.pdf post-fix:

differentRender.pdf post-fix:

[1Jesper1]

@callabr1 Thanks for testing, interesting to see the change does not fix all problems. Let's hope the canvas bug in Chrome will be fixed soon!

[DonikaV]

@wojtekmaj could you please check this PR and merge? Thanks.

[1Jesper1]

mozilla/pdf.js#14641 (comment) Maybe we need to also add willReadFrequently to PDFJS node_module? Seems to work... @DonikaV @callabr1

[wojtekmaj]

What worries me is that with HW acceleration on some users already have performance issues, and by forcing software rendering on all users we will increase the problem. Perhaps we could make it into a flag 🤔

[wojtekmaj]

I also don't understand why PDF.js itself doesn't use that option. It does, however, use alpha: false 🤔

https://github.com/mozilla/pdfjs-dist/blob/eb245b8de89c8d631d175ae937136de54ea3ed51/lib/web/pdf_page_view.js#LL657C16-L659C8

[1Jesper1]

@wojtekmaj This is the info for the alpha attribute, I don't now if it's relevant.

alpha
A boolean value that indicates if the canvas contains an alpha channel. If set to false, the browser now knows that the backdrop is always opaque, which can speed up drawing of transparent content and images.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext

[1Jesper1]

@wojtekmaj To fix the bug for all our users we now use patch-package on react-pdf and pdfjs-dist to patch all occurrences of canvas.getContext('2d' to use , { willReadFrequently: true });

[callabr1]

What worries me is that with HW acceleration on some users already have performance issues, and by forcing software rendering on all users we will increase the problem. Perhaps we could make it into a flag 🤔

This is also concerning to me, I've been trying my best to test on all browsers in order to ensure no regressions arise from turning HW acceleration on but if it's dependent on the Graphics Card I only really have 2 or 3 viable setups to test with. I will try my best to regression test, I have a few other QAs with varied machines that can also test, but there's still a risk that fixing this for the specific cards breaks PDF rendering for other cards.

@wojtekmaj To fix the bug for all our users we now use patch-package on react-pdf and pdfjs-dist to patch all occurrences of canvas.getContext('2d' to use , { willReadFrequently: true });

Thanks @1Jesper1 we will try this when we can. I haven't been able to test much this week as we are about to push our last major release of the year, so I don't want to put anything in there that could negatively impact end-users more than the current bug does.
Once we release and my QA environment is updated to the unreleased version I will test out putting that line on all occurrences of canvas.getContext and report back here.

[Greg-Hitchon]

@1Jesper1 Do you mind sharing the patch doc on pdfjs-dist?

Edit: mine looked something like this (using an older version of react-pdf)

diff --git a/node_modules/pdfjs-dist/build/pdf.js b/node_modules/pdfjs-dist/build/pdf.js
index 4316828..1eb555b 100644
--- a/node_modules/pdfjs-dist/build/pdf.js
+++ b/node_modules/pdfjs-dist/build/pdf.js
@@ -1635,7 +1635,7 @@ class BaseCanvasFactory {
 
     return {
       canvas,
-      context: canvas.getContext("2d")
+      context: canvas.getContext("2d", { willReadFrequently: true })
     };
   }
 
@@ -4428,7 +4428,7 @@ exports.FontLoader = FontLoader;
 
       canvas.width = 1;
       canvas.height = 1;
-      const ctx = canvas.getContext("2d");
+      const ctx = canvas.getContext("2d", { willReadFrequently: true });
       let called = 0;
 
       function isFontReady(name, callback) {
@@ -12401,7 +12401,8 @@ class TextLayerRenderTask {
     canvas.height = canvas.width = DEFAULT_FONT_SIZE;
     canvas.mozOpaque = true;
     this._layoutTextCtx = canvas.getContext("2d", {
-      alpha: false
+      alpha: false,
+      willReadFrequently: true
     });
 
     if (this._textContent) {

[1Jesper1]

@Greg-Hitchon
patch-package of react-pdf version "5.7.2"
react-pdf+5.7.2.patch
patch-package of pdfjs-dist version "2.12.313"
pdfjs-dist+2.12.313.patch

[MatanYemini]

@wojtekmaj Can you just merge it and publish a new version? it starts to get weird where I have already seen (including myself 7 copies of the same) 🙏🏾 :)

[wojtekmaj]

I'm parking this for now - see #1010 (comment) for reasoning.

[MatanYemini]

@wojtekmaj Tried with latest 6.2.2 and still having some issues:

[cle1000]

@wojtekmaj +1

[wojtekmaj]

I'm closing this since the defaults appear to be working for most folks and we now have customRenderer option which enables you to overwrite the entire canvas rendering process anyway, if you really need it.