Pdfstream as cmap #264

fakabbir · 2019-07-12T23:33:56Z

Extension of work done in #228.
Encapsulates initialization of cmap in a setter function.

pietermarsman

Looking good! I have some small remarks though, and @ganeshtata also already made some small remarks on #228.

pietermarsman · 2019-07-14T15:54:25Z

pdfminer/pdffont.py

+        if type(name) is PDFStream:
+            if 'CMapName' in name:
+                name = name.get('CMapName').name
+                if name in('DLIdent-H','OneByteIdentityH','Identity-H') :


@ganeshtata made some suggestions about this: #228 (comment).

pietermarsman · 2019-07-14T15:55:10Z

pdfminer/pdffont.py

+            name = 'unknown'
+        if type(name) is PDFStream:
+            if 'CMapName' in name:
+                name = name.get('CMapName').name


@ganeshtata made some suggestions about this: #228 (comment)

I agree that overwriting name is confusing.

pietermarsman · 2019-07-14T16:00:31Z

pdfminer/pdffont.py

-            if strict:
-                raise PDFFontError(e)
-            self.cmap = CMap()
+        self.cmap = (spec, strict)


This is the first time a @property is used in this repository. Do you have any special reason to introduce it here? Otherwise I would suggest keeping it simple and don't use properties.

Suggested change

self.cmap = (spec, strict)

self.cmap = self.get_cmap_from_spec(spec, strict)

pietermarsman · 2019-07-14T16:02:52Z

This PR solves #210

pombredanne · 2019-07-15T13:48:09Z

@fakabbir could you also add some tests?

pietermarsman · 2019-07-15T20:31:27Z

Yes, if you add one ore more tests this PR is totally ready for merging.

Suggestion for test (the first is not working on master):

from pdfminer.cmapdb import IdentityCMap
from pdfminer.pdffont import PDFCIDFont
from pdfminer.pdftypes import PDFStream


def test_pdf_cid_font_with_pdfstream_as_cmap_name():
    stream = PDFStream({}, 'Identity-H')

    spec = {'Encoding': stream}
    font = PDFCIDFont(None, spec)

    assert isinstance(font.cmap, IdentityCMap)


def test_pdf_cid_font_with_string_as_cmap_name():
    spec = {'Encoding': 'Identity-H'}
    font = PDFCIDFont(None, spec)

    assert isinstance(font.cmap, IdentityCMap)

pietermarsman · 2019-07-22T20:39:32Z

Looking good. I have still a few questions left though:

In fa40043 you remove the mapping from different cmap names to standard cmap names. I think this changes the behavior, is this on purpose?
For example, earlier 'DLIdentit-H' was mapped to 'Identity-H' which returned IdentityCMap(WMode=0) from CMapDB.get_cmap(). With this PR it returns CMap().

-CMAP_ENCODER = {
-    'DLIdent-H': 'Identity-H',
-    'OneByteIdentityH': 'Identity-H',
-    'Identity-H': 'Identity-H',
-    'DLIdent-V': 'Identity-V',
-    'OneByteIdentityV': 'Identity-V',
-    'Identity-V': 'Identity-V'
-}
+IDENTITY_ENCODER = ('Identity-H', 'Identity-V')

Also, I have some remarks in the review.

pdfminer/pdffont.py

tests/test_pdfencoding.py

+        spec = {'Encoding': PSLiteral('Identity-V')}
+        font = PDFCIDFont(None, spec)
+        assert isinstance(font.cmap, IdentityCMap)
+


fakabbir · 2019-07-24T06:26:26Z

pdfminer/pdffont.py

+            if strict:
+                raise PDFFontError('Encoding is unspecified')
+            cmap_name = 'unknown'
+        if type(cmap_name) is PDFStream:


def literal_name(x): if not isinstance(x, PSLiteral): if settings.STRICT: raise PSTypeError('Literal required: %r' % (x,)) else: name=x else: name=x.name if six.PY3: try: name = str(name,'utf-8') except: pass return name

