Repository navigation
Compute operator bounding boxes in the worker - #22289
calixteman wants to merge 3 commits into
Conversation
Viewer previewCommit e92cb3b (build logs).
Previews are removed when the PR closes and may be pruned after 30 days without an update. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22289 +/- ##
==========================================
+ Coverage 90.52% 90.64% +0.11%
==========================================
Files 278 280 +2
Lines 68586 68764 +178
==========================================
+ Hits 62090 62331 +241
+ Misses 6496 6433 -63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 13.42%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
343cf11 to
3207050
Compare
3207050 to
932615e
Compare
932615e to
6f38415
Compare
Snuffleupagus
left a comment
Snuffleupagus
left a comment
There was a problem hiding this comment.
It looks like this made Util.multiplyByDOMMatrix unused, so that can probably be removed now?
6f38415 to
99ccb6b
Compare
99ccb6b to
34f4838
Compare
Propagate bounds to operator dependencies and send packed boxes with the final operator-list chunk. Expose them through OperatorBBoxes.
Migrate operation filters and debugger highlights to worker bounds. Remove display-side bounding-box tracking and recordOperations.
34f4838 to
e92cb3b
Compare
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
This PR breaks the dependency highlight in the stepper view, which really helped me debug problems while first developing this feature.
(when hovering on step 15 of issue16263.pdf):
| Master | This PR |
|---|---|
|
|
Also, some bboxes in that issue16263.pdf got larger, e.g. step 33. Larger bboxes just means that it's less optimized, but I don't know if they got larger because of a bug that could cause problems elsewhere:
| Master | This PR |
|---|---|
|
|
| return bbox; | ||
| } | ||
| const operatorList = font.charProcOperatorList?.get(id); | ||
| const index = operatorList?.fnArray.indexOf(OPS.setCharWidthAndBounds); |
There was a problem hiding this comment.
Is this trustworthy? Or can a Type 3 font lie about its bounds? Previously we were actually executing the char's operator list to get its full bbox.
| ); | ||
| return !(renderingIntent & RenderingIntentFlag.DISPLAY) | ||
| ? null | ||
| : (this._intentStates.get(cacheKey)?.operatorList?.bboxes ?? null); |
There was a problem hiding this comment.
_intentStates gets empties by #tryCleanup(). Could we still need the list of bboxes after that? I'm not sure about when cleanup happens, but maybe we can still zoom in after that? Before .recordedBBoxes was just a class property so it was staying around.
| return info; | ||
| } | ||
| const fontMatrix = font?.fontMatrix || FONT_IDENTITY_MATRIX; | ||
| const fontBBox = font?.bbox; |
There was a problem hiding this comment.
The previous version was reading the font bbox, but then checking if it was actually trustworthy:
pdf.js/src/display/canvas_dependency_tracker.js
Lines 705 to 706 in bfe4cca
The reason I added that is because some fonts were missing a bbox, others were lying. Would it be possible to keep that fallback? Maybe by drawing in an OffscreenCanvas?
An example of a font without a bbox are the "standard" ones (from https://github.com/mozilla/pdf.js/blob/master/test/pdfs/standard_fonts.pdf), where PDF.js is kind of expected to know their properties because the PDF only references them by name. You can see e.g. op 1406 from the first page of that PDF, rendering "A" in Courier. With this patch it gets a full-page bbox, while before it had the correct one.
There was a problem hiding this comment.
Maybe we could somehow reuse the logic from #22419?
But a good first step would be to hard-code bboxes for those well-known fonts.
| return bbox; | ||
| } | ||
| const operatorList = font.charProcOperatorList?.get(id); | ||
| const index = operatorList?.fnArray.indexOf(OPS.setCharWidthAndBounds); |
There was a problem hiding this comment.
If I remember correctly, type 3 fonts can have ops that have dependencies on the outer PDF. We'd need to traverse this operatorList to look for ops that have them.
|
This file shows a regression with tracking of the SMask bbox. With this PR, if you zoom it on the white part it will turn red because it skips the |
Compute operator bounds and propagate them to dependencies in the worker. Send packed bounds to the display layer for partial rendering and debugger highlights.
Remove display-side bounding-box tracking and recordOperations, and enable optimized partial rendering by default.