Improves rendering performance of annotation layers. #5214
Conversation
|
I noticed this when looking at possible performance bottlenecks in #5207 - That PDF has many annotations, so this PR should help. Besides from making the dom lighter it also saves an 3 element array in all cases where no annotation color was set. |
|
I should also mention that I almost have a PR ready for a complete rewrite of the annotation layer, and these issues are also resolved in that PR. I'll try to put it up tomorrow so we can see how we can go from there (I only have to combine the 50+ commits into two or three sensible ones). In that PR the Link annotation will be implemented (with even more support than we currently have for special borders etc.) and if we like the structure, we can easily extend it for other annotation types such as Text annotations. |
|
cool, And with the array I save, I mean that in case color is not an array (to be precise not an array with 3 elements) then it will not just default to set it to But I am happy to dump my changes and perhaps improve yours. The current code just does quite a lot of unnecessary work. |
|
Oh, i also had planned a follow up PR. because as far as I see the annotationHighlight div is generated but never ever used (its hidden) the highlight is implemented with hover css. We should check that as well. |
|
while this PR only aims at specific performance improvements, all of them are covered by #5218, great work @timvandermeij - depending on when #5218 will get merged we can merge or abandon this. |
|
@CodingFabian Could you rebase this PR? Let's try to land this first as my PR requires some more work and has to be rebased anyway. |
|
sure, i didnt want to spend time if this makes your work more difficult. if you are ok with rebasing and landing, i will do that |
CodingFabian
force-pushed
the
optimize-annotation-layer
branch
from
4e6e51c to
3c17063
Compare
|
rebased. |
|
@CodingFabian Looks really good. One thing I have found: see #5218 (diff) and http://logs.glob.uno/?c=mozilla%23pdfjs&s=20+Aug+2014&e=20+Aug+2014#c23865 for reference. For that PR we did a lot of research and found that Adobe Reader defaults back to black if the C entry is missing. The spec only talks about the case where the array has 0 elements, but it does not say what to do if it's missing entirely. Otherwise we regress https://pdf.yt/d/XBjrpaYuieax9D6b (also compare that one to Adobe Reader to see that it falls back to a black border). Could you update that for this PR, i.e., make a distinction between the empty array and missing array case? |
|
yes you are right. i did not update this pr because i thought we might abandon it or rebase it onto yours. |
CodingFabian
force-pushed
the
optimize-annotation-layer
branch
from
3c17063 to
d8b003e
Compare
|
thanks for bringing this up @timvandermeij - should be good now, right? |
|
Yes, that's also what I had in mind. I'll go over it one more time tomorrow and then we can merge this. Thanks! |
| // colorspaces | ||
| data.color = color; | ||
| } else { | ||
| if (color === null) { |
There was a problem hiding this comment.
For https://pdf.yt/d/XBjrpaYuieax9D6b color yields undefined. Please change line 81 to var color = dict.get('C') || null; to fix this issue and check that this document renders correctly then.
There was a problem hiding this comment.
i made the null check a !color
This change does the following: * Address TODO to remove getEmptyContainer helper. * Not set container bg-color. The old code is incorrect, causing it to not have any effect. It sets color to an array (item.color) rather css string. Also in most cases it would set it to black background which is incorrect. * only add border instructions when there is actually a border * reduce memory consumption by not creating new 3 element arrays for annotation colors. In fact according to spec, this would be incorrect, as the default should be "transparent" for an empty array. Adobe Reader interprets a missing color array as black however. Note that only Link annotations were actually setting a border style and color. While Text annotations might have calculated a border they did not color it. This behaviour is now controlled by the boolean flag.
CodingFabian
force-pushed
the
optimize-annotation-layer
branch
from
d8b003e to
9796351
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/567feedb80265f7/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/567feedb80265f7/output.txt Total script time: 0.76 mins Published
|
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/d31a9d64e7c94d5/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/84f218e6c7263ab/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/84f218e6c7263ab/output.txt Total script time: 17.33 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/d31a9d64e7c94d5/output.txt Total script time: 23.26 mins
|
Improves rendering performance of annotation layers.
|
Thank you! |
…-layer Improves rendering performance of annotation layers.
This change does the following:
not have any effect. It sets color to an array (item.color) rather
css string. Also in most cases it would set it to black background
which is incorrect.
annotation colors. In fact according to spec, this would be incorrect,
as the default should be "transparent".
Note that only Link annotations were actually setting a border style and
color. While Text annotations might have calculated a border they did
not color it. This behaviour is now controlled by the boolean flag.