Skip to content
Snippets Groups Projects

Freetext annotation: respect the font when there is DA but no AP

This fixes #6 (closed)

Merge request reports

Pipeline #4324 passed

Pipeline passed for 1ad0eff6 on sander:fix-font-in-freetext-annotation-without-ap

Approval is optional

Closed by Albert Astals CidAlbert Astals Cid 6 years ago (Sep 27, 2018 12:02pm UTC)

Merge details

  • The changes were not merged into master.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • mentioned in issue #248

  • Oliver Sander added 2 commits

    added 2 commits

    • 82035cfb - Use Object for the fontname, rather than std::string
    • b5930e45 - Error out right away if font subdictionary is not a dictionary

    Compare with previous version

  • Oliver Sander resolved all discussions

    resolved all discussions

  • Here are two more commits that implement the requested changes. I suggest to squash them all into one before merging.

  • Oliver Sander added 21 commits

    added 21 commits

    • b5930e45...93b21498 - 17 commits from branch poppler:master
    • fec6efbe - Freetext annotation: respect the font when there is DA but no AP
    • dfa51a06 - Use Object for the fontname, rather than std::string
    • 885f4950 - Error out right away if font subdictionary is not a dictionary
    • b137e922 - Merge branch 'fix-font-in-freetext-annotation-without-ap' of…

    Compare with previous version

  • (Sorry for unresolving these two discussions... I tried to specifically not do that but it seems that GitLab does not even let me resolve them explicitly now. Again, sorry for noise.)

  • Oliver Sander added 2 commits

    added 2 commits

    • e54112b6 - Simplify code by letting Dict::lookup do the dereferencing
    • 0d0c425a - Remove redundant check

    Compare with previous version

  • Oliver Sander mentioned in merge request !10 (closed)

    mentioned in merge request !10 (closed)

  • Oliver Sander resolved all discussions

    resolved all discussions

  • Can you please rebase this?

  • Oliver Sander added 37 commits

    added 37 commits

    • 0d0c425a...4f039c57 - 31 commits from branch poppler:master
    • b01d1287 - Freetext annotation: respect the font when there is DA but no AP
    • bd5e7991 - Use Object for the fontname, rather than std::string
    • 29b75c91 - Error out right away if font subdictionary is not a dictionary
    • 3ec8aff0 - Simplify code by letting Dict::lookup do the dereferencing
    • e9cce5d6 - Remove redundant check
    • 905b8493 - Merge branch 'fix-font-in-freetext-annotation-without-ap' of…

    Compare with previous version

  • Oliver Sander added 1 commit

    added 1 commit

    • 0d349832 - Do not include <string>, we are not actually using it

    Compare with previous version

  • Done!

  • pdftoppm crashes when run with this patch and the file attached to this comment. 2211.pdf

    #0  0x00007ffff7e8934c in std::__atomic_base<int>::operator++ (this=0x28) at /usr/include/c++/8.2.0/bits/atomic_base.h:295
    #1  Dict::incRef (this=0x0) at /home/tsdgeos/devel/poppler/poppler/Dict.h:107
    #2  Object::copy (this=0x55555565d9c0) at /home/tsdgeos/devel/poppler/poppler/Object.cc:104
    #3  0x00007ffff7e893b4 in Object::fetch (this=this@entry=0x55555565d9c0, xref=<optimized out>, recursion=recursion@entry=0) at /home/tsdgeos/devel/poppler/poppler/Object.cc:124
    #4  0x00007ffff7e1df11 in Dict::lookup (this=this@entry=0x55555565d8b0, key=key@entry=0x7ffff7f0b79a "Resources", recursion=recursion@entry=0) at /home/tsdgeos/devel/poppler/poppler/Dict.cc:181
    #5  0x00007ffff7e434a4 in Gfx::drawAnnot (this=this@entry=0x555555646a30, str=str@entry=0x7fffffffdcd0, border=border@entry=0x0, aColor=0x55555565a1b0, xMin=487, yMin=yMin@entry=203, xMax=xMax@entry=511, yMax=yMax@entry=227, rotate=rotate@entry=0)
        at /home/tsdgeos/devel/poppler/poppler/Gfx.cc:5282
    #6  0x00007ffff7e09d0e in AnnotText::draw (this=0x55555566f480, gfx=0x555555646a30, printing=<optimized out>) at /home/tsdgeos/devel/poppler/poppler/Annot.cc:2490
    #7  0x00007ffff7e8efc4 in Page::displaySlice (this=0x5555555c2a30, out=0x5555555a85f0, hDPI=<optimized out>, vDPI=<optimized out>, rotate=0, useMediaBox=<optimized out>, crop=<optimized out>, sliceX=0, sliceY=0, sliceW=1275, sliceH=1651, printing=false, 
        abortCheckCbk=0x0, abortCheckCbkData=0x0, annotDisplayDecideCbk=0x0, annotDisplayDecideCbkData=0x0, copyXRef=false) at /home/tsdgeos/devel/poppler/poppler/Annot.h:1658
    #8  0x0000555555557c50 in savePageSlice (ppmFile=0x555555659640 "bla-30.png", pg_h=1650.0000000000002, pg_w=<optimized out>, h=<optimized out>, w=<optimized out>, y=<optimized out>, x=0, pg=30, splashOut=0x5555555a85f0, doc=0x5555555a6de0)
        at /home/tsdgeos/devel/poppler/utils/pdftoppm.cc:287
    #9  main (argc=<optimized out>, argv=<optimized out>) at /home/tsdgeos/devel/poppler/utils/pdftoppm.cc:600
  • The backtrace looks like the Object::Object(Dict*) constructor is called with nullptr somewhere...

    • The problem seems to be that there is code calling Annot::createForm with resDict == null which due to the old check for if (resDict) yielded no resource dictionary. But with the forwarding in place, the check if (resDictObject.isDict()) is true but resDictObject.getDict() == nullptr.

      So either the check should become if (resDictObject.isDict() && resDictObject.getDict() != nullptr) or (what I would prefer personally) the forwarding constructor passes on resDict ? Object(resDict) : Object().

      Edited by Adam Reichold
    • Thank's for debugging this.

      or (what I would prefer personally) the forwarding constructor passes on resDict ? Object(resDict) : Object()

      Agree, did that. Imho constructing Object of type objDict with inner Dict = nullptr is a bug in the first place, and should be spotted in first place, if at all. E.g., let class Object Ctors call OBJECT_TYPE_CHECK. Or let them construct a objNull Object if the inner object is nullptr.

    • Note that the type was correct in this case, i.e. we created and used an instance of type objDict, just with the inner pointer being nullptr, so OBJECT_TYPE_CHECK wouldn't help AFAIU.

      This however does not seem to be valid, e.g. the destructors of Object definitely assume dict to be non-null:

      case objDict:
          if (!dict->decRef()) {
            delete dict;
          }
          break;

      I'll create a separate MR for asserting these pointers in the constructors (which is how I debugged this) and we can discuss there whether we like this or not.

    • @haxtibal I created !38 (closed) to discuss these issues separately.

    • Please register or sign in to reply
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading