[api-minor] Decode all JPEG images with the built-in PDF.js decoder in src/core/jpg.js
#11601
Conversation
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
from
e5413ed
to
b3952a7
Compare
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
7 times, most recently
from
7fbc0e5
to
636991c
Compare
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
7 times, most recently
from
79cff41
to
efca660
Compare
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
4 times, most recently
from
837ad7d
to
2e2dca3
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/c34edfcd473c2f6/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/4fcfc8db635eafa/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/c34edfcd473c2f6/output.txt Total script time: 19.67 mins
Image differences available at: http://54.67.70.0:8877/c34edfcd473c2f6/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/4fcfc8db635eafa/output.txt Total script time: 25.07 mins
Image differences available at: http://54.215.176.217:8877/4fcfc8db635eafa/reftest-analyzer.html#web=eq.log |
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
3 times, most recently
from
a7de12c
to
b11bf8a
Compare
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
from
b11bf8a
to
f9ce22c
Compare
|
The bug is fixed upstream, but I'm not sure if it made it to today's Nightly yet. Let's see... /botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b98a3f10a69802a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/c443edc5f08f38c/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/b98a3f10a69802a/output.txt Total script time: 26.08 mins
Image differences available at: http://54.67.70.0:8877/b98a3f10a69802a/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/c443edc5f08f38c/output.txt Total script time: 29.55 mins
Image differences available at: http://54.215.176.217:8877/c443edc5f08f38c/reftest-analyzer.html#web=eq.log |
…n `src/core/jpg.js` Currently some JPEG images are decoded by the built-in PDF.js decoder in `src/core/jpg.js`, while others attempt to use the browser JPEG decoder. This inconsistency seem unfortunate for a number of reasons: - It adds, compared to the other image formats supported in the PDF specification, a fair amount of code/complexity to the image handling in the PDF.js library. - The PDF specification support JPEG images with features, e.g. certain ColorSpaces, that browsers are unable to decode natively. Hence, determining if a JPEG image is possible to decode natively in the browser require a non-trivial amount of parsing. In particular, we're parsing (part of) the raw JPEG data to extract certain marker data and we also need to parse the ColorSpace for the JPEG image. - While some JPEG images may, for all intents and purposes, appear to be natively supported there's still cases where the browser may fail to decode some JPEG images. In order to support those cases, we've had to implement a fallback to the PDF.js JPEG decoder if there's any issues during the native decoding. This also means that it's no longer possible to simply send the JPEG image to the main-thread and continue parsing, but you now need to actually wait for the main-thread to indicate success/failure first. In practice this means that there's a code-path where the worker-thread is forced to wait for the main-thread, while the reverse should *always* be the case. - The native decoding, for anything except the *simplest* of JPEG images, result in increased peak memory usage because there's a handful of short-lived copies of the JPEG data (see PR 11707). Furthermore this also leads to data being *parsed* on the main-thread, rather than the worker-thread, which you usually want to avoid for e.g. performance and UI-reponsiveness reasons. - Not all environments, e.g. Node.js, fully support native JPEG decoding. This has, historically, lead to some issues and support requests. - Different browsers may use different JPEG decoders, possibly leading to images being rendered slightly differently depending on the platform/browser where the PDF.js library is used. Originally the implementation in `src/core/jpg.js` were unable to handle all of the JPEG images in the test-suite, but over the last couple of years I've fixed (hopefully) all of those issues. At this point in time, there's two kinds of failure with this patch: - Changes which are basically imperceivable to the naked eye, where some pixels in the images are essentially off-by-one (in all components), which could probably be attributed to things such as different rounding behaviour in the browser/PDF.js JPEG decoder. This type of "failure" accounts for the *vast* majority of the total number of changes in the reference tests. - Changes where the JPEG images now looks *ever so slightly* blurrier than with the native browser decoder. For quite some time I've just assumed that this pointed to a general deficiency in the `src/core/jpg.js` implementation, however I've discovered when comparing two viewers side-by-side that the differences vanish at higher zoom levels (usually around 200% is enough). Basically if you disable [this downscaling in canvas.js](https://github.com/mozilla/pdf.js/blob/8fb82e939cf0c8618a4e775ff17fc96f726872b5/src/display/canvas.js#L2356-L2395), which is what happens when zooming in, the differences simply vanish! Hence I'm pretty satisfied that there's no significant problems with the `src/core/jpg.js` implementation, and the problems are rather tied to the general quality of the downscaling algorithm used. It could even be seen as a positive that *all* images now share the same downscaling behaviour, since this actually fixes one old bug; see issue 7041.
With the changes in the previous patch, this is now dead code which should thus be removed.
…ocument` parameters, since it's now unused in the API With the changes in previous patches, the `disableCreateObjectURL` option/functionality is no longer used for anything in the API and/or in the Worker code. Note however that there's some functionality, mainly related to file loading/downloading, in the GENERIC version of the default viewer which still depends on this option. Hence the `disableCreateObjectURL` option (and related compatibility code) is moved into the viewer, see e.g. `web/app_options.js`, such that it's still available in the default viewer.
…task With the changes made in the previous patch, the `web/app_options.js` file no longer depends on anything *except* files residing in the `web/` folder. Hence the `gulp default_preferences` task can now be further simplified and thus becomes even faster than before; see also PR 11724.
Snuffleupagus
force-pushed
the
rm-nativeImageDecoderSupport
branch
from
5a0dd45
to
ebef67b
Compare
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/1e697729f942157/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/28ac54b94eeb884/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/28ac54b94eeb884/output.txt Total script time: 25.93 mins
Image differences available at: http://54.67.70.0:8877/28ac54b94eeb884/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/1e697729f942157/output.txt Total script time: 28.89 mins
Image differences available at: http://54.215.176.217:8877/1e697729f942157/reftest-analyzer.html#web=eq.log |
|
Thank you! This greatly simplifies the code and should improve performance. /botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/2d106f21a4793c1/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/5fe5e8787af7161/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/2d106f21a4793c1/output.txt Total script time: 24.14 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/5fe5e8787af7161/output.txt Total script time: 26.69 mins
|
Please refer to the individual commit messages.
Fixes #6984 (comment)
Fixes #7041