[poppler] How to read textbox positions?
Albert Astals Cid
aacid at kde.org
Mon Feb 26 23:47:16 UTC 2018
El dijous, 8 de febrer de 2018, a les 4:17:33 CET, suzuki toshiya va escriure:
> Dear Albert,
>
> > Can you please try adding some documentation in the cpp file like the
> > other
> > functions have?
>
> Oh, sorry for overlooking that task. Here it is...
Commited.
>
> Regards,
> mpsuzuki
>
> Albert Astals Cid wrote:
> > El diumenge, 28 de gener de 2018, a les 13:24:45 CET, suzuki toshiya va
> >
> > escriure:
> >> Dear Albert,
> >>
> >>> As far as i can see we don't use exceptions anywhere in the poppler
> >>> code,
> >>> so i would not introduce them "just for this".
> >>
> >> Oh, I'm sorry for my overlooking that. Here is updated patch which checks
> >> out-of-bound value to char_bbox() and return rect(0,0,0,0) if bad index
> >> is passed. Also I changed the type of the index from int to size_t.
> >
> > Can you please try adding some documentation in the cpp file like the
> > other
> > functions have?
> >
> > Cheers,
> >
> > Albert
> >>
> >> Regards,
> >> mpsuzuki
> >>
> >> Albert Astals Cid wrote:
> >>> El dissabte, 27 de gener de 2018, a les 17:17:37 CET, suzuki toshiya va
> >>>
> >>> escriure:
> >>>> Dear Albert,
> >>>>
> >>>> Oops, I apologize that I overlooked your response to
> >>>> improve the patch.
> >>>>
> >>>>> You have one text_box where you have 17 char bboxes but the
> >>>>> ustr.to_utf8().size() is 27, so if you iterate it's easy to get a
> >>>>> crash
> >>>>> because you get an out of bounds.
> >>>>
> >>>> Indeed, it might be common scenarios for the
> >>>> programmers assuming the input as ASCII data but
> >>>> something different is given.
> >>>>
> >>>>> So it would be nice to make
> >>>>> rectf char_bbox(int i) const;
> >>>>> not crash on an invalid i, i guess return a 0x0 rect
> >>>>> and then probably add some documentation about it?
> >>>>
> >>>> Checking the features of C++ std::vector, I think
> >>>> we have 2 options:
> >>>>
> >>>> a) use (as current patch) m_data->char_bbox[i] and
> >>>> sanitize the i-index before using it (as you proposed),
> >>>> and the bad values cause 0x0 rectf.
> >>>>
> >>>> b) use m_data->char_bbox.at(i) and let stdlib raise
> >>>> std::out_of_range error.
> >>>>
> >>>> which is better for C++ programmers?
> >>>
> >>> As far as i can see we don't use exceptions anywhere in the poppler
> >>> code,
> >>> so i would not introduce them "just for this".
> >>>
> >>> Cheers,
> >>>
> >>> Albert
> >>>
> >>>>> FWIW this was a real problem in Okular and i fixed it with
> >>>>> https://cgit.kde.org/okular.git/commit/generators/poppler/generator_pd
> >>>>> f.
> >>>>> cp
> >>>>> p?id=dcf0e78227959a52300d8f253c4b1058b3e81567
> >>>>>
> >>>>> Am i making any sense here?
> >>>>
> >>>> Thank you very much for the important suggestion and
> >>>> sorry for delayed reponse!
> >>>>
> >>>> Regards,
> >>>> mpsuzuki
> >>>>
> >>>> Albert Astals Cid wrote:
> >>>>> El dimecres, 3 de gener de 2018, a les 21:10:06 CET, Adam Reichold va
> >>>
> >>> escriure:
> >>>>>> Hello mpsuzuki,
> >>>>>>
> >>>>>> I do not think you need the two indirections: Since you want to keep
> >>>>>> that m_data indirection for ABI compatibility, just return
> >>>>>> std::vector<text_box> as text_box basically "is" a unique_ptr to its
> >>>>>> data with the additional getter methods. (You just need to make sure
> >>>>>> to
> >>>>>> default the move constructor and assignment operator since no copying
> >>>>>> implies no moving by default.)
> >>>>>>
> >>>>>> Attached is an update patch along those lines. (I also took the
> >>>>>> liberty
> >>>>>> to replace some bare deletes by use of unique_ptr in the
> >>>>>> implementation
> >>>>>> of page::text_list as well.)
> >>>>>
> >>>>> One thing that i wanted to improve in the Qt5 frontend but never go
> >>>>> to,
> >>>>> maybe we can start here is the fact that the number of boxes is "a bit
> >>>>> random", for example take https://bugs.kde.org/attachment.cgi?id=66189
> >>>>>
> >>>>> You have one text_box where you have 17 char bboxes but the
> >>>>> ustr.to_utf8().size() is 27, so if you iterate it's easy to get a
> >>>>> crash
> >>>>> because you get an out of bounds.
> >>>>>
> >>>>> So it would be nice to make
> >>>>> rectf char_bbox(int i) const;
> >>>>> not crash on an invalid i, i guess return a 0x0 rect
> >>>>> and then probably add some documentation about it?
> >>>>>
> >>>>> FWIW this was a real problem in Okular and i fixed it with
> >>>>> https://cgit.kde.org/okular.git/commit/generators/poppler/generator_pd
> >>>>> f.
> >>>>> cp
> >>>>> p?id=dcf0e78227959a52300d8f253c4b1058b3e81567
> >>>>>
> >>>>> Am i making any sense here?
> >>>>>
> >>>>> Cheers,
> >>>>>
> >>>>> Albert
> >>>>>>
> >>>>>> Best regards, Adam.
> >>>>>>
> >>>>>> Am 03.01.2018 um 07:38 schrieb suzuki toshiya:
> >>>>>>> Dear Adam,
> >>>>>>>
> >>>>>>> Thank you for the technical information on the difference of
> >>>>>>> std::shared_ptr and std::unique_ptr. Just I've tried to replace
> >>>>>>> the vector of the pointers to that of the unique_ptrs, and
> >>>>>>> remove the next_xxx methods.
> >>>>>>>
> >>>>>>> I attached the revised patch (gzipped) to the master, and
> >>>>>>> here I quote the revised part from my last revision, for
> >>>>>>> convenience to take a look on it.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> mpsuzuki
> >>>>>>>
> >>>>>>> diff --git a/cpp/poppler-private.h b/cpp/poppler-private.h
> >>>>>>> index 726068a..2cedd1e 100644
> >>>>>>> --- a/cpp/poppler-private.h
> >>>>>>> +++ b/cpp/poppler-private.h
> >>>>>>> @@ -72,12 +72,11 @@ class text_box_data
> >>>>>>>
> >>>>>>> {
> >>>>>>>
> >>>>>>> public:
> >>>>>>> text_box_data()
> >>>>>>>
> >>>>>>> - : next_text_box(0), has_space_after(false)
> >>>>>>> + : has_space_after(false)
> >>>>>>>
> >>>>>>> {
> >>>>>>> }
> >>>>>>> ustring text;
> >>>>>>> rectf bbox;
> >>>>>>>
> >>>>>>> - text_box *next_text_box;
> >>>>>>>
> >>>>>>> std::vector<rectf> char_bboxes;
> >>>>>>> bool has_space_after;
> >>>>>>>
> >>>>>>> };
> >>>>>>>
> >>>>>>> diff --git a/cpp/poppler-page.h b/cpp/poppler-page.h
> >>>>>>> index 18af213..3013997 100644
> >>>>>>> --- a/cpp/poppler-page.h
> >>>>>>> +++ b/cpp/poppler-page.h
> >>>>>>> @@ -22,6 +22,8 @@
> >>>>>>>
> >>>>>>> #include "poppler-global.h"
> >>>>>>> #include "poppler-rectangle.h"
> >>>>>>>
> >>>>>>> +#include <memory>
> >>>>>>> +
> >>>>>>>
> >>>>>>> namespace poppler
> >>>>>>> {
> >>>>>>>
> >>>>>>> @@ -33,12 +35,10 @@ class POPPLER_CPP_EXPORT text_box {
> >>>>>>>
> >>>>>>> ~text_box();
> >>>>>>> ustring text() const;
> >>>>>>> rectf bbox() const;
> >>>>>>>
> >>>>>>> - text_box *next_text_box() const;
> >>>>>>> - text_box *next_word() { return this->next_text_box(); };
> >>>>>>>
> >>>>>>> rectf char_bbox(int i) const;
> >>>>>>> bool has_space_after() const;
> >>>>>>>
> >>>>>>> private:
> >>>>>>> - text_box_data* m_data;
> >>>>>>> + std::unique_ptr<text_box_data> m_data;
> >>>>>>>
> >>>>>>> };
> >>>>>>>
> >>>>>>> class document;
> >>>>>>>
> >>>>>>> @@ -79,7 +79,7 @@ public:
> >>>>>>> ustring text(const rectf &rect = rectf()) const;
> >>>>>>> ustring text(const rectf &rect, text_layout_enum layout_mode)
> >>>>>>> const;
> >>>>>>>
> >>>>>>> - std::vector<text_box*> text_list(rotation_enum rotation) const;
> >>>>>>> + std::vector<std::unique_ptr<text_box>> text_list(rotation_enum
> >>>>>>> rotation) const;>
> >>>>>>>
> >>>>>>> private:
> >>>>>>> page(document_private *doc, int index);
> >>>>>>>
> >>>>>>> diff --git a/cpp/poppler-page.cpp b/cpp/poppler-page.cpp
> >>>>>>> index 9461ab9..5da2e1d 100644
> >>>>>>> --- a/cpp/poppler-page.cpp
> >>>>>>> +++ b/cpp/poppler-page.cpp
> >>>>>>> @@ -291,14 +291,13 @@ ustring page::text(const rectf &r,
> >>>>>>> text_layout_enum
> >>>>>>> layout_mode) const
> >>>>>>>
> >>>>>>> */
> >>>>>>>
> >>>>>>> text_box::text_box(const ustring& text, const rectf &bbox)
> >>>>>>> {
> >>>>>>>
> >>>>>>> - m_data = new text_box_data();
> >>>>>>> + m_data = std::unique_ptr<text_box_data>(new text_box_data());
> >>>>>>>
> >>>>>>> m_data->text = text;
> >>>>>>> m_data->bbox = bbox;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> text_box::~text_box()
> >>>>>>> {
> >>>>>>>
> >>>>>>> - delete m_data;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> ustring text_box::text() const
> >>>>>>>
> >>>>>>> @@ -311,11 +310,6 @@ rectf text_box::bbox() const
> >>>>>>>
> >>>>>>> return m_data->bbox;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> -text_box* text_box::next_text_box() const
> >>>>>>> -{
> >>>>>>> - return m_data->next_text_box;
> >>>>>>> -}
> >>>>>>> -
> >>>>>>>
> >>>>>>> rectf text_box::char_bbox(int i) const
> >>>>>>> {
> >>>>>>>
> >>>>>>> return m_data->char_bboxes[i];
> >>>>>>>
> >>>>>>> @@ -326,10 +320,10 @@ bool text_box::has_space_after() const
> >>>>>>>
> >>>>>>> return m_data->has_space_after;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> -std::vector<text_box*> page::text_list(rotation_enum rotate) const
> >>>>>>> +std::vector<std::unique_ptr<text_box>>
> >>>>>>> page::text_list(rotation_enum
> >>>>>>> rotate) const>
> >>>>>>>
> >>>>>>> {
> >>>>>>>
> >>>>>>> TextOutputDev *output_dev;
> >>>>>>>
> >>>>>>> - std::vector<text_box*> output_list;
> >>>>>>> + std::vector<std::unique_ptr<text_box>> output_list;
> >>>>>>>
> >>>>>>> const int rotation_value = (int)rotate * 90;
> >>>>>>>
> >>>>>>> /* config values are same with Qt5 Page::TextList() */
> >>>>>>>
> >>>>>>> @@ -366,7 +360,7 @@ std::vector<text_box*>
> >>>>>>> page::text_list(rotation_enum
> >>>>>>> rotate) const
> >>>>>>>
> >>>>>>> double xMin, yMin, xMax, yMax;
> >>>>>>> word->getBBox(&xMin, &yMin, &xMax, &yMax);
> >>>>>>>
> >>>>>>> - text_box* tb = new text_box(ustr, rectf(xMin, yMin, xMax-xMin,
> >>>>>>> yMax-yMin)); + std::unique_ptr<text_box> tb =
> >>>>>>> std::unique_ptr<text_box>(new text_box(ustr, rectf(xMin, yMin,
> >>>>>>> xMax-xMin,
> >>>>>>> yMax-yMin)));
> >>>>>>>
> >>>>>>> tb->m_data->has_space_after = (word->hasSpaceAfter() == gTrue);
> >>>>>>>
> >>>>>>> tb->m_data->char_bboxes.reserve(word->getLength());
> >>>>>>>
> >>>>>>> @@ -375,10 +369,7 @@ std::vector<text_box*>
> >>>>>>> page::text_list(rotation_enum
> >>>>>>> rotate) const
> >>>>>>>
> >>>>>>> tb->m_data->char_bboxes.push_back(rectf(xMin, yMin, xMax-
> >
> > xMin,
> >
> >>>>>>> yMax-yMin));
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> - if (output_list.size() > 0)
> >>>>>>> - output_list.back()->m_data->next_text_box = tb;
> >>>>>>> -
> >>>>>>> - output_list.push_back(tb);
> >>>>>>> + output_list.push_back(std::move(tb));
> >>>>>>>
> >>>>>>> }
> >>>>>>> delete word_list;
> >>>>>>> delete output_dev;
> >>>>>>>
> >>>>>>> diff --git a/cpp/tests/poppler-dump.cpp b/cpp/tests/poppler-dump.cpp
> >>>>>>> index 09b180d..d26a0ef 100644
> >>>>>>> --- a/cpp/tests/poppler-dump.cpp
> >>>>>>> +++ b/cpp/tests/poppler-dump.cpp
> >>>>>>> @@ -333,7 +333,7 @@ static void print_page_text_list(poppler::page
> >>>>>>> *p)
> >>>>>>>
> >>>>>>> std::cout << std::endl;
> >>>>>>> return;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> - std::vector<poppler::text_box*> text_list =
> >>>>>>> p->text_list(poppler::rotate_0); +
> >>>>>>> std::vector<std::unique_ptr<poppler::text_box>> text_list =
> >>>>>>> p->text_list(poppler::rotate_0);
> >>>>>>>
> >>>>>>> std::cout << "---" << std::endl;
> >>>>>>> for (size_t i = 0; i < text_list.size(); i ++) {
> >>>>>>>
> >>>>>>> @@ -343,7 +343,6 @@ static void print_page_text_list(poppler::page
> >>>>>>> *p)
> >>>>>>>
> >>>>>>> std::cout << "( x=" << bbox.x() << " y=" << bbox.y() << "
> >>>>>>> w="
> >>>>>>> <<
> >>>>>>>
> >>>>>>> bbox.width() << " h=" << bbox.height() << " )";
> >>>>>>>
> >>>>>>> std::cout << std::endl;
> >>>>>>>
> >>>>>>> - delete text_list[i];
> >>>>>>>
> >>>>>>> }
> >>>>>>> std::cout << "---" << std::endl;
> >>>>>>>
> >>>>>>> }
> >>>>>>>
> >>>>>>> Adam Reichold wrote:
> >>>>>>>> Hello mpsuzuki,
> >>>>>>>>
> >>>>>>>> std::shared_ptr was introduced in C++11 and Poppler does use C++11
> >>>>>>>> only
> >>>>>>>> since a relatively short time, so I guess there just was no time to
> >>>>>>>> use
> >>>>>>>> it anywhere.
> >>>>>>>>
> >>>>>>>> In any case it is a pretty heavy-handed solution due to the atomic
> >>>>>>>> memory accesses on the reference counter which will slow down even
> >>>>>>>> single-threaded programs which do not need it. Hence I would
> >>>>>>>> suggest
> >>>>>>>> starting with std::unique_ptr to get a move-only type.
> >>>>>>>>
> >>>>>>>> Best regards, Adam.
> >>>>>>>>
> >>>>>>>> Am 02.01.2018 um 08:19 schrieb suzuki toshiya:
> >>>>>>>>> Dear Albert, Adam,
> >>>>>>>>>
> >>>>>>>>> Thank you for the helpful review. I should have more
> >>>>>>>>> training on C++... Please let me spend to improve the
> >>>>>>>>> patch.
> >>>>>>>>>
> >>>>>>>>>> (Another easy option albeit with some
> >>>>>>>>>> overhead to get automatic destruction would be to wrap the actual
> >>>>>>>>>> data
> >>>>>>>>>> inside a std::shared_ptr and hence use reference counting.)
> >>>>>>>>>
> >>>>>>>>> Yeah, from some posts in stackoverflow, I found about
> >>>>>>>>> shared_ptr... But it seems that current poppler does
> >>>>>>>>> not use shared_ptr at all. Is this a coding policy?
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> mpsuzuki
> >>>>>>>>>
> >>>>>>>>> Adam Reichold wrote:
> >>>>>>>>>> Hello
> >>>>>>>>>>
> >>>>>>>>>> Am 02.01.2018 um 01:06 schrieb Albert Astals Cid:
> >>>>>>>>>>> El diumenge, 31 de desembre de 2017, a les 0:45:16 CET, suzuki
> >>>>>>>>>>> toshiya
> >>>>>>>>>>> va
> >>>>>>>>>>>
> >>>>>>>>>>> escriure:
> >>>>>>>>>>>> Hi,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I've tried to implement the suggestion, I attached my current
> >>>>>>>>>>>> patch.
> >>>>>>>>>>>>
> >>>>>>>>>>>> As suggested, the most part is just copied from Qt frontend and
> >>>>>>>>>>>> renamed,
> >>>>>>>>>>>> except of one point: TextBox.nextWord() looks slightly
> >>>>>>>>>>>> confusing,
> >>>>>>>>>>>> because the returned object is a pointer to TextBox. I wrote
> >>>>>>>>>>>> text_box.next_text_box() and a macro text_box.next_word() which
> >>>>>>>>>>>> calls next_text_box() internally.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Another point I want to discuss is the design of the list give
> >>>>>>>>>>>> by
> >>>>>>>>>>>> poppler::page::text_list(). In Qt frontend, Page::textList()
> >>>>>>>>>>>> returns
> >>>>>>>>>>>> QList<TextBox*>. For similarity, current patch returns
> >>>>>>>>>>>> std::vector<text_box*> for similarity to Qt frontend.
> >>>>>>>>>>>>
> >>>>>>>>>>>> But, if we return the vector of pointers, the client should
> >>>>>>>>>>>> destruct
> >>>>>>>>>>>> the objects pointed by the vector, before destructing vector
> >>>>>>>>>>>> itself.
> >>>>>>>>>>>> Using a vector of text_box (not the pointer but the object
> >>>>>>>>>>>> itself),
> >>>>>>>>>>>> like std::vector<text_box>, could be better, because the
> >>>>>>>>>>>> destructor
> >>>>>>>>>>>> of the vector would internally call the destructor for text_box
> >>>>>>>>>>>> object.
> >>>>>>>>>>>> (Qt has qDeleteAll(), but I think std::vector does not have
> >>>>>>>>>>>> such).
> >>>>>>>>>>>> If I'm misunderstanding about C++, please correct.
> >>>>>>>>>>>
> >>>>>>>>>>> First you need to fix the text_box class, you either need to
> >>>>>>>>>>> make
> >>>>>>>>>>> it
> >>>>>>>>>>> uncopiable (and thus making your suggestion not possible) or you
> >>>>>>>>>>> need
> >>>>>>>>>>> to make the copy and assignment operators not break your class
> >>>>>>>>>>> since
> >>>>>>>>>>> at the moment the default operators will just copy the pointer
> >>>>>>>>>>> and
> >>>>>>>>>>> everything will break because you will end up with a dangling
> >>>>>>>>>>> pointer.
> >>>>>>>>>>>
> >>>>>>>>>>> If you go for the second option you'll want the move constructor
> >>>>>>>>>>> and
> >>>>>>>>>>> move
> >>>>>>>>>>> operator too.
> >>>>>>>>>>
> >>>>>>>>>> Should the first option also be viable if text_box becomes a
> >>>>>>>>>> move-only
> >>>>>>>>>> type, i.e. no copying but move construction and assignment? I
> >>>>>>>>>> think
> >>>>>>>>>> std::vector can handle it. (Another easy option albeit with some
> >>>>>>>>>> overhead to get automatic destruction would be to wrap the actual
> >>>>>>>>>> data
> >>>>>>>>>> inside a std::shared_ptr and hence use reference counting.)
> >>>>>>>>>>
> >>>>>>>>>> Best regards, Adam.
> >>>>>>>>>>
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>>
> >>>>>>>>>>> Albert
> >>>>>>>>>>>>
> >>>>>>>>>>>> Regards,
> >>>>>>>>>>>> mpsuzuki
> >>>>>>>>>>>>
> >>>>>>>>>>>> Albert Astals Cid wrote:
> >>>>>>>>>>>>> El dimecres, 27 de desembre de 2017, a les 12:26:25 CET,
> >>>>>>>>>>>>> Jeroen
> >>>>>>>>>>>>> Ooms
> >>>>>>>>>>>>> va
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> escriure:
> >>>>>>>>>>>>>> Is there a method in poppler-cpp to extract text from a pdf
> >>>>>>>>>>>>>> document,
> >>>>>>>>>>>>>> including the position of each text box? Currently we use
> >>>>>>>>>>>>>> page->text()
> >>>>>>>>>>>>>> with page::physical_layout which gives all text per page, but
> >>>>>>>>>>>>>> I
> >>>>>>>>>>>>>> need
> >>>>>>>>>>>>>> more detailed information about each text box per page.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> You want to code the variant of qt5 frontend
> >>>>>>>>>>>>> Poppler::Page::textList() for
> >>>>>>>>>>>>> cpp frontend, it shouldn't be that hard getting inspiration
> >>>>>>>>>>>>> (i.e.
> >>>>>>>>>>>>> almost-copying) the code, do you have time for it?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Albert
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>>>> poppler mailing list
> >>>>>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>>> poppler mailing list
> >>>>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>>>>>>>>
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> poppler mailing list
> >>>>>>>>>>> poppler at lists.freedesktop.org
> >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >
> > _______________________________________________
> > poppler mailing list
> > poppler at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/poppler
More information about the poppler
mailing list