Bug 83642

Interesting in the glib frontend you still have the outputdevice around. I've been thinking of something like this for the qt frontend but we delete the output after rendering since we do/did not have any reason to keep it around "wasting" memory.

I was wondering how hard for you guys in the glib side would be instead of having this, have something that just renders the annotation in a separate image buffer and then do the composition at a higher level (i.e. maybe even inside evince/okular).

Do you think that's doable?

Albert, I didnt understand the keeping the outputdevice around... Usually in cairo we have the cairo context of a surface to be drawn in screen. In other part, in evince, we have a image cache of the page already rendered, so what we 'll do is to blit this image surface into the screen, and then to use renderannot over the screen surface. THis saves us a new blit... but also I didn't figure out a way of having two images (one with the page cache and one with the annotation drawing on top) and the blit them in order. My problem is that annotations are drawn with tdifferent blenModes. which sets a different cairo operator, so in theory, we could keep this operator, change poppler code to render al of this wihtout setting the operator to the surface. and then set the cairo operator only when drawing the image with the annotation over the image with the page... but that seemed too omplicated to me and needed to change how poppler is drawing. drawing directly over the screen surface allows me to first draw the page on the screen, and then use the same codepath of popppler to render the annotaitons directly over the screen using the proper cairo operators wihtout changing how poppler works... hope my answer makes sense.

Ok, ignore my comment, what i wanted to do is actually impossible since there are transparent annotations so you can't render the annotation without taking the  page contents into account.

Created attachment 137545 [details]
QImage rendered with similar QT5 patch

Hi guys,

is this issue still worth discussing or is it already doomed as impossible / too complicated / too little benefit?

I'm currently doing a patch with similar goals for the QT5 frontend (just for fun and learning). Patrick Golden pointed me to here - thanks Patrick! If you like, we could join efforts.

For now you can watch patch progress here [0]. The attached PNG demonstrates the current state: An opaque free text annotation was rendered with the patch to an QImage, the QImage was saved to file and then opened in GIMP.

My patch differs a bit from yours:
-QT5 frontend
-render API method is added to Annotation instead of Page class
-I refactored Annot::draw with template design pattern in mind, to be able to hook in between the newly established makeAnnotation and drawAnnotaiton steps. This way I'm able to respect the size of the dynamically created appearance object (may be != Annot::rect).

Imho valid use cases do still exist:
UC1 Extract isolated annotations from PDF in non-pdf-reader environments, maybe some command line tool.
UC2 When new annotation shall be added in a reader (e.g. popup note), show a realistic preview of the icon, instead of some inaccurate dummy icon. [1]
UC3 Paint annotations while dragging them around (rendering page wise is too expensive). [2]
UC4 Enable composition of annotations in readers.
UC5 WYSIWYG editing inline notes in readers. [3]

For UC1..2, one probably wants to get just an isolated annotation drawn on transparent background, without other objects mixed in. Seems not too difficult.
For UC3..5, blending/compositing and probably z-order comes into play, which is somewhere between a bit more tricky and impossible, I don't know :)

Would you agree with those UCs?

Cheers
Tobias

[0] https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a
[1] https://bugs.kde.org/show_bug.cgi?id=358065
[2] https://bugs.kde.org/show_bug.cgi?id=333981
[3] https://bugs.kde.org/show_bug.cgi?id=358061

you have waaaaaaaaaaaaaaaay to many unrelated whitespace changes in that patch, please remove them, i don't want to spend my time checking if something has changed in mass whitespace changes like https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a#file-poppler-render_individual_annots-patch-L804

Whitespace changes are removed now. See rev. 2 of https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a. Sorry, wanted to cleanup interleaved tabs and thought one could ignore changes during review. But obviously the "?w=1" trick doesn't work for gists. Btw., if you prefer we can shift the patch over to bugs.freedesktop.org anytime.

You'll probably find more to dislike. I do, just had no better ideas yet. Some reasoning in advance:
The changes around Qt5SplashOutputDev/ArthurOutputDev [0] are a bit heavy and unfinished (dislike!). But I wanted to share as much "factory code" as possible between Poppler::Page and Poppler::Annotation, thus factored out common stuff.
The changes regarding Annot::draw [1] are a bit heavy and unfinished (again dislike). But I had to find a common place amongst all annotation types where one can hook in to get the real size of the dynamically generated appearance *1). Plus the template pattern approach hopefully helps poppler newbies like me to understand things better, as it makes common steps in Annot::draw schematic and explicit.

Regarding API... QImage Poppler::Annotation::renderToImage(double hDPI, double vDPI) was the simplest API I could imagine. Functionality explicitly cares about one single annotation and no other objects, thus the method is bound to Annotation, instead of Page. We don't need to bother clients with position/size arguments, poppler knows it all, and better. We probably don't need callbacks. Maybe we want to allow fine tuning some rendering settings? E.g. an enableTransparentBackground option could make sense to chose either page color or transparency as background. Also a further method Poppler::Annotation::renderToArthur(...) could be useful and would be consistent to Poppler::Page::renderToArthur.

*1) Annot::rect and actual drawn size may differ. E.g. icons are forced to 24pt regardless of what's in Annot::rect. A bug on it's own imho [2], but that's the way it is for now.

[0] https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a#file-poppler-render_individual_annots-patch-L2710
[1] https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a#file-poppler-render_individual_annots-patch-L91
[2] https://bugs.kde.org/show_bug.cgi?id=388458

> --- Comment #6 from Tobias Deiminger <haxtibal@posteo.de> ---
> Whitespace changes are removed now. See rev. 2 of
> https://gist.github.com/haxtibal/eab9320d43e47dfefe47249ee1e6d01a. Sorry,
> wanted to cleanup interleaved tabs and thought one could ignore changes
> during review. But obviously the "?w=1" trick doesn't work for gists. Btw.,
> if you prefer we can shift the patch over to bugs.freedesktop.org anytime.

Could you try to split this into a series of patches so they are smaller and easier to review since each patch has a clear focus/idea behind it? Seems like there's various stuff going on there (specially all the qt5 code moving seems like could be easily split).

> You'll probably find more to dislike. I do, just had no better ideas yet.
> Some reasoning in advance:
> The changes around Qt5SplashOutputDev/ArthurOutputDev [0] are a bit heavy
> and unfinished (dislike!). But I wanted to share as much "factory code" as
> possible between Poppler::Page and Poppler::Annotation, thus factored out
> common stuff. The changes regarding Annot::draw [1] are a bit heavy and
> unfinished (again dislike). But I had to find a common place amongst all
> annotation types where one can hook in to get the real size of the
> dynamically generated appearance *1). Plus the template pattern approach
> hopefully helps poppler newbies like me to understand things better, as it
> makes common steps in Annot::draw schematic and explicit.

makeAppearance(obj, drawRect, getXRef()); is a bit ackward API, I think it would be better if you return obj instead of passing it as output parameter.

> 
> Regarding API... QImage Poppler::Annotation::renderToImage(double hDPI,
> double vDPI) was the simplest API I could imagine. Functionality explicitly
> cares about one single annotation and no other objects, thus the method is
> bound to Annotation, instead of Page. We don't need to bother clients with
> position/size arguments, poppler knows it all, and better. We probably
> don't need callbacks. Maybe we want to allow fine tuning some rendering
> settings? E.g. an enableTransparentBackground option could make sense to
> chose either page color or transparency as background. 

What would be the use case for wanting the page color as background? 

> Also a further
> method
> Poppler::Annotation::renderToArthur(...) could be useful and would be
> consistent to Poppler::Page::renderToArthur.

Guess so yeah, but i wouldnt' really worry about that for now.

Thanks for your comments Albert. I pretty much agree with everything. So this chunk will get split into a series of three patches and I will add them into one new bugzilla entry. Patches 1/3 ("qt5 code moving") and 2/3 ("template design pattern for Annot::draw") prepare the ground and can then hopefully be useful even if the actual feature in 3/3 doesn't land.

> What would be the use case for wanting the page color as background? 

I was just guessing, ignore it for now.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/401.