Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23519006:
pdf: write only ToUnicode mappings needed by the font, trimming anything out of [firstChar, lastCha… (Closed)
|
Created:
7 years, 3 months ago by edisonn Modified:
7 years, 1 month ago Reviewers:
vandebo (ex-Chrome) CC:
skia-review_googlegroups.com, reed1, epoger Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Descriptionpdf: write only ToUnicode mappings needed by the font, trimming anything out of [firstChar, lastChar] interval. (checked in from another client) - closed
Patch Set 1 #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 3
Patch Set 8 : #Patch Set 9 : #
Total comments: 12
Patch Set 10 : #Patch Set 11 : #
Total comments: 9
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #
Messages
Total messages: 13 (0 generated)
part1 of a probably long series of updates to make poppler warnings go away.
Just nits. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: while (i < bfchar.count() && bfchar[i].fGlyphId < firstGlypthID) { While the end point probably doesn't take a lot of time, this one could. Probably worth the effort to figure out SkTSearch and use it here. At that point you might as well use it below as well. Or you could do both outside the loop and note the start and end indices. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode429 src/pdf/SkPDFFont.cpp:429: if (count == 0) { Not possible, count must be >= 1 because of the check on line 417 https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode451 src/pdf/SkPDFFont.cpp:451: while (i < bfrange.count() && bfrange[i].fEnd < firstGlypthID) { Same here re SkTSearch https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode474 src/pdf/SkPDFFont.cpp:474: cmap->writeHexAsText(j == 0 ? start : bfrange[i + j].fStart, 4); It's probably the same computational cost to do the trinary operator as it is to just inline the SkMax32 here. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode517 src/pdf/SkPDFFont.cpp:517: uint16_t firstGlypthID = 0x0000, No need for defaults, these args are always passed in.
https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: while (i < bfchar.count() && bfchar[i].fGlyphId < firstGlypthID) { N/A - moved filtering in the caller - fastest way to do something is not to do it at all. On 2013/09/04 22:07:53, vandebo wrote: > While the end point probably doesn't take a lot of time, this one could. > Probably worth the effort to figure out SkTSearch and use it here. At that > point you might as well use it below as well. Or you could do both outside the > loop and note the start and end indices. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode429 src/pdf/SkPDFFont.cpp:429: if (count == 0) { On 2013/09/04 22:07:53, vandebo wrote: dead code Done. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode451 src/pdf/SkPDFFont.cpp:451: while (i < bfrange.count() && bfrange[i].fEnd < firstGlypthID) { dead code On 2013/09/04 22:07:53, vandebo wrote: > Same here re SkTSearch https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode474 src/pdf/SkPDFFont.cpp:474: cmap->writeHexAsText(j == 0 ? start : bfrange[i + j].fStart, 4); On 2013/09/04 22:07:53, vandebo wrote: > It's probably the same computational cost to do the trinary operator as it is to > just inline the SkMax32 here. dead code https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode517 src/pdf/SkPDFFont.cpp:517: uint16_t firstGlypthID = 0x0000, On 2013/09/04 22:07:53, vandebo wrote: > No need for defaults, these args are always passed in. Done.
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + 1; ++i) { How about just making this loop go from firstGlyphID to min(count + 1, lastGlyphID) ?
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + 1; ++i) { Done, but the solution is a little bit more complicated because of the logic inside the loop, as we need to start with a character before, and stop we we got out of a range. On 2013/09/06 21:22:55, vandebo wrote: > How about just making this loop go from firstGlyphID to min(count + 1, > lastGlyphID) ?
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + 1; ++i) { On 2013/09/09 14:36:53, edisonn wrote: > Done, but the solution is a little bit more complicated because of the logic > inside the loop, as we need to start with a character before, and stop we we got > out of a range. I don't see it, why do you need to start with the character before? > > On 2013/09/06 21:22:55, vandebo wrote: > > How about just making this loop go from firstGlyphID to min(count + 1, > > lastGlyphID) ? > https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:507: i < count && I think this line is meant to terminate the range on the last iteration of the loop. Probably easiest/best to set count to SkMax32(count, lastGlyphID) and maybe rename it. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:511: BFRange range = {(uint16_t)SkMax32(firstGlyphID, With the loop changed, I don't think you need any of the changes here. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:526: } else if (i > lastGlyphID && !inSubset && !inRange) { or here
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 16:04:50, vandebo wrote: > I think this line is meant to terminate the range on the last iteration of the > loop. Probably easiest/best to set count to SkMax32(count, lastGlyphID) and > maybe rename it. in this case i < count is to protect on OutOfBounds the next line. Of I rename and change cunt, code does not get simpler, and the loop continues to iterate anyway, because we do not break. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:511: BFRange range = {(uint16_t)SkMax32(firstGlyphID, On 2013/09/09 16:04:50, vandebo wrote: > With the loop changed, I don't think you need any of the changes here. I need it, I start from firstGlypthID - 1 https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:526: } else if (i > lastGlyphID && !inSubset && !inRange) { On 2013/09/09 16:04:50, vandebo wrote: > or here This one i need, we go to max(lastGlyphID, count)
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 16:48:41, edisonn wrote: > On 2013/09/09 16:04:50, vandebo wrote: > > I think this line is meant to terminate the range on the last iteration of the > > loop. Probably easiest/best to set count to SkMax32(count, lastGlyphID) and > > maybe rename it. > > in this case i < count is to protect on OutOfBounds the next line. Of I rename > and change cunt, code does not get simpler, and the loop continues to iterate > anyway, because we do not break. But why do you think the loop goes to count + 1? The last unicode data is at count. The loop goes around once more to close the range. Both inRange and inSubset have checks for i < count, so they will both be false in that last loop. If a range is open, it will be closed, if not a new one will not open because of the values of inRange and inSubset. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:511: BFRange range = {(uint16_t)SkMax32(firstGlyphID, On 2013/09/09 16:48:41, edisonn wrote: > On 2013/09/09 16:04:50, vandebo wrote: > > With the loop changed, I don't think you need any of the changes here. > > I need it, I start from firstGlypthID - 1 I don't see why you start at first - 1, please explain. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:526: } else if (i > lastGlyphID && !inSubset && !inRange) { On 2013/09/09 16:48:41, edisonn wrote: > On 2013/09/09 16:04:50, vandebo wrote: > > or here > This one i need, we go to max(lastGlyphID, count) Isn't this break redundant with the loop condition?
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 17:13:11, vandebo wrote: > On 2013/09/09 16:48:41, edisonn wrote: > > On 2013/09/09 16:04:50, vandebo wrote: > > > I think this line is meant to terminate the range on the last iteration of > the > > > loop. Probably easiest/best to set count to SkMax32(count, lastGlyphID) and > > > maybe rename it. > > > > in this case i < count is to protect on OutOfBounds the next line. Of I rename > > and change cunt, code does not get simpler, and the loop continues to iterate > > anyway, because we do not break. > > But why do you think the loop goes to count + 1? The last unicode data is at > count. The loop goes around once more to close the range. Both inRange and > inSubset have checks for i < count, so they will both be false in that last > loop. If a range is open, it will be closed, if not a new one will not open > because of the values of inRange and inSubset. if (i < count && foo[i]) woks if (i < max(count, bar) && foo[i]) will assert if bar > count count = max(count, bar) if (i < count && foo[i]) asserts too. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:511: BFRange range = {(uint16_t)SkMax32(firstGlyphID, On 2013/09/09 17:13:11, vandebo wrote: > On 2013/09/09 16:48:41, edisonn wrote: > > On 2013/09/09 16:04:50, vandebo wrote: > > > With the loop changed, I don't think you need any of the changes here. > > > > I need it, I start from firstGlypthID - 1 > > I don't see why you start at first - 1, please explain. Correct, it is not needed. https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:526: } else if (i > lastGlyphID && !inSubset && !inRange) { On 2013/09/09 17:13:11, vandebo wrote: > On 2013/09/09 16:48:41, edisonn wrote: > > On 2013/09/09 16:04:50, vandebo wrote: > > > or here > > This one i need, we go to max(lastGlyphID, count) > > Isn't this break redundant with the loop condition? no, the loop goes to max(last, count), while this if breaks if we exceeded last, and there is no rand not subset Without the if, the loop with continue to cycle doing nothing until max(count, last)
optimized the code to do the minimum amount of work, this include processing last glyph + one more, those +2 at the end of for
https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()) + 2; These changes seem to work for me and are closer to what's already here: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()); https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:498: for (int i = firstGlyphID; i < limit; ++i) { i < limit + 1 https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:499: bool inSubset = i < glyphToUnicode.count() && i < limit && https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:509: i < glyphToUnicode.count() && i <= lastGlyphID && i < limit && https://codereview.chromium.org/23519006/diff/38001/tests/ToUnicode.cpp File tests/ToUnicode.cpp (right): https://codereview.chromium.org/23519006/diff/38001/tests/ToUnicode.cpp#newco... tests/ToUnicode.cpp:20: printf("A %i %i\n", (int)(offset + len), (int)data->size()); Remove debug lines
https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()) + 2; It does not work with the new tests I added. Please apply the ToUnitode.cpp test. the issue is that count requires a < operator, while lastGlyph requires a <= operator. To make it work I can use SkMin32(lastGlyphID + 1, glyphToUnicode.count()); This works and makes the code simpler, but I do not like it because the code is based on a technicality, and the idea why we use +1 is lost in the code, and the readability is lost from the code, it is like a compiled code for perf and beauty On 2013/09/09 20:00:29, vandebo wrote: > These changes seem to work for me and are closer to what's already here: > > const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()); https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:498: for (int i = firstGlyphID; i < limit; ++i) { On 2013/09/09 20:00:29, vandebo wrote: > i < limit + 1 Done. https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:509: i < glyphToUnicode.count() && i <= lastGlyphID && On 2013/09/09 20:00:29, vandebo wrote: > i < limit && Done.
If you like patch set 13, I'm fine with it - LGTM. https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new... src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()) + 2; On 2013/09/10 14:50:11, edisonn wrote: > It does not work with the new tests I added. Please apply the ToUnitode.cpp > test. > the issue is that count requires a < operator, while lastGlyph requires a <= > operator. > > To make it work I can use SkMin32(lastGlyphID + 1, glyphToUnicode.count()); > > This works and makes the code simpler, but I do not like it because the code is Fair, I think this can be fixed by using another variable or two and working on variable names... const int lastUnicodeIndex = glyphToUnicode.count() - 1; const int endIndex = SkMin32(lastGlyphID, lastUnicodeIndex) + 1; // Loop around once more after endIndex in order to close any open range for (int i ... ? > based on a technicality, and the idea why we use +1 is lost in the code, and the > readability is lost from the code, it is like a compiled code for perf and > beauty > > > On 2013/09/09 20:00:29, vandebo wrote: > > These changes seem to work for me and are closer to what's already here: > > > > const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()); > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
