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

[Albert Astals Cid]

A Dissabte, 6 de novembre de 2010, Matthias Fauconneau va escriure:
> > - 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.

As you can see Pino commited your patch and it will be part of poppler 0.15.2 
that i plan to release "soon (TM)"

Albert