[poppler] [PATCH] Arthur backend fixes; poppler-qt4 direct rendering

[Matthias Fauconneau]

> - try to avoid mixing indentation and actual code changes, they make
> reviewing the changes a bit difficult

Some projects don't allow cosmetic fixes and indentations only gets
fixed when the code is changed.
So how should I create my patch ?
If diff ignore whitespace only changes, the indentations will be
half-fixed which is worse.
I could break it in two, a cosmetic patch and then the fixes patch.
but it will complicate my workflow.
I'll try to do better next time :)

> - using QImage::scanLine() can be not that fast, especially if called
> repeately. much better if you iterate "by row" manually yourself of the
> image buffer
I'm not sure the few checks in scanLine are relevant, but I fixed it anyway.

> - in the past I had issues with Format_RGB32 and the alpha pixels not
> being totally opaque (ie 0xff): you can just use ARGB32 for both the
> cases (creating the QImage out of the if(), and on stack) and reset to
> 0xff the alpha values

Fixed, I also merged the two loops, I hope the per row maskColors test
is not a too big performance hit for you :).
I think it makes the code cleaner. I would expect an optimizing
compiler to make two loops anyway.

> I'm not sure to like the implementation of basically making (again) a
> rendering method specific to a backend, with a generic method...

True, but I don't see any other method to have OpenGL PDF rendering.
The QPainter intermediate representation is also very useful for my application.
It allows me to recognize notes in a PDF and highlight them in sync
with a MIDI source.

> Anyway, for renderToPainter:
> - it should check for the painter pointer (if 0)
Done.
> - it should have a parameter of a flag type for options for the "painter
>  way" rendering
I added PainterFlags

> - it should by default save+restore the painter, to avoid messying a
>  caller-owned painter; the flag parameter proposed above should have an
>  enum saying to avoid this save/restore job (similar to QGraphicsItem);

I didn't find what you're talking about in QGraphicsItem documentation page.
So I made a SaveRestore flag which is default.
I think it's clearer than to check a double negation if( !(flags &
NoSaveRestore) ) painter.save();

>  renderToImage should use this enum value, of course
Done. renderToImage clear the SaveRestore flag

> - "renderToPainter(&painter,xres,yres,x,y,w,h,rotate);"
>  please add spaces after commas
Fixed. I'm very minimalist in my whitespace usage :)

Also why are the local variables declared at the beginning of the
functions everywhere ?
It makes the code quite hard to read and is only mandatory for some
old C standard.

I'm also removing save/restore in drawChar().
I think they are unnecessary overhead. it should be handled correctly
in saveState/restoreState.
Feel free to remove it if you find PDF's which are broken with this
change but since Arthur wasn't working before anyway, there shouldn't
be any complaints :D.

I hope Arthur quality will be good enough that someday Okular will be
able to scroll through an entire PDF at 60 fps without any cache.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arthur.patch
Type: application/octet-stream
Size: 12169 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20101106/32862b70/attachment-0001.obj>