Created attachment 135558 [details] [review] Patch implementing ArthurOutputDev::axialShadedFill The attached patch implements ArthurOutputDev::axialShadedFill. This does not improve the rendering quality much, because the fall-back approximation code in Gfx does a pretty good job at it. However, the new code - Should be faster - Does improve rendering a little bit: Previously, axial gradients showed spurious thin white lines when rendered with the arthur backend - Is a warm-up exercise for radial gradients, which do look strange when rendered with Arthur. Unfortunately I only have a few documents with axial gradients. Some extra testing would be appreciated! Unlike CairoOutputDev the Arthur implementation does not use the updateFillColorStop mechanism, but rather implements the entire QLinearGradient setup and painting in the axialShadedFill method. This makes the code a bit longer, but: - It avoids having to add additional internal state to the ArthurOutputDev class. This makes debugging easier. - Besides computing color stops, the method Gfx::doAxialShFill (which is the one that calls updateFillColorStop) does many other things that are not actually needed when using Qt for painting the gradient. In particular, it constructs a sequence of small rectangles to approximate the gradient, which are then never used. - Gfx::doAxialShFill assumes that the output devices can only paint polygons with a single color. Therefore it needs to construct lots of such polygons to approximate a smooth gradient. However, QPainter can do linear interpolation of colors itself. The new bisection implementation in Arthur::axialShadedFill takes this into account, and therefore produces a much smaller number of color stops in many cases.
Want me to run this over the files i have here to spot any big regression or should i just commit it?
Please run this over your files. Thank you!
doesn't compile, does this need something else? /home/tsdgeos/devel/poppler/qt5/src/ArthurOutputDev.cc:750:13: error: request for member ‘top’ in ‘((ArthurOutputDev*)this)->ArthurOutputDev::m_painter’, which is of pointer type ‘QPainter*’ (maybe you meant to use ‘->’ ?) m_painter.top()->fillPath( convertPath( state, state->getPath(), Qt::WindingFill ), newBrush );
Apologies---you will need the patches from https://bugs.freedesktop.org/show_bug.cgi?id=102036 for this to compile. Somehow I was expecting you to review my patches in the order of submission. :-) If you want I can rebase this patch to the master. Or better: can you run the regression tests with the patches from 102036 and this one together? I don't really expect 102036 to introduce regressions, but extra testing certainly would not hurt.
Have a look at the attached file, the gradient is fixed (i.e. you can't see the X anymore), but it's purple instead of green.
Created attachment 136535 [details] wrongly colored shading regression
I'll have a look, but it will have to wait until february.
Created attachment 137943 [details] [review] Updated patch that fixes the color problem
I also figured out why the other shadings in that document look bad. They are all drawn by the default shader in Gfx.cc, which calls the actual output device only for drawing lots of small unicolor polygons. That code does the right thing. However, Arthur draws those polygons with antialiasing enabled, which makes them a tiny bit smaller. As a result, you get thin transparent lines around these polygons, and these are the artifacts you see. If I switch off antialiasing, then the test document is rendered correctly. The problem now is that I don't want to switch off antialiasing unconditionally. I want to switch it off when the shading operation begins, and restore it afterwards. However, there does not seem to be a hook that gets called after a shading has completed. Any ideas?
Which kind of shading it is? I can see two solutions: * Add pre/post function calls in gfx.cc before the shading is done * Implemnt the shading in arthuroutputdev, but make the implementation just be disableAA(); doWhateverTheGfxCodeDoes(); enableAA();
> Which kind of shading it is? I tried it with all those from the example document you posted in Comment 6. But it seems that I have to revisit my previous comment a little. I am simply experiment using the following hack: --- a/qt5/src/ArthurOutputDev.cc +++ b/qt5/src/ArthurOutputDev.cc @@ -777,6 +777,7 @@ void ArthurOutputDev::stroke(GfxState *state) void ArthurOutputDev::fill(GfxState *state) { + m_painter.top()->setRenderHint(QPainter::Antialiasing,false); m_painter.top()->fillPath( convertPath( state, state->getPath(), Qt::WindingFill ), m_currentBrush ); } But I just learned that this only works if the resolution we are rendering to is high enough (using test-render-to-file-qt5). So this needs more thought. > I can see two solutions: > * Add pre/post function calls in gfx.cc before the shading is done That would be easy, but it would mean a new API in OutputDev.h. It's not on me to decide whether that is a good idea. > * Implemnt the shading in arthuroutputdev, but make the implementation just be > disableAA(); > doWhateverTheGfxCodeDoes(); > enableAA(); Problematic, because doWhateverTheGfxCodeDoes() is a lot of code.
(In reply to oliver.sander from comment #11) > > Which kind of shading it is? > > I tried it with all those from the example document you posted in Comment 6. > > But it seems that I have to revisit my previous comment a little. I am > simply experiment using the following hack: > > --- a/qt5/src/ArthurOutputDev.cc > +++ b/qt5/src/ArthurOutputDev.cc > @@ -777,6 +777,7 @@ void ArthurOutputDev::stroke(GfxState *state) > > void ArthurOutputDev::fill(GfxState *state) > { > + m_painter.top()->setRenderHint(QPainter::Antialiasing,false); > m_painter.top()->fillPath( convertPath( state, state->getPath(), > Qt::WindingFill ), m_currentBrush ); > } > > But I just learned that this only works if the resolution we are rendering > to is high enough (using test-render-to-file-qt5). So this needs more > thought. > > > I can see two solutions: > > * Add pre/post function calls in gfx.cc before the shading is done > > That would be easy, but it would mean a new API in OutputDev.h. It's not on > me to decide whether that is a good idea. Do you think it would be a bad idea? > > > * Implemnt the shading in arthuroutputdev, but make the implementation just be > > disableAA(); > > doWhateverTheGfxCodeDoes(); > > enableAA(); > > Problematic, because doWhateverTheGfxCodeDoes() is a lot of code. We can put that "lot of code" in a function to allow for code reuse.
I've pushed the patch here, let's continue the other discussion either in the mailing list or in a bug about that issue in itself?
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.