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

[Fabio D'Urso]

On Wednesday, April 03, 2013 04:52:35 PM Adam Reichold wrote:
> 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.

We're agreed then! :) So here's what I am going to do:

1) Edit rendering patches so that they only look at rotation A.
Unfortunatly I'll be preparing for an exam in the next two weeks. I'll try to 
post a preliminary version as soon as I can.

2) Add extra rotation and scale arguments (maybe in a container class, haven't 
decided yet) to all Annotation getter/setters.
They will default to 0°/100%, which will correspond to the same position you 
get in renderToImage.

If you want to start some work on the annotation rendition API in the 
meantime, I suggest you to have a look at 
http://lists.freedesktop.org/archives/poppler/2012-April/008956.html .
Basically, don't trust the annotations' bounding rectangle.
There's code in some AnnotXXX::draw methods (e.g. AnnotInk, AnnotLine) that 
expands the rectangle as needed just before drawing. That code will probably 
need to be decoupled from draw methods so we can know the exact rectangle size 
in advance.

Fabio

> 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