def get_cmap_from_spec(self, spec, strict): try: spec_encoding = spec['Encoding'] if hasattr(spec_encoding, 'name'): cmap_name = literal_name(spec['Encoding']) else: cmap_name = literal_name(spec_encoding['CMapName']) except KeyError: if strict: raise PDFFontError('Encoding is unspecified') cmap_name = 'unknown' if type(cmap_name) is PDFStream: if 'CMapName' in cmap_name: cmap_name = cmap_name.get('CMapName').name else: if strict: raise PDFFontError('CMapName unspecified for encoding') cmap_name = 'unknown' if cmap_name in IDENTITY_ENCODER: return CMapDB.get_cmap(cmap_name) else: return CMap()

This is never true since literal_name always returns a str

If x is not an instance of PSLiteral it will return the __repr__(if repr exist). __repr__ function actually doesn't return a type str. So the name.replace operation isn't working for such cases.

In case the cmap_name is not an instance of PSLiteral but an instance of PDFStream, we would try to extract cmap_name, so if type(cmap_name) is PDFStream is helpful for this case.

With regards to PDFs mentioned in #210. Those PDF are being extracted fine with no errors.

For example, earlier 'DLIdentit-H' was mapped to 'Identity-H' which returned IdentityCMap(WMode=0) from CMapDB.get_cmap(). With this PR it returns CMap().

I have mapped DLIdentit-H intentionally with CMap() for the following reasons.

The PR @jtkese raised solved the issue by using .get method to extract cmap_name when the instance type is PDFStream.

I haven't found any conclusive evidence of considering same mapping for DLIdentit-H and Identity-H (documentations available online). Also I haven't seen DLIdentit-H as supported encoding.

If x is not an instance of PSLiteral it will return the repr(if repr exist). repr function actually doesn't return a type str.

That seems strange to me. __repr__ should always return a string. But I guess we can leave it as it is for now. Could you point out where this is happening? I cannot find any related use of repr() or __repr__() in the repo.

I haven't found any conclusive evidence of considering same mapping for DLIdentit-H and Identity-H

Good enough. Let's use the default of CMap() when we do not know what to do!

https://github.com/pdfminer/pdfminer.six/blob/master/pdfminer/pdftypes.py#L201

def __repr__(self): if self.data is None: assert self.rawdata is not None return '<PDFStream(%r): raw=%d, %r>' % (self.objid, len(self.rawdata), self.attrs) else: assert self.data is not None return '<PDFStream(%r): len=%d, %r>' % (self.objid, len(self.data), self.attrs)

That seems strange to me. repr should always return a string.

Yes, I have also assumed that the string operations are valid for __repr__ before coming through this bug. This being the very reason many developers use __str__ data model for denoting class object.

That sounds strange to me! I still do not understand how this __repr__() returns something else than a str (or an AssertionError). In which cases does this __repr__() returns something other than a string? If you could provide an example than we can open a new super specific issue.

In which cases does this __repr__() returns something other than a string? If you could provide an example than we can open a new super specific issue.

I feel it's depending on PDFs. Let me see some PDF that have been reported by users as incompatible.

pietermarsman · 2019-07-27T07:11:40Z

pdfminer/pdffont.py

+            if strict:
+                raise PDFFontError('Encoding is unspecified')
+            cmap_name = 'unknown'
+        if type(cmap_name) is PDFStream:


If x is not an instance of PSLiteral it will return the repr(if repr exist). repr function actually doesn't return a type str.

That seems strange to me. __repr__ should always return a string. But I guess we can leave it as it is for now. Could you point out where this is happening? I cannot find any related use of repr() or __repr__() in the repo.

I haven't found any conclusive evidence of considering same mapping for DLIdentit-H and Identity-H

Good enough. Let's use the default of CMap() when we do not know what to do!

tataganesh · 2019-07-31T16:52:20Z

The base branch should have been develop, not master. I merged without checking that. Any idea how I can revert this?

fakabbir · 2019-08-01T04:49:14Z

@ganeshtata For admins there should be a "revert" and "restore branch" button which appears in the PR thread. Just near the message. https://youtu.be/DgKNEH3capM

fakabbir · 2019-08-01T09:27:43Z

There is the Revert and View Details Button

john-redshelf and others added 3 commits February 25, 2019 11:42

Handle PDFStream as character map name in PDFCIDFont

8ab2e28

Encapsulates character map name

c022358

Corrects Indentation

8e4a82a

pietermarsman requested changes Jul 14, 2019

View reviewed changes

pietermarsman mentioned this pull request Jul 14, 2019

Handle PDFStream as character map name in PDFCIDFont #228

Closed

Removes @Property, Adds docstring

cc40af3

pietermarsman mentioned this pull request Jul 16, 2019

2 Pdf to text issues #222

Closed

fakabbir added 2 commits July 17, 2019 11:38

Adds Test, Removes Unnecessary Assumptions

fa40043

Removes Code Comments

b4c261b

pietermarsman requested changes Jul 22, 2019

View reviewed changes

Adds Test Cases, Neater Code For CMap Assignment

f1a4dce

pietermarsman approved these changes Jul 27, 2019

View reviewed changes

pietermarsman mentioned this pull request Jul 30, 2019

text extraction while font Encoding is a PDFStream object #279

Closed

tataganesh merged commit 48b2593 into pdfminer:master Jul 31, 2019

This was referenced Aug 10, 2019

Pdfstream as cmap #283

Merged

Extra Character in pdf2text.py output #284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pdfstream as cmap #264

Pdfstream as cmap #264

fakabbir commented Jul 12, 2019

pietermarsman left a comment

pietermarsman Jul 14, 2019

pietermarsman Jul 14, 2019

fakabbir Jul 15, 2019

pietermarsman Jul 14, 2019 •

edited

Loading

pietermarsman commented Jul 14, 2019

pombredanne commented Jul 15, 2019

pietermarsman commented Jul 15, 2019 •

edited

Loading

pietermarsman commented Jul 22, 2019

This comment was marked as outdated.

This comment was marked as outdated.

fakabbir Jul 24, 2019

pietermarsman Jul 27, 2019 •

edited

Loading

fakabbir Jul 27, 2019

pietermarsman Jul 27, 2019 •

edited

Loading

fakabbir Jul 29, 2019

pietermarsman Jul 27, 2019 •

edited

Loading

tataganesh commented Jul 31, 2019

fakabbir commented Aug 1, 2019

fakabbir commented Aug 1, 2019

	self.cmap = (spec, strict)
	self.cmap = self.get_cmap_from_spec(spec, strict)

Pdfstream as cmap #264

Pdfstream as cmap #264

Conversation

fakabbir commented Jul 12, 2019

pietermarsman left a comment

Choose a reason for hiding this comment

pietermarsman Jul 14, 2019

Choose a reason for hiding this comment

pietermarsman Jul 14, 2019

Choose a reason for hiding this comment

fakabbir Jul 15, 2019

Choose a reason for hiding this comment

pietermarsman Jul 14, 2019 • edited Loading

Choose a reason for hiding this comment

pietermarsman commented Jul 14, 2019

pombredanne commented Jul 15, 2019

pietermarsman commented Jul 15, 2019 • edited Loading

pietermarsman commented Jul 22, 2019

This comment was marked as outdated.

This comment was marked as outdated.

fakabbir Jul 24, 2019

Choose a reason for hiding this comment

pietermarsman Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

fakabbir Jul 27, 2019

Choose a reason for hiding this comment

pietermarsman Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

fakabbir Jul 29, 2019

Choose a reason for hiding this comment

pietermarsman Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

tataganesh commented Jul 31, 2019

fakabbir commented Aug 1, 2019

fakabbir commented Aug 1, 2019

pietermarsman Jul 14, 2019 •

edited

Loading

pietermarsman commented Jul 15, 2019 •

edited

Loading

pietermarsman Jul 27, 2019 •

edited

Loading

pietermarsman Jul 27, 2019 •

edited

Loading

pietermarsman Jul 27, 2019 •

edited

Loading