Add a color-scheme option to pdf.js

[Gusted]

Hi everyone reading this!

This is my first PR for pdf.js so a fair warning that I might have done things that are considered not "best practices".
This PR is mainly to resolve #2071 as I'm getting tired of whipping out a external PDF Viewer to not eyestrain while reading PDF's.

Having this automatically implemented within the browser(Firefox) is a feature I really loved to see, and thereby currently extensions like Dark Reader(DISCLAIMER: I'm the maintainer) cannot access the pdf files on Firefox, so it was basically my only option to see such feature in Firefox.

I'm really looking towards fixing issues being raised and answering how this whole thing actually works and of course the moment that's being merged ;).

Preview:

Regards,
Gusted

[Snuffleupagus]

Thank you for looking into issue #2071.
Based on a quick look, I've got some reservations about the implementation in this PR:

• First of all you should not be changing the UI of the viewer itself it in this way, since there's already a preference viewerCssTheme used for that purpose and the toolbar buttons shouldn't be modifying the viewer itself (only the actual PDF document).
• Secondly, looking at the actual color-inversion part of the actual PDF document, I'm a bit worried about the maintainability of all of this new code (since it's e.g. completely untested) and also about any possible performance impact.

Can we not simplify all of this considerably, by using a CSS-only approach (there's some discussion about that in the issue) to just invert the colours after the fact and thus remove a lot of overall complexity?

[Gusted]

* First of all you should _not_ be changing the UI of the viewer it in this way, since there's already a preference `viewerCssTheme` used for that purpose and the toolbar buttons shouldn't be modifying the viewer itself (only the actual PDF document).

As a new-user I would "kind of" expect if I select such option to have the whole UI change with it. But no hassle to remove that bit of code.

Secondly, looking at the actual color-inversion part of the actual PDF document

Luckily that code is only used for images as they somehow will need to be changed to - I actually haven't really got much of testing with that code and the images as I only could find a 1BIT_Images.pdf which is working as intended with that code.
About the code in speaking, it's a much more simplifies version of something we use in Dark Reader, to have a "natural" color inversion with some options to play with the sepia/brightness.

Also it's used for the setStrokeRGBColor and setFillRGBColor this is mainly to satisfy the "graphs" on the default loaded .pdf as they set a white color as background and on top of that paint the graph, which will look worse when painted without any color inversion.

So that part is pure to satisfy images-like and graphs on documents.

Can we not simplify all of this considerably, by using a CSS-only approach

I've looked into it, and I'm not really satisfied by the result, especially because invert and hue-rotate will caus the render-engine(Both chromium and firefox) to have this purple-ish sub-pixels on rendering text and some other elements. It's a side-effect of that approach which is something that my currently native pdf-viewer does and that doesn't satisfy my eyes either. That's why I went with this approach to not have such side-effect and have a more natural feeling.

[Snuffleupagus]

As a new-user I would "kind of" expect if I select such option to have the whole UI change with it. But no hassle to remove that bit of code.

Sorry, but anything related to the existing viewerCssTheme handling would need to be removed.

Luckily that code is only used for images as they somehow will need to be changed to - I actually haven't really got much of testing with that code and the images as I only could find a 1BIT_Images.pdf which is working as intended with that code.

There's a lot of existing test-cases available in https://github.com/mozilla/pdf.js/tree/master/test/pdfs, and https://github.com/mozilla/pdf.js/wiki/Contributing explains how to run tests (doing so will also download all the linked test-cases as well).

When I spoke about the maintainability/complexity, I didn't mean just the colour conversion code added in this PR but mostly the overall changes being made in the src/display/canvas.js file.

Also it's used for the setStrokeRGBColor and setFillRGBColor this is mainly to satisfy the "graphs" on the default loaded .pdf as they set a white color as background and on top of that paint the graph, which will look worse when painted without any color inversion.

Is there also not a risk that all the added checks, in various (potentially hot) parts of the src/display/canvas.js code could have a performance impact for all documents even in the "normal" colour mode?

I've looked into it, and I'm not really satisfied by the result,

Maybe a CSS-only approach isn't perfect, but would it not be "good enough" for many cases!?

[Gusted]

Sorry, but anything related to the existing viewerCssTheme handling would need to be removed.

No problems, fixed.

Is there also not a risk that all the added checks, in various (potentially hot) parts of the src/display/canvas.js code could have a performance impact for all documents even in the "normal" colour mode?

for "normal" colour mode, it shouldn't be a problem, modern and even older browsers are very fast at checking properties for truthiness. For when "dark" colour mode is enabled, it will yes iterate over the images chunks and over the pixels, that is the only "big" hassle I could see when bench marking, the modifyColor is pretty fast and has caching implemented where available and should be for the setStrokeRGBColor and setFillRGBColor not a big problem in my eyes.

Maybe a CSS-only approach isn't perfect, but would it not be "good enough" for many cases!?

If the PDF is simply a text-only. With some title's. Yeah it would be a suit-able? Other then the rendering of purple-ish sub-pixels. But if even a small images or graphs comes into place, I wouldn't even remotely recommending it.

As for the test-cases, it seems like gulp test doesn't run the gulp unittest command that explicitly test for all test cases, Ill be looking into running those unittests on the code.

[Gusted]

I've had some success with the unittest after finally downloading them all(got a lot of times where network got dropped), luckily they are all passing with the current changes, however I imagine they will also need to be checked on dark-mode? Any recommended way to do so?

[Gusted]

fd22262 removes the modifying images code. Thus results to less more code being changed in src/display/canvas.js and now being minimal, and only does the modifying color of stroke/fill and actually make sure that the drawn text is being the correct color.

[Gusted]

4e04a74 makes the color_utils.js more minimal and combined with the previous commit removes around ~200 LOC with having minimal impact on the PR.

[Snuffleupagus]

for "normal" colour mode, it shouldn't be a problem, modern and even older browsers are very fast at checking properties for truthiness.

Apart from the readability/maintainability aspect of all this (untested) code being sprinkled throughout the src/display/canvas.js file, there's (even in the last version) also the potential performance issue of the repeated and for normal rendering completely unnecessary updating of the canvas-context fillStyle for every single text-rendering operation. That really doesn't feel great at all, as far as I'm concerned!

Again, for all of the reasons already mentioned, a CSS-only solution definitely seem to be the way to go here. /cc @timvandermeij What's your opinion on this patch?

[Gusted]

also the potential performance issue of the repeated and for normal rendering completely unnecessary updating of the canvas-context fillStyle for every single text-rendering operation.

I could check for the current fillStyle/strokeStyle and only update when necessarry but these 2 properties are being modified and not being reset(my understanding) and I don't have any assurance when the text is being rendered that the correct styles are the correct text-color.

That really doesn't feel great at all, as far as I'm concerned!

I get that, I don't like it either.

a CSS-only solution definitely seem to be the way to go here.

This is going to be a agree to disagree situation on that.

[timvandermeij]

I agree with @Snuffleupagus that this is quite a lot of code for something that should be much simpler to address with CSS. It might not give perfect results, but to be honest looking at https://user-images.githubusercontent.com/25481501/124488836-d8b1c680-dd9f-11eb-9241-3b189003b24f.png doesn't quite look good to me as well. I find the text quite hard to read because because of the color change; the letters aren't "crisp" anymore and look like they consist of multiple colors of pixels instead of being white.

Actually, I think the images from the simple CSS invert solution in #2071 look much better to me. If I look at https://user-images.githubusercontent.com/37630203/116760785-7a3b3580-aa48-11eb-9051-443a33d9402a.png the text is much more "crisp" and evenly colored, which makes me wonder why the CSS solution wouldn't just suffice here?!

[Gusted]

, which makes me wonder why the CSS solution wouldn't just suffice here?!

Honestly, I've worked quite hard on this system that now the PR is, took me a good couple of days. And the CSS Filter doesn't really spark interest in me. Because any issues raised with them will be way way harder to fix. Because such filter affect every element at once and not individually. Thereby this is just an opinion that the filter is way too dark, but that can be fixed, to be more inline with the dark cssViewerTheme.

About the "crisp", my current setup with fonts and Firefox isn't the best and shouldn't be used for reference, the "broken" crispness is to fix some issue with my current monitor.

Il get this PR changed a CSS alternative, thank you all for the reviewing of the code, I really do appreciate it.

[Gusted]

So, 1 of the downside of filter, any colors, won't have the original "color", we mainly resolve that with the hue-rotate(180deg) which gives you a slightly different color of the original but still in the same spectrum. However adding such filter-function will cause this "crisp" on text-rendering.

So it's either no crisp but have incorrect-coloring.
Or have crisp on text but have better coloring which makes more sense.

[Gusted]

Comparison:

Better coloring but crisp:

Incorrect coloring but no crisp:

[Filip62]

I'm a little bit disappointed that @Gusted's work isn't getting attention anymore.

He was clearly listening to your opinions and putting work in. Both of the images above look good enough to me, either option is so much better than nothing.

Many people would really appreciate to have either implementation.

[burtonator]

Isn't there a better way to implement this to take the PDF internally and swap the default colors so that the background and foreground text colors are different?

This way pictures would look normal.

[Gusted]

Isn't there a better way to implement this to take the PDF internally and swap the default colors so that the background and foreground text colors are different?

This way pictures would look normal.

Ah well, that was my first thought too(see earlier comments) and it was the implementation I had created at first. However the pdf.js team disagrees with this and prefers to use filter which makes these pictures looks weird.

If the pdf.js team still wants to use this CSS implementation rather than the modifying color(which is still in the commit history) then I will be closing the PR as I don't like nor want that kind of implementation be the way it should be, it's poor and not the correct way to do this. I don't want my name signed off on this feature with such a poor implementation.

[avatar-lavventura]

@Gusted

Why dark-theme is not completely black on the background?

[Gusted]

Why dark-theme is not completely black on the background?

Pure black isn't natural to look at, it's only interesting for people with a amoled screen and most of those are phone users anyway. Looking at a lighter shade of black makes it more natural to look at than pure black.

[avatar-lavventura]

Pure black isn't natural to look at, it's only interesting for people with a amoled screen and most of those are phone users anyway. Looking at a lighter shade of black makes it more natural to look at than pure black.

I believe pure-black is healtier for the eye and easier to detect white fonts. If you try https://darkreader.org to read pdf, it does uses pure black on the end.

[shivaprsd]

I made an add-on for the viewer that does this (and a bit more). Live demo here (color scheme can be changed from the toolbar).

It employs the same tactic as @Gusted's original patch, of modifying the Canvas drawing on the go. However instead of patching the PDF.js source I wrote it as an external script. Clearly there is a performance impact (probably more than Gusted's). But as Gusted said the CSS-filter method is suboptimal experience-wise (my add-on includes it only as a fallback option).

It's not pretty, a bit too fancy (what it does) and, as @Snuffleupagus said, a lot of complexity for the maintainers. So I didn't imagine it getting merged anyway, and that's where stuff like this would actually be ideal as add-ons. It's really unfortunate Firefox add-ons don't work on the built-in viewer.

I guess I might be able put together a custom viewer with my add-on on top and make it into a Firefox extension, and make it the default PDF viewer.. but then it'd be unnecessary bloat, and also elicit the refrain oft-repeated here:

... that it not just be an unmodified version. Please re-skin it or build upon it.

Can we have the maintainers' take on this? I hope I am clear that my only intention is to extend the default viewer, not to "monkey patch" it. This seems to be the only way, at least till PDF.js natively supports add-ons.

It does what I want (view PDFs in eye-friendly colors), and I don't mind the performance hit most of the time. It seems a lot of users desire the same and IMHO we are being held back by mere technicalities here.

[Snuffleupagus]

Let's close this PR for now, given the differing opinions and perspectives regarding how this could be implemented.
(As mentioned previously, an implementation that requires adding a bunch of branches/checks throughout the src/display/canvas.js file will unfortunately have negative effects on overall readability, maintainability, and performance of the code.)

Please note that #2071 is still open for tracking this issue.

[shivaprsd]

I guess I might be able put together a custom viewer with my add-on on top and make it into a Firefox extension

https://github.com/shivaprsd/doqment