Re-factor the imageCache used in PartialEvaluator.getOperatorList

[Snuffleupagus]

This changes the imageCache from a simple object to a class, which will allow (future) support for re-sizing of large images.
Compared to my initial attempt(s) at implementing this, this patch purposely avoids trying to fix "everything" at once, since that required too large/invasive changes to the relevant code.

Please note: This patch, on its own, won't completely unblock PR #9255. However once this patch has landed, that PR can be re-factored on top of it and the new ImageCache class extended to properly support re-sized images.

[timvandermeij]

I'm seeing a bit of unused features in this code. I assume you'll extend this later, but I think it would help to add this to e.g., the commit message so we know what is to be updated later on, or preferably add it later when it actually becomes relevant. For example, dimnesions, ctm and transform are in ImageCache, but don't seem to be passed in/used inside the class anywhere. Perhaps I'm missing a bit of context here though; I think preparation work is fine (such as using parameter objects), but I'm not entirely sure about adding unused properties/features already when they will only be used later.

[Snuffleupagus]

[...] but I think it would help to add this to e.g., the commit message so we know what is to be updated later on,

While I've got a basic idea of how it all will fit together in the end, I couldn't write this down without completely implementing the follow-up first. (Initially I tried to do "everything" at once, which just turned into a giant mess.)

or preferably add it later when it actually becomes relevant.

The "later" here would be more-or-less immediately, since I should only need a free weekend to wire everything up for PR #9255 once the foundation is in place.

My apologies if this comes off as grumpy, but I've already spent a fair amount of time (and with more to come) essentially fixing someone else's PR here. Honestly, tagging the original issue as 5-good-beginner-bug probably wasn't such a great idea in hindsight :-(

---

Edit: I suppose the only thing I could possibly do here is to put this PR on hold until I've had time to actually implement the follow-up patch (on top of this PR and #9255), such that it's possible to provide a more complete picture. (Everything should still land separately, of course.)

[Snuffleupagus]

/botio test

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/56c2677929a149e/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/189f2651d536152/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.67.70.0:8877/56c2677929a149e/output.txt

Total script time: 18.03 mins

• Font tests: Passed
• Unit tests: Passed
• Regression tests: Passed

[pdfjsbot]

From: Bot.io (Windows)

---

Success

Full output at http://54.215.176.217:8877/189f2651d536152/output.txt

Total script time: 25.67 mins

• Font tests: Passed
• Unit tests: Passed
• Regression tests: Passed

[timvandermeij]

My apologies if this comes off as grumpy, but I've already spent a fair amount of time (and with more to come) essentially fixing someone else's PR here.

I understand, and I think it's really nice of you to help out with getting that PR ready to merge! I'm fine with doing the unit tests in a follow-up since I can see it's easier to do that once the final code is in place. The assertions will do for now.

I suppose the only thing I could possibly do here is to put this PR on hold until I've had time to actually implement the follow-up patch (on top of this PR and #9255), such that it's possible to provide a more complete picture.

I think that's a good idea. I also think it's easier to review once the code is complete too. Obviously we can still land this in these chunks.

[Snuffleupagus]

I suppose the only thing I could possibly do here is to put this PR on hold until I've had time to actually implement the follow-up patch (on top of this PR and #9255), such that it's possible to provide a more complete picture. (Everything should still land separately, of course.)

I've not yet gotten around to do this, on account of having a cold with a slight fever for the last couple of days, but hopefully will during the next weekend.

Also, a more general reason for all of this taking time is not only because of it being quite complex and involving both the main/worker-thread, but also me wanting to try to implement something that with a minimum of effort can be used if/when other kinds of image data will be downsized in the future.

[timvandermeij]

I've not yet gotten around to do this, on account of having a cold with a slight fever for the last couple of days, but hopefully will during the next weekend.

No problem; please take care of getting better first, that's much more important!

[...] to try to implement something that with a minimum of effort can be used if/when other kinds of image data will be downsized in the future.

That's actually a good idea as we have seen multiple attempts for other image kinds as well.

[Snuffleupagus]

Note for possible follow-up: This really doesn't make sense, since whether or not caching will actually happen depends on the existence of a cacheKey; should this be imgData.cached = !!cacheKey; instead?

[Snuffleupagus]

While continuing to work on this, I'm come across a few different pre-existing inefficiencies that have led to PRs such as #10644 and #10647, both of which are relevant to memory usage.

Currently I'm trying to come up with the best way of ensuring that any kind of downsized image data, no matter its type or how it's sent from the worker, will correctly cause the main-thread caches to be cleared such that re-rendering at a different scale won't break. At the moment, I'm still not sure exactly how/where to implement this.
Somewhat related to the above, but still a separate issue, is adding non-production/test-only asserts to ensure that certain image types won't ever be accidentally downsized in the future. At the moment, it's also not clear to me how best to implement this.

[Snuffleupagus]

Sigh; after now having spent more time than I'd like to think about getting PR #9255 into shape for landing, I've now found that the approach in that PR is truly broken for many existing test-cases :-(

Unfortunately I made the mistake of not running all tests locally with only the patch from PR #9255 applied, before starting to hack on this. However, I'd assumed that PR #9255 was actually tested locally during development, but it seems pretty clear to me at this point that that was not actually the case :-(

The problem, which is very evident from the existing reference tests, is that many ImageMasks are simply downsized way too much and thus become blurry; for images containing "text" this is especially noticeable.

This is as far as I got: master...Snuffleupagus:ImageCache, which should work if it weren't for the fact that PR #9255 is broken; and honestly it makes no sense for me to sink even more time into this unless PR #9255 actually works and passes all tests first.

This rant is probably in clear violation of the newly added code of conduct, but it's just so frustrating spending time trying to fix something that was broken by design; sorry about this.

[timvandermeij]

I agree. To move forward, let's close this and first fix up the original PR in terms of functionality before spending more time on figuring out caching. Thank you for your efforts though; offering to help out with a "stuck" PR is really nice and in the process you did manage to find and fix some other issues related to this, which is really useful in general.