[poppler] [PATCH v2 2/5] SplashXPathScanner: Reset span state explicitly when advancing line
Brüns, Stefan
Stefan.Bruens at rwth-aachen.de
Wed May 30 16:03:33 UTC 2018
On Mittwoch, 30. Mai 2018 09:31:32 CEST Albert Astals Cid wrote:
> So this saves checking
> (y < yMin || y > yMax)
> a few times in exchange of making the code more error prone since now you
> have to remember to actaully call setRow, have you measured this has an
> actual speed improvement impact?
1. It saves the y boundary check
2. It saves the "if (interY != y) {" check
3. It makes the getNextSpan single purpose - see the function description.
Previously, it was 5 lines, now it is 2.
4. It removes the required initialization of interY from the constructor.
Try to read the old variant of getNextSpan. Can you answer by looking at the
code what the purpose of interY is?
Also have a look at SplashXPathScanner::renderAALine(..) and
SplashXPathScanner::clipAALine(...). The first contains a code fragment almost
identical to getNextSpan, but it also updates the y position and accompanied
state outside the loop. After this patch, renderAALine could call
getNextSpan() instead of using its own implementation.
After the change it is obvious that intercount and interIdx are initialized
exactly once per row. You don't have to hunt down where else interY is used
(anwser: only in the constructor), where intercount and interIdx are used
(answer: in getNextSpan, renderAALine and clipAALine) and what the
relationship between these uses is.
Efficiency is a secondary concern here (although it definitely helps), code
clarity is the prime motivator for this change.
Kind regards,
Stefan
> El dissabte, 26 de maig de 2018, a les 19:51:21 CEST, Stefan Brüns va
>
> escriure:
> > Avoid checking for a line change in each span. The required state reset
> > can be done from the call site.
> > ---
> >
> > splash/Splash.cc | 9 +++++++++
> > splash/SplashXPathScanner.cc | 19 ++++++++++---------
> > splash/SplashXPathScanner.h | 12 ++++++------
> > 3 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/splash/Splash.cc b/splash/Splash.cc
> > index d749ab03..1eb907d8 100644
> > --- a/splash/Splash.cc
> > +++ b/splash/Splash.cc
> > @@ -2612,6 +2612,9 @@ SplashError Splash::fillWithPattern(SplashPath
> > *path,
> > GBool eo, }
> >
> > } else {
> >
> > for (y = yMinI; y <= yMaxI; ++y) {
> >
> > + if (!scanner->setRow(y)) {
> > + continue;
> > + }
> >
> > while (scanner->getNextSpan(y, &x0, &x1)) {
> >
> > if (clipRes == splashClipAllInside) {
> >
> > drawSpan(&pipe, x0, x1, y, gTrue);
> >
> > @@ -2736,6 +2739,9 @@ SplashError Splash::xorFill(SplashPath *path, GBool
> > eo) {
> >
> > // draw the spans
> > for (y = yMinI; y <= yMaxI; ++y) {
> >
> > + if (!scanner->setRow(y)) {
> > + continue;
> > + }
> >
> > while (scanner->getNextSpan(y, &x0, &x1)) {
> >
> > if (clipRes == splashClipAllInside) {
> >
> > drawSpan(&pipe, x0, x1, y, gTrue);
> >
> > @@ -6492,6 +6498,9 @@ SplashError Splash::shadedFill(SplashPath *path,
> > GBool hasBBox, } else {
> >
> > SplashClipResult clipRes2;
> > for (y = yMinI; y <= yMaxI; ++y) {
> >
> > + if (!scanner->setRow(y)) {
> > + continue;
> > + }
> >
> > while (scanner->getNextSpan(y, &x0, &x1)) {
> >
> > if (clipRes == splashClipAllInside) {
> >
> > drawSpan(&pipe, x0, x1, y, gTrue);
> >
> > diff --git a/splash/SplashXPathScanner.cc b/splash/SplashXPathScanner.cc
> > index 1e48e90b..b36f5322 100644
> > --- a/splash/SplashXPathScanner.cc
> > +++ b/splash/SplashXPathScanner.cc
> > @@ -122,7 +122,6 @@ SplashXPathScanner::SplashXPathScanner(SplashXPath
> > *xPathA, GBool eoA, allInter = nullptr;
> >
> > inter = nullptr;
> > computeIntersections();
> >
> > - interY = yMin - 1;
> >
> > }
> >
> > SplashXPathScanner::~SplashXPathScanner() {
> >
> > @@ -216,14 +215,6 @@ GBool SplashXPathScanner::testSpan(int x0, int x1,
> > int
> > y) { GBool SplashXPathScanner::getNextSpan(int y, int *x0, int *x1) {
> >
> > int interEnd, xx0, xx1;
> >
> > - if (y < yMin || y > yMax) {
> > - return gFalse;
> > - }
> > - if (interY != y) {
> > - interY = y;
> > - interIdx = inter[y - yMin];
> > - interCount = 0;
> > - }
> >
> > interEnd = inter[y - yMin + 1];
> > if (interIdx >= interEnd) {
> >
> > return gFalse;
> >
> > @@ -246,6 +237,16 @@ GBool SplashXPathScanner::getNextSpan(int y, int *x0,
> > int *x1) { return gTrue;
> >
> > }
> >
> > +GBool SplashXPathScanner::setRow(int y) {
> > + if (y < yMin || y > yMax) {
> > + return gFalse;
> > + }
> > +
> > + interCount = 0;
> > + interIdx = inter[y - yMin];
> > + return gTrue;
> > +}
> > +
> >
> > void SplashXPathScanner::computeIntersections() {
> >
> > SplashXPathSeg *seg;
> > SplashCoord segXMin, segXMax, segYMin, segYMax, xx0, xx1;
> >
> > diff --git a/splash/SplashXPathScanner.h b/splash/SplashXPathScanner.h
> > index b6c358d9..99f2c28f 100644
> > --- a/splash/SplashXPathScanner.h
> > +++ b/splash/SplashXPathScanner.h
> >
> > @@ -69,13 +69,14 @@ public:
> > // path.
> > GBool testSpan(int x0, int x1, int y);
> >
> > - // Returns the next span inside the path at <y>. If <y> is
> > - // different than the previous call to getNextSpan, this returns the
> > - // first span at <y>; otherwise it returns the next span (relative
> > - // to the previous call to getNextSpan). Returns false if there are
> > - // no more spans at <y>.
> > + // Returns the next span inside the path at <y>.
> > + // Returns false if there are no more spans at <y>.
> >
> > GBool getNextSpan(int y, int *x0, int *x1);
> >
> > + // Sets the current span to the first span at <y>.
> > + // Returns false if the row is outside the path.
> > + GBool setRow(int y);
> > +
> >
> > // Renders one anti-aliased line into <aaBuf>. Returns the min and
> > // max x coordinates with non-zero pixels in <x0> and <x1>.
> > void renderAALine(SplashBitmap *aaBuf, int *x0, int *x1, int y,
> >
> > @@ -102,7 +103,6 @@ private:
> > int allInterLen; // number of intersections in <allInter>
> > int allInterSize; // size of the <allInter> array
> > int *inter; // indexes into <allInter> for each y value
> >
> > - int interY; // current y value - used by getNextSpan
> >
> > int interIdx; // current index into <inter> - used by
> >
> > // getNextSpan
> >
> > int interCount; // current EO/NZWN counter - used by
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/poppler
More information about the poppler
mailing list