[poppler] Annot borders

Thu Nov 28 05:09:55 PST 2013

Albert Astals Cid <aacid at kde.org> writes:

> El Dimecres, 27 de novembre de 2013, a les 08:51:42, Carlos Garcia Campos va 
> escriure:
>> Albert Astals Cid <aacid at kde.org> writes:
>> > El Dimarts, 26 de novembre de 2013, a les 21:11:11, Carlos Garcia Campos
>> > va
>> > 
>> > escriure:
>> >> I've noticed some small issues in the way we handle the border of
>> >> annots compared to what the spec says and what acroread does. Attached
>> >> are 4 patches:
>> >> 
>> >> [PATCH 1/4] annots: Remove unused AnnotBorderType from AnnotBorder
>> >> 
>> >> This is simply a cleanup, we have a base class AnnotBorder that allows
>> >> to create instances of it with the unknown type. It doesn't make much
>> >> sense and we are not using it at all, so I've removed the
>> >> AnnotBorderType, and made the constructor protected.
>> >> 
>> >> [PATCH 2/4] annots: Add helper function Annot::setLineStyleForBorder
>> >> 
>> >> The code to set the line width and dash was duplicated in several draw()
>> >> methods, so I've moved it to a helper function. This patch also changes
>> >> the way we are using it. We were always ignoring the border when the
>> >> border with was 0, but it doesn't seem to be always correct. The PDF
>> >> spec says that in both Border and BS entries when the width is 0, no
>> >> border should be drawn at all. But some annotations like lines,
>> >> geometry, polygons, etc. don't use the border entry to actually draw a
>> >> border, but to set the line with and dash for the stroke operations. For
>> >> example, we were not drawing lines for annots with no border or with a
>> >> border width = 0, but acroread does, because in this case the border
>> >> entry is used to set the line with and dash pattern, and the PDF spec
>> >> also says that a line width of 0 should be drawn as the thinnest line
>> >> that
>> >> can be rendered at device resolution: 1 device pixel wide.
>> >> So, for FreeText annotations where we actually draw a border, we only
>> >> call Annot::setLineStyleForBorder if we have a border and the width is
>> >> greater than 0, and in all other cases we always call it when we have a
>> >> border. Of course, this only affects annotations not having an AP entry.
>> >> 
>> >> [PATCH 3/4] annots: Use a default border for annots that can have a BS
>> >> entry
>> >> 
>> >> According to the PDF spec if neither the Border nor the BS entry is
>> >> present, the border shall be drawn as a solid line with a width of 1
>> >> point. But again, acroread seems to only apply this rule for annotations
>> >> that can have a BS entry. This patch moves the parsing of the BS entry
>> >> From the base Annot class to the specific annot classes that can have a
>> >> BS entry, and it creates a default border object when neither Border nor
>> >> Bs is present. It also removes the border passed to Gfx::drawAnnot() in
>> >> AnnotFileAttachment::draw and AnnotSound::draw because acroread ignores
>> >> the Border entry also for annots that can't have a BS entry.
>> >> This ensures that we always draw line, geometry, polygon, etc, even if
>> >> there's no border specified.
>> >> 
>> >> [PATCH 4/4] annots: Make Annot::setBorder receive an AnnotBorder object
>> >> 
>> >> Currently we can only set AnnotBorderArray objects to annots. This might
>> >> have no effect if the annots already has a BS entry, for example,
>> >> because the BS takes precedence over Border. This patches changes
>> >> Annot::setBorder to receive an AnnotBorder object, so that you can
>> >> either pass an AnnotBorderArray or an  AnnotBorderBS. Frontends should
>> >> always use BS when updating an annotation that can have BS entries. The
>> >> patch also completes the implementation of writeToObject method for
>> >> array borders and adds an impementation for BS borders too.
>> >> 
>> >> I've passed my tests with no regressions,
>> > 
>> > Are there improvements in rendering of some files?
>> 
>> Nothing changed in my tests, because most of the PDF documents either
>> use explicit borders or don't use 0 border widths. I noticed this
>> problem when reviewing the patches to add support for adding square and
>> circle annotations in the glib frontend. Germán told me he needed to add
>> a border or nothing was drawn, and that sounded wrong to me. So, reading
>> the spec, our code and playing with acroread I ended up writing these
>> patches. See bug https://bugs.freedesktop.org/show_bug.cgi?id=70983.
>> 
>> There might be an improvement if there were any file with a /Border in a
>> FileAttachment annotation, for example, since we were drawing a border
>> in such case, while these patches don't.
>> 
>> >> but I can't test the qt
>> >> frontend (that uses setBorder(), for example), so it would be great if
>> >> someone could test these patches to make sure they don't introduce any
>> >> regression in qt (it should build in any case).
>> > 
>> > Why would it introduce any regression?
>> 
>> Because qt is the only frontend using Annot::setBorder() and I've
>> changed that. I don't mean regressions of rendering existing files, but
>> annotating files. I don't even know how Annot::setBorder() is used in
>> qt, I just grepped and saw it was used.
>
> You could have looked, it's like 4 lines. I'm not a huge specialist of the 
> annot code, but looks like it should still work.

Yes, I'm sorry, I wrote these patches quickly because I didn't have much time.

> Anyway I did run Okular against a patched poppler and a unpatched one and 
> annotations that hit that path render the same to my naked eyes.

Great, thanks for testing!

> BTW you can't commit the first patch since the qt frontends use that getType 
> function you removed because noone was using.

Oops, I'll fix it then, this time I'll have to actually look at the
code :-P

> Next time we meet i'll give you a memory stick so you can backup some of your 
> stuff there and you can install Qt in the freed space.

:-D

> Cheers,
>   Albert
>
>> 
>> > Cheers,
>> > 
>> >   Albert
>> >> 
>> >> Leonard, please, feel free to correct me if I'm wrong in any of my
>> >> interpretations of the PDF spec.
>> >> 
>> >> Regards,
>> > 
>> > _______________________________________________
>> > poppler mailing list
>> > poppler at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/poppler
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler

-- 
Carlos Garcia Campos
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20131128/1f905a0e/attachment.pgp>