[poppler] [PATCH] Arthur backend fixes; poppler-qt4 direct rendering
matthias.fauconneau at gmail.com
Sat Nov 6 02:19:25 PDT 2010
> - 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)
> - 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
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...
Size: 12169 bytes
Desc: not available
More information about the poppler