Slightly refactor the fontRef handling in PartialEvaluator_loadFont (issue 7403 and issue 7402)
#7347
Conversation
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/e8857c99c118ed5/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/9f6d1be36b6075f/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/e8857c99c118ed5/output.txt Total script time: 21.35 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/9f6d1be36b6075f/output.txt Total script time: 27.62 mins
|
|
/botio-linux lint |
From: Bot.io (Linux)ReceivedCommand cmd_lint from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/9e739d778b4dccf/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/9e739d778b4dccf/output.txt Total script time: 1.21 mins
|
Snuffleupagus
changed the title
fontRef handling in PartialEvaluator_loadFontfontRef handling in PartialEvaluator_loadFont (issue 7403)
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/4c32a3f426453c4/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/dc36290a342a652/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/4c32a3f426453c4/output.txt Total script time: 22.51 mins
Image differences available at: http://107.22.172.223:8877/4c32a3f426453c4/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/dc36290a342a652/output.txt Total script time: 27.80 mins
Image differences available at: http://107.21.233.14:8877/dc36290a342a652/reftest-analyzer.html#web=eq.log |
Snuffleupagus
changed the title
fontRef handling in PartialEvaluator_loadFont (issue 7403)fontRef handling in PartialEvaluator_loadFont (issue 7403 and issue 7402)
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/5ad3d68f90815c1/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/b1a39b6d552852a/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/b1a39b6d552852a/output.txt Total script time: 21.93 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/5ad3d68f90815c1/output.txt Total script time: 27.82 mins
|
|
@brendandahl Do you have time to review this PR? |
I'd guess that both Brendan and Yury are currently at https://wiki.mozilla.org/All_Hands/2016_London, so if anyone else wants to review this, feel free to :-) |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/e0cf8d88de4824c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/310cbb64ac3adba/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/310cbb64ac3adba/output.txt Total script time: 21.97 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/e0cf8d88de4824c/output.txt Total script time: 27.71 mins
|
|
The PR was rebased to fix a merge conflict in |
…t` (issue 7403 and issue 7402) Originally, I was just going to change this code to use `Ref_toString` in a couple more places. When I started reading the code, I figured that it wouldn't hurt to clean up a couple of comments. While doing this, I noticed that the logic for the (rare) `isDict(fontRef)` case could do with a few improvements. There should be no functional changes with this patch, but given the added reference checks, we will now avoid bogus `Ref`s when resolving font aliases. In practice, as issue 7403 shows, the current code can break certain PDF files even if it's very rare. Note that the only thing that this patch will change, is the `font.loadedName` in the case where a `fontRef` is a reference *and* the font doesn't have a descriptor. Previously for `fontRef = Ref(4, 0)` we'd get `font.loadedName = 'g_d0_f4_0'`, and with this patch `font.loadedName = g_d0_f4R`, which is actually one character shorted in most cases. (Given that `Ref_toString` contains an optimization for the `gen === 0` case, which is by far the most common `gen` value.) In the already existing fallback case, where the `fontName` is used to when creating the `font.loadedName`, we allow any alphanumeric character. Hence I don't see how (as mentioned above) e.g. `font.loadedName = g_d0_f4R` would be an issue here.
…osed to `Ref`s, to prevent re-rendering after `cleanup` from breaking (issue 7403 and issue 7402) Fonts that are not referenced by `Ref`s are very uncommon in practice, but it can unfortunately happen. In this case, we're currently not caching them in the usual way, i.e. by `Ref`, which leads to failures when a page is rendered after `cleanup` has run. The simplest solution would have been to remove the `font.translated` workaround, but since this would have meant loading these kind of fonts over and over, the patch attempts to be a bit clever about this situation. Note that if we instead loaded fonts per *page*, instead of per document, this issue wouldn't have existed.
|
The PR was rebased to fix a merge conflict in |
|
/botio makeref |
From: Bot.io (Linux)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 0 Live output at: http://107.21.233.14:8877/ac1028e9e801155/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 1 Live output at: http://107.22.172.223:8877/795021fb44cb055/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/795021fb44cb055/output.txt Total script time: 22.73 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/ac1028e9e801155/output.txt Total script time: 27.02 mins
|
Originally, I was just going to change this code to use
Ref_toStringin a couple more places. When I started reading the code, I figured that it wouldn't hurt to clean up a couple of comments. While doing this, I noticed that the logic for the (rare)isDict(fontRef)case could do with a few improvements.There should be no functional changes with this patch, but given the added reference checks, we will now avoid bogus
Refs when resolving font aliases.In practice, I don't think the current code has caused issues, but it seems good to clean this up to prevent any future problems.As issue #7403 shows, there're issues with the current code.Note that the only thing that this patch will change, is the
font.loadedNamein the case where afontRefis a reference and the font doesn't have a descriptor. Previously forfontRef = Ref(4, 0)we'd getfont.loadedName = 'g_d0_f4_0', and with this patchfont.loadedName = g_d0_f4R, which is actually one character shorted in most cases. (Given thatRef_toStringcontains an optimization for thegen === 0case, which is by far the most commongenvalue.)In the already existing fallback case, where the
fontNameis used to when creating thefont.loadedName, we allow any alphanumeric character. Hence I don't see how (as mentioned above) e.g.font.loadedName = g_d0_f4Rwould be an issue here.Edit: This also fixes #7403 and #7402.