[poppler] [PATCHES] Support for the NoRotate annotation flag

Wed Apr 3 07:52:35 PDT 2013

Hello Fabio,

Am 02.04.2013 22:42, schrieb Fabio D'Urso:
> On Tuesday, April 02, 2013 10:05:20 PM Adam Reichold wrote:
>> Hello again,
>>
>> Am 02.04.2013 21:44, schrieb Fabio D'Urso:
>>> On Tuesday, April 02, 2013 08:55:01 PM Adam Reichold wrote:
>>>> Hello Fabio,
>>>>
>>>> Am 02.04.2013 20:28, schrieb Fabio D'Urso:
>>>>> On Tuesday, April 02, 2013 07:59:57 PM Adam Reichold wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Am 02.04.2013 19:38, schrieb Albert Astals Cid:
>>>>>>> El Dimarts, 2 d'abril de 2013, a les 18:36:48, Adam Reichold va
>>>>>>>
>>>>>>> escriure:
>>>>>>>> Hello,
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>> When implementing this in qpdfview, I spent most of the time
>>>>>>>> figuring out how to correctly transform the annotation boundaries
>>>>>>>> given to me by the qt4 frontend. This is of course not problem in
>>>>>>>> itself, but it aligns with this observation as the applications
>>>>>>>> is bound to use the internal coordinate transformations and needs
>>>>>>>> to correctly recreate their results.
>>>>>>>>
>>>>>>>> Maybe it would be better to let applications that want to
>>>>>>>> implement this draw those annotations by themselves, hiding
>>>>>>>> Poppler's rendition?
>>>>>>>
>>>>>>> What's the benefit of everyone doing the same? Wasn't the
>>>>>>> repetition of work why libraries where created?
>>>>>>
>>>>>> There is some duplication the current setting as well as the
>>>>>> application has to recreate the internal transformations, but of
>>>>>> course this is much less than actually drawing the annotations. I am
>>>>>> just not sure how well the coupling of these interactive elements with
>>>>>> application behavior through an interface designed to render paginated
>>>>>> content will work. (And how prone it will be to breakage by internal
>>>>>> changes.)
>>>>>
>>>>> I think I see your point, my patches only tell where the annotation is
>>>>> if
>>>>> you render the page with the default rotation.
>>>>> If you ask renderToImage to render the page with a different rotation
>>>>> the
>>>>> position changes, and you want to know this new position, correct?
>>>>
>>>> Yes, that seems to be one way to ensure that the application uses the
>>>> right coordinates no matter how those flags are implemented.
>>>>
>>>>> I thought about this when I wrote the patches and came to the conclusion
>>>>> that "by default" we should return coordinates referring to the default
>>>>> rotation, so that we don't break existing applications, at least the
>>>>> "simple" ones (e.g. okular always requests 0-rotated rendering and
>>>>> applies user-requested rotations on his own, and I guess most
>>>>> applications keeping internal caches do the same, but of course I might
>>>>> be wrong :)).
>>>>
>>>> Actually, I invalidate the cache on rotation change. I was thinking of
>>>> speed and quality improvements if the rotation is done by Splash on the
>>>> vector data instead of using Qt to rotate a QImage. Is doing it like
>>>> this pointless?
>>>
>>> I have no idea if it makes any quality difference. I guess that as long as
>>> pixels are "squares" (i.e. horizontal and vertical DPI are equal) it's the
>>> same. But I'm just guessing, can't tell for certain.
>>>
>>> >From a speed point of view, it really depends on the document. A simple
>>>> page>>
>>> is obviously faster rotated directly in Splash. A page that takes longer
>>> to
>>> render (e.g. a lot of primitives, or images to be upscaled) is better
>>> rendered once and rotated afterwards each time the user changes rotation.
>>>
>>> Still, if I remember correctly, okular too invalidated the cache last time
>>> I checked. It goes through a full SplashRendering+PostRotation every time
>>> the user changes rotation. I guess it's like this because not all
>>> file-format backends can handle rotation internally.
>>>
>>>>> For those who need coordinates at different rotations too we have two
>>>
>>> options:
>>>>>  - Let applications do the math themselves. That is, what you have been
>>>>>  doing>
>>>>>  
>>>>>    for qpdfview. I know it's a PITA, I too did that for okular in my
>>>>>    first
>>>>>    design.
>>>>
>>>> I think it is less about doing the math, but we would also enter a
>>>> rather restrictive contract on how not to change the implementation of
>>>> the NoRotate flag. (Is the transformation applied part of the PDF spec?
>>>> Maybe this point is invalid then.)
>>>
>>> Behavior for the NoRotate flag is described immediatly after the
>>> annotation
>>> flag list in PDF specs.
>>>
>>> By the way, in the "let applications do the math themselves" approach
>>> applications have to transform not only the Annotation's boundary
>>> rectangle, but subclass-specific data too (e.g. LineAnnotation's
>>> vertices,
>>> InkAnnotation's paths...)
>>>
>>>>>  - Add some hook in poppler qt4 to request coordinates at different
>>>>>  rotations,>
>>>>>  
>>>>>    I can think of two ways to do this:
>>>>> 	a. Add an extra rotation argument to all annotation getters and 
> setters
>>>>> 	b. Add a per-Annotation setting to set the desired coordinate 
> rotation
>>>>>
>>>>> Both of them would break the ABI (well, #1 can also be implemented by
>>>>> adding a new set of overloaded getters and setters [*]).
>>>>>
>>>>> #2 would break the API too because this variable would have to be put in
>>>>> the Annotation class, not AnnotationPrivate (AnnotationPrivate is
>>>>> transparently shared across several instances of the same annotation)
>>>>>
>>>>> I like #1 much more, especially in the light of the thread-safety
>>>>> progresses that have been made, but if we can't break the ABI it adds a
>>>>> lot duplication.
>>>>>
>>>>> * = still, this breaks souce compatibility if applications use pointers
>>>>> to
>>>>> annotation methods, but I guess this is very unlikely)
>>>>
>>>> I agree, version a/#1 sounds much nicer.
>>>
>>> In the meantime we can also add the scale factor in preparation for the
>>> NoScale flag, so we don't break the ABI again in the future.
>>
>> I am not sure this would be possible as if you want to implement NoScale
>> for high resolution media, you would have to uncouple resolution and
>> scale factor for rendering as well, e.g. the resolution parameters of
>> 'Poppler::Page::renderToImage'.
>>
>>>>>>>> (This could also solve the problem of the annotations being
>>>>>>>> rotated out of the page bounding box.)
>>>>>>>
>>>>>>> I'm sure we can find some other solution for that.
>>>>>>
>>>>>> Of course there are other solutions, for example adding an annotation
>>>>>> rendering interface to the frontends the results of which the
>>>>>> application can then compose by itself. My point is basically that if
>>>>>> the interactive elements begin to be more than just
>>>>>> some-more-stuff-on-the-page, the API should probably reflect that.
>>>>>
>>>>> That'd be great! It would also solve the issue of having to redraw the
>>>>> whole page on each single annotation change. I like this idea very very
>>>>> much! But I'm too busy to work on it atm :(
>>>>
>>>> I would like to try to work something out, but waiting for this would
>>>> probably keep the Okular bug open for rather long time.
>>>
>>> Why, can't it be done on top of these patches? Or even current git master,
>>> I don't think that there are going to be a lot of conflicts.
>>
>> I agree, it would be possible to do both, but maybe not very desirable:
>> We would still break applications not expecting the changed boundary of
>> NoRotate annotations. And IMHO, the main benefit of a separate
>> presentation API would be to keep these complications out of the
>> annotation data model.
> 
> Note that the NoRotate flag "unrotates" *two* rotations:
>  A. The one specified by the /Rotate page attribute (i.e. what the pdf tells)
>  B. The extra one added by the user
> 
> Speaking about the data model (i.e. the qt4 side of the patches), I'm OK with 
> ignoring rotation B, but I think that rotation A, which is fixed and only 
> depends on the page, should still be applied. The qt4 side of my patches does 
> exactly this.
> 
> On the rendering side (i.e. patch 0001), my code will probably need to be 
> changed to only care about rotation A instead of the sum like it currently 
> does.
> This would also "solve" the cairo issue by avoiding rotations that depend on 
> cairo's context.
> 
> Please tell me if I haven't been clear enough as the text that I've written 
> seems a bit convoluted :/

So I interpret it like this: Since the interaction of NoRotate and
rotation A is independent of the 'rotate' parameter of 'renderToImage',
the frontend behaves exactly the same (and user applications actually
need no change at all since) but if I open the same file with and
without a patched Poppler, some annotations might have a different
('unrotated') boundary.

If that is correct, I completely agree that it makes sense to implement
it like that.

Best regards, Adam.

>>> I would also leave an option to render annotations along with the page
>>> like we do now (enabled by default), mainly for backward compatibility
>>> reasons, but also for simpler use cases (e.g. file managers'
>>> thumbnailers)
>>
>> My idea would be to leave the behavior of the current rendering as it
>> is, ignoring NoScale and NoRotate, and add an annotation rendition API
>> for applications that want to use these flags but must be able to do the
>> composition themselves.
> 
> With the idea above, NoRotate would only honor rotation A (which is fixed, and 
> transparently hidden by the qt4's data model).
> I agree on ignoring NoScale.
> 
> In other words, the "compatibility mode" would behave exactly as if you put 
> acroread on 100% zoom and original orientation, and literally rotated the 
> monitor ;)
> 
> Fabio
> 
>> Best regards, Adam.
>>
>>>> Best regards, Adam.
>>>>
>>>>> Fabio
>>>>>
>>>>>> Best regard, Adam.
>>>>>>
>>>>>>> Cheers, Albert _______________________________________________
>>>>>>> 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
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>
>> _______________________________________________
>> 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
> 