Add support for midnight mode with color

[smithzvk]

Not sure if the other PDF-Tools repo is dead on not so reposting here (originally at politza/pdf-tools#698)

This inversion method attempts to maintain the color hue and saturation but inverts the lightness using the OKLab color space.

About the OKLab color space: https://bottosson.github.io/posts/oklab/

This addresses feature request #608 (in politza's repo)

Original image:

Image in midnight mode with pdf-tools-midnight-invert set to true (hues maintained but lightness inverted):

[vedang]

Thank you for this work. Please rebase it on top of the latest master so that CI tests will run correctly. I will review and test it over the weekend

[smithzvk]

Got it passing the CI except appveyor, but that seems to have been broken for other commits recently so I suspect that isn't my change.

[vedang]

Yup, Appveyor isn't because of your change. Thanks for this. Will review and take forward as soon as time permits

…

On Thu, Jan 13, 2022, 10:14 AM Zach Kost-Smith ***@***.***> wrote: Got it passing the CI except appveyor, but that seems to have been broken for other commits recently so I suspect that isn't my change. — Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUUHVZXPFNAI6DY2DYJ53UVZKERANCNFSM5LTGA7XQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[smithzvk]

Just bumping to see if there are any concerns and to say that if there is an issue or just a dislike, let me know and maybe I can find a way to make this work within your desires for the project.

[vedang]

I haven't had time to review thoroughly and test on my own yet @smithzvk . There is no action item on you, just on me to make the time to merge this in / get back .

Sorry for the delay, I'll get this done as soon as possible.

[vedang]

Hi @smithzvk ,

Thank you for this work and sorry for the delay in the review. I've added a few comments. Can you please make the requested changes and push new commits?

Thanks,
Vedang

[vedang]

Since :render/usecolors is no longer a boolean, there needs to be an explanation of what the three supported values indicate, as a comment for future devs

[smithzvk]

Let me know if more is desired here.

[vedang]

Valid values for usecolors are now 0, 1 and 2 (not just boolean). These need to be explained in the code. Perhaps a switch-case with comments and a default case to throw errors would be great.

[smithzvk]

I used internal_error() here. Let me know if this is not the right method for throwing an error. No return value here to pass errors back up the stack.

[vedang]

I didn't understand why this is not part of the rgb2oklab function itself.

[smithzvk]

Added comments for clarity. The inversion isn't part of the color space conversion and should be separate.

The gamma correction on the other hand... I don't understand the need for it at all given that the lightness should be a linear w.r.t how we perceive the lightness (the entire point of the color conversion), but I perceive that with the correction yields better results.

As a blanket statement here, I tried a lot of different methods to achieve this affect but this yielded the best results to date and, save for that gamma correction, at least seems to theoretically be the right thing to do.

[vedang]

Hi @smithzvk ,

Not sure if you got a notification for the review comments I left above. No rush on making the actual fixes, but do let me know a tentative timeline by when you'll be able to address the comments

[smithzvk]

I did get a notification. I'm looking through them this morning and try to update on an expected ETA.

[smithzvk]

I've been using this pretty much exclusively lately. I think it is pretty good, but I will note that there are instances where colors turn out less than ideal. In particular, some journal articles use a blue for the links which, for whatever reason, isn't very highly legible in the inverted recolored image, but the standard midnight mode works well. In addition, this can be a bit slow for older machines... incorporating a SIMD path would likely resolve this slight slowness.

I feel like this is a 90% solution right now. If you think we should hold off, I can attempt to resolve as time permits.

Also, let me know if you would like me to rebase to master or combine commits etc, or of course feel free to do so yourself. I'm picky about repo history, so respect if you have similar pet peeves.

[vedang]

Took a quick look at the changes and they look good to me! Thanks!

Will review them in some more detail when time permits, test a bit over the next weekend and then merge the changes in.

I'll do a rebase and fixup the commits into a single commit.

[vedang]

I want to set expectations here: The change looks good to me, and I've planned to merge it into the 1.1.0 milestone release of pdf-tools. This means that I will merge the changes into master after I cut a 1.0.0 release, which I have planned for sometime this month (pending testing across all the changes).

No code change is expected here, thank you for your patience :). I will post updates on the ticket as soon as I start merging the changes in.

[vedang]

Changes look good to me. Thank you!

[bhankas]

Hello, anything keeping these changes from being merged?

[vedang]

I want to cut a stability release before I cut a new features release. Apologies for the delay on it , I'll get to it soon!

…

On Sat, Sep 24, 2022, 6:59 PM bhankas ***@***.***> wrote: Hello, anything keeping these changes from being merged? — Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUUHXPERMRJ4UPQWLA3ITV7363LANCNFSM5LTGA7XQ> . You are receiving this because you commented.Message ID: ***@***.***>

[vedang]

I have finally merged this change in, and it is now the default behavior of pdf-view-midnight-minor-mode. Thank you!

[smithzvk]

Great news. Thank you for your work continuing this project. Definitely my favorite PDF reader.

…

On Tue, Jan 17, 2023 at 12:35 AM Vedang Manerikar ***@***.***> wrote: I have finally merged this change in, and it is now the default behavior of pdf-view-midnight-minor-mode. Thank you! — Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACIUR3AP5BEYYWW6JWW4QLWSY4SRANCNFSM5LTGA7XQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Atreyagaurav]

I have a problem that the values of pdf-view-midnight-colors are being ignored in the pdf, I feel like this is due to this thing. It was fine, and I hadn't updated my packages for a while, when I updated this and the ignoring of the midnight color coincides, and looking at this and seeing the code made me feel like this is related. Can you test it and confirm it?

It's really great to see the color brightness inversion, I had always wondered if that was possible and would be a good way to invert things. I really love this.

[smithzvk]

Yes, you are right. When pdf-view-midnight-invert is enabled, the values in pdf-view-midnight-colors are ignored as the colors are based on the original PDF content. Unfortunately, at least until someone comes up with something very clever, you will need to choose between the invert behavior and the original behavior which allows you to match your color theme.

With the latest change to make the invert mode the default, this is going to impact people who have been relying on the original behavior. But all you need to do right now is set pdf-view-midnight-invert to nil to restore the original behavior.

[Atreyagaurav]

I like the new behavior in the color stuffs, it's really great. It's life-changing lol.

The high contrast for fg and bg color is the problem, One idea I can think of is to make the original pdf-view-midnight-colors's lightness as thresholds (min and max) for new lightness calculated by the oklab method, theoretically it should work for reducing the contrast for all colors not just black and white. Someone who's implemented it might have an easier job integrating it. Otherwise, I can look it up to see if I can do that when I have time.

[fw623]

The changes proposed in #169 respect the pdf-view-midnight-colors (at the time I didn't know that the changes in #69 don't), so feel free to use the changes there directly or as inspiration, if you're interested in implementing this.

[Atreyagaurav]

I think we can add a line to make sure oklab.l is between the l value of fg and bg here, what do you think? And add in documentations that pdf-view-midnight-colors will be used to determine the lightness in this case, but those colors will be used as it is in the other case. This should help with not letting the contrast be too high, but I'm not sure about the ramification of that. I thought about changing the precomputed white and black that way, but that might not work with colors that are close to them, so I think this implementation should be better.

[smithzvk]

I wasn't aware that there was another concurrent PR for the same fix. I can take a look and reviewing that change and whether it can be adapted to what was merged, hopefully addressing the contrast issue. However I'll be busy at least until the weekend, so I'm not sure when I can look at it.

…

On Wed, Mar 1, 2023, 11:54 Zero ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In server/epdfinfo.c <#69 (comment)>: > + } + else if (color_equal(precomputed_rgb, rgb)) + { + rgb = precomputed_inv_rgb; + } + else + { + struct color oklab = rgb2oklab(rgb); + precomputed_rgb = rgb; + + /* Gamma correction. Shouldn't be necessary, but colors + * 'feel' too dark and fonts too thin otherwise. */ + oklab.l = pow(oklab.l, 1.8); + + /* Invert the perceived lightness */ + oklab.l = 1.0 - oklab.l; I think we can add a line to make sure oklab.l is between the l value of fg and bg here, what do you think? And add in documentations that pdf-view-midnight-colors will be used to determine the lightness in this case, but those colors will be used as it is in the other case. This should help with not letting the contrast be too high, but I'm not sure about the ramification of that. I thought about changing the precomputed white and black that way, but that might not work with colors that are close to them, so I think this implementation should be better. — Reply to this email directly, view it on GitHub <#69 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACIURZGKTEPRD6FB4LT6VDWZ6EMDANCNFSM5LTGA7XQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Atreyagaurav]

This seems to work for me:

--- a/epdfinfo.c
+++ b/epdfinfo-new.c
@@ -502,6 +502,9 @@ image_recolor (cairo_surface_t * surface, const PopplerColor * fg,
     .g = bg->green / f,
     .b = bg->blue / f
   };
+  const double fg_l = a[0] * rgb_fg.r + a[1] * rgb_fg.g + a[2] * rgb_fg.b;
+  const double bg_l = a[0] * rgb_bg.r + a[1] * rgb_bg.g + a[2] * rgb_bg.b;
+  const double diff_l = fg_l - bg_l;
 
   const struct color rgb_diff = {
     .r = rgb_bg.r - rgb_fg.r,
@@ -576,9 +579,9 @@ image_recolor (cairo_surface_t * surface, const PopplerColor * fg,
                      * 'feel' too dark and fonts too thin otherwise. */
                     oklab.l = pow(oklab.l, 1.8);
 
-                    /* Invert the perceived lightness */
-                    oklab.l = 1.0 - oklab.l;
-
+                    /* Invert the perceived lightness, and scales it */
+                    oklab.l = bg_l + diff_l * (1.0 - oklab.l);
+                   
                     rgb = oklab2rgb(oklab);
 
                     precomputed_inv_rgb = rgb;

Here, the bottom part of the image is when I have inverted the PDF.

This is on: (setq pdf-view-midnight-colors '("gray70" . "gray25")), it works normally if you have (setq pdf-view-midnight-colors '("white" . "black"))

If you think just this much is fine, I can make a pull request, but if you want to test it on other cases to see if it's the correct way, then feel free to use the part from the patch. This should at least act as a band-aid for the sudden change to high contrast rendering that doesn't respect user configurations.