Bug 89586 - Drivers/DRI/swrast
Summary: Drivers/DRI/swrast
Status: RESOLVED MOVED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/swrast (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-16 08:02 UTC by Dan Sebald
Modified: 2019-09-18 18:45 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Example image with droped pixels during zoom, creating vertical lines in image (9.60 KB, image/png)
2015-03-16 08:02 UTC, Dan Sebald
Details
Minimal changeset to fix vertical lines when -1 < xfactor < 1 (7.54 KB, text/plain)
2015-03-16 08:07 UTC, Dan Sebald
Details
Illustration of why ceil() is the correct operation to apply to zoom box (6.29 KB, image/png)
2015-03-16 08:09 UTC, Dan Sebald
Details
Example gradient image resulting after changeset applied. Vertical lines gone. (16.17 KB, image/png)
2015-03-16 08:12 UTC, Dan Sebald
Details
Changeset to fix vertical lines and fine tune positive_unzoom_x() and negative_unzoom_x() (12.67 KB, text/plain)
2015-03-16 08:20 UTC, Dan Sebald
Details
Piglit pixelzoom test suite (17.33 KB, patch)
2015-03-24 19:24 UTC, Dan Sebald
Details | Splinter Review
swrast legacy Piglit test images (18.84 KB, image/png)
2015-03-24 19:34 UTC, Dan Sebald
Details
swrast legacy with patch Piglit test images (18.77 KB, image/png)
2015-03-24 19:36 UTC, Dan Sebald
Details
swrast Gallium Piglit test images (18.81 KB, image/png)
2015-03-24 19:37 UTC, Dan Sebald
Details
swrast Gallium with patch Piglit test images (18.81 KB, image/png)
2015-03-24 19:38 UTC, Dan Sebald
Details
swrast legacy Piglit test images (18.92 KB, image/png)
2015-03-24 20:13 UTC, Dan Sebald
Details
swrast legacy with patch Piglit test images (18.83 KB, image/png)
2015-03-24 20:14 UTC, Dan Sebald
Details
swrast Gallium Piglit test images (18.90 KB, image/png)
2015-03-24 20:15 UTC, Dan Sebald
Details
swrast Gallium with patch Piglit test images (18.90 KB, image/png)
2015-03-24 20:17 UTC, Dan Sebald
Details
Piglit pixelzoom test suite (18.59 KB, patch)
2015-03-25 17:00 UTC, Dan Sebald
Details | Splinter Review
Changeset to fix vertical lines and fine tune positive_unzoom_x() and negative_unzoom_x() (12.87 KB, patch)
2015-03-30 04:33 UTC, Dan Sebald
Details | Splinter Review
Piglit pixelzoom test suite (18.71 KB, patch)
2015-03-30 04:36 UTC, Dan Sebald
Details | Splinter Review
swrast legacy Piglit test images (18.99 KB, image/png)
2015-03-30 04:38 UTC, Dan Sebald
Details
swrast legacy with patch Piglit test images (19.00 KB, image/png)
2015-03-30 04:40 UTC, Dan Sebald
Details
swrast Gallium Piglit test images (18.35 KB, image/png)
2015-03-30 04:41 UTC, Dan Sebald
Details
swrast Gallium with patch Piglit test images (19.00 KB, image/png)
2015-03-30 04:42 UTC, Dan Sebald
Details

Description Dan Sebald 2015-03-16 08:02:15 UTC
Created attachment 114338 [details]
Example image with droped pixels during zoom, creating vertical lines in image

When plotting large images (images that typically span in the x-direction greater than GL_MAX_TEXTURE_SIZE) with an application using Mesa/OpenGL, several developers with quite varying versions of Mesa were seeing vertical white lines spaced at GL_MAX_TEXTURE_SIZE.  I assume that Mesa is displaying images in hunks of GL_MAX_TEXTURE_SIZE x GL_MAX_TEXTURE_SIZE or less, and it seems that--at least in the X-dimension--that either the first pixel or the last pixel is being dropped.

I've attached an example gradient image run through a palette called Screenshot-Mesa-VerticalLines.png.  The various colors should make the vertical lines easily visible.  This sample image has an input buffer which is 40,000 pixels wide, enough to span a couple GL_MAX_TEXTURE_SIZE (16384) pixels.  This image as a pixel scaling factor which is quite small, approximately 1/100.  But that doesn't seem unreasonably small, meaning it's not in the realm where it should coincide with numerical issues.

After getting to know the Mesa code a bit and compiling from scratch locally using PKG_CONFIG_PATH, I eventually tracked down the problem to this hunk of code (which appears at several different locations in the code, based upon the type of data being worked on) in the file s_zoom.c:

         for (i = 0; i < zoomedWidth; i++) {
            GLint j = unzoom_x(ctx->Pixel.ZoomX, imgX, x0 + i) - span->x;
            assert(j >= 0);
            assert(j < (GLint) span->end);
            COPY_4V(zoomed.array->attribs[VARYING_SLOT_COL0][i], rgba[j]);
         }

When printing out these variable values on the fly when j happens to be less than zero (one of the assert conditions) when x0+i = x0 (i.e., i=0, or the "first" pixel in the zoomed image block).  For example,

j=-71, imgX=73, x0=250, i=0, zoom=0.010850
j=-50, imgX=73, x0=428, i=0, zoom=0.010850

But it strikes me that the j < 0 or j > span->end cases cannot be ignored, i.e., tossed out via the assert(), because these correspond to an i value of the mapped output image which is visible.  There has to be something there.  Basically, my thinking is that, approximately, -1/xfactor < j < 0 corresponds to a valid pixel, it's just that the pixel crosses the memory chunk/block boundary in the transformation (the code used the word "chunks", so I will use chunk).  That is, the pixel for j < 0 should come from the previous GL_MAX_TEXTURE_SIZE/SWRAST_MAX_WIDTH chunk of data.

I'm going to attach two git changesets to address this issue, a minimal changeset and a more thorough changeset.  The first changeset will be something just to make the current code work by adding a few lines and show where the main bug is.  The second changeset has a few extra changes, mostly straightforward, but when taken as a whole they might seem confusing.  I'll attach the second changeset as a different post to help organization.

So I'll explain how I went about this and my thought process for making those changes in the changeset.  First, let's verify this formula is correct:

            GLint j = unzoom_x(ctx->Pixel.ZoomX, imgX, x0 + i) - span->x;

I will refer to 'j' as the "chunk" unzoomed x.  Elsewhere I'll use the OpenGL nomenclature of 'n' as the pixel array unzoomed x.  By the OpenGL definition when x0 + i = imgX the value of j (and n, because it is the first block) should be exactly zero.  That condition is the scenario of looking at the first unzoomed pixel.  I will print out the value as follows:

            GLint j = unzoom_x(ctx->Pixel.ZoomX, imgX, x0 + i) - span->x;
if (x0 + i == imgX)
  fprintf(stderr, "x0+i = imgX: j=%d\n", j);

Here is the result:

x0+i = imgX: j=0

That's a good result, and it suggests that those values of j < 0 aren't some bogus offset or some numerical problem.

Now, let's verify that j < 0 is a valid value, corresponding to a pixel by testing whether it correctly fits the OpenGL standard for zooming a pixel array listed here:

https://www.opengl.org/sdk/docs/man2/xhtml/glPixelZoom.xml

  xr + n * xfactor <= xz < xr + (n+1) * xfactor  (1)
  yr + m * yfactor <= yz < yr + (m+1) * xfactor  (2)

Rather than list the commands for printing, I'll just state what I'm printing from this point forward.  I will run the same app program and this time print out the value of j when j < 0, the zoomed pixel value, and the formula shown in the link above:

j=-71:  spanX=16457, imgX=73, spanX-imgX=16384
          (j+spanX-imgX)*ZoomX <= x0+i-imgX < (j+spanX-imgX+1)
                  176.996048    <=    177    <    177.006897
j=-50:  spanX=32841, imgX=73, spanX-imgX=32768
          (j+spanX-imgX)*ZoomX <= x0+i-imgX < (j+spanX-imgX+1)
                  354.990295    <=    355    <    355.001160

OK, that looks correct to me:  spanX-imgX is a factor of SWRAST_MAX_WIDTH (check), so we are effectly adding a factor of SWRAST_MAX_WIDTH to j (check), I've made sure to subtract imgX for all values because xr is treated as the origin of the scaling formula (check) and when we apply the glPixelZoom formulas the center of the pixel in the zoomed image lies right between the range limits (check).  So I conclude that those are valid pixels, and when j < 0 the algorithm needs to effectively refer to the previous chunk of data to get the data value to be displayed in the zoomed pixel array.

There's at least one bug then.

After trying various ideas and not being happy with the cumbersome programming I was producing (e.g., calling the routine twice to pick up overlapping, etc.), I found the solution is actually rather straightforward.  All that is needed is computing the bounds for the span-X for-loop more accurately.  The proper formula for computing the span limits involves ceiling, and the current code is doing something akin to floor, so the loop limits are off by one, hence the dropped pixel.  To see that ceiling is correct, I've included an illustration in glpixelzoom_illustration.png in a couple cases where the zoom rectangle falls between pixel centers and when it falls on pixel centers.  Remember that the definition states that pixels centers on the bottom or left edge qualify for inclusion.  (If there is a subtlety here I'm missing, please respond.  I'm sure developers have more experience with this than I have.)

There is another advantage of exactly computing the bounds: those sanity checks within all the loops are no longer necessary.  Because the zoom/unzoom operations are essentially linear, or let's say monotonic (still sufficient), if it is known the end-point indeces are a valid transformation, then all the indeces inbetween are valid transforms.  In the patch, I've removed all the assert(j >= 0), assert(j< Xmax).  Works fine.

After making that change above to readjuste the x-index limits, the vertical lines are gone.  See the attached figure Screenshot-Mesa-VerticalLinesFixed.png.  The right-most edge of the image appears to no-longer be short of the black vertical boundary as well.  Now it could be that the border lines aren't always exactly correct (if so I'm not concerned with that right now, might be simply the application not programming OpenGL correctly).

Notice that the white artifact along the bottom border is still present.  The misalignment on the right edge always seems to be none or too wide.  The misalignment on the bottom edge always seems to be none or too short.  I think that has to do with the fact that the x-dimension is using xfactor > 0 and the y-dimension is using xfactor < 0.

I've tried different input image sizes, 0 < xfactor < 1 (large number of x-pixels), xfactor > 1 (few xpixels), -1 < yfactor < 0, yfactor < -1.  So that has run the algorithm through its paces and I've seen no problems.
Comment 1 Dan Sebald 2015-03-16 08:07:30 UTC
Created attachment 114339 [details]
Minimal changeset to fix vertical lines when -1 < xfactor < 1
Comment 2 Dan Sebald 2015-03-16 08:09:20 UTC
Created attachment 114340 [details]
Illustration of why ceil() is the correct operation to apply to zoom box
Comment 3 Dan Sebald 2015-03-16 08:12:32 UTC
Created attachment 114341 [details]
Example gradient image resulting after changeset applied.  Vertical lines gone.
Comment 4 Dan Sebald 2015-03-16 08:18:11 UTC
I'm now going to attach the full patch which fixes what I believe is another subtle bug and attempts to make things slightly more efficient in the innermost loop.

I notice in the following hunk of code from s_zoom.c:

static inline GLint
unzoom_x(GLfloat zoomX, GLint imageX, GLint zx)
{
   /*
   zx = imageX + (x - imageX) * zoomX;
   zx - imageX = (x - imageX) * zoomX;
   (zx - imageX) / zoomX = x - imageX;
   */
   GLint x;
   if (zoomX < 0.0)
      zx++;
   x = imageX + (GLint) ((zx - imageX) / zoomX);
   return x;
}

there is a test for zoomX < 0.0.  That zoomX test is run for every unzoomed computation, which can be a lot of tests if the image displayed is large.  (I will say more about efficiency later.)  That is, in the calling routine:

         for (i = 0; i < zoomedWidth; i++) {
            GLint j = unzoom_x(ctx->Pixel.ZoomX, imgX, x0 + i) - span->x;
            assert(j >= 0);
            assert(j < (GLint) span->end);
            COPY_4V(zoomed.array->attribs[VARYING_SLOT_COL0][i], rgba[j]);
         }

that value of ctx->Pixel.ZoomX is a constant.  So, really, the zoom sign test could be brought outside the for-loop, and perhaps increase efficiency by, oh, maybe 25%.  (For the loop, but there is inefficiency elsewhere of course so may not be noticeable.  However, I'm one who tends to aim for efficiency.)  That is, this "if (zoomX < 0.0)" is probably one or two machine instructions, while "x = imageX + (GLint) ((zx - imageX) / zoomX);" is probably about five machine instructions.

In the full patch I have replaced the unzoom_x() with a positive_unzoom_x() and negative_unzoom_x().  As you might imagine, the changeset then first tests the polarity of zoomX the picks a loop to run, depending on the sign of zoomX.  This is done in a clean and graceful way using a macro called SPAN_LOOP_X() where the argument is one or two lines of code that is run inside the loop, i.e., the memory copy.

Now, having done the positive/negative_unzoom_x() construct, I also wrote some more extensive formula derivations in comments in the code to show what I think is a discrepancy with the formula of unzoom_x() above, in particular at least this

   if (zoomX < 0.0)
      zx++;

By my derivation, the addition (effectively subtraction) should be done after the division operation.  Doing the addition before dividing is something different that really doesn't have the same effect.  Also, notice there is the subtle case of ((xz - xr) / xfactor) being an integer that isn't properly addressed.  It takes some time to think through, and if you don't agree with my derivation/code, point out what is wrong and we can refine things.
Comment 5 Dan Sebald 2015-03-16 08:20:17 UTC
Created attachment 114342 [details]
Changeset to fix vertical lines and fine tune positive_unzoom_x() and negative_unzoom_x()
Comment 6 Dan Sebald 2015-03-16 08:36:58 UTC
One last comment about the efficiency of the pixel zooming.  This routine seems slow when I rescale the image; it's tolerable, but could be faster.  There is a setup of the memory addresses and context for every row of the image written to the frame buffer.  malloc() is used every row as well.

I think things could be simplified if in s_drawpix.c a single big memory buffer is first created and these routines that "write" spans could be changed to simply "fill" the big memory buffer.  That would avoid the setting up of the frame buffer memory for each row.  Instead, it would just be an extra tight loop inside SPAN_LOOP_X() that copies r1-r0-1 rows.  Then after the big buffer is filled with the image (pretty much the same loop as is currently done for writing), then use the fast image write routine.  That would then be just one malloc(), filled buffer piecewise, write to frame buffer, free().

To get an accurate pixel size for the zoomed image, put the declaration

static GLboolean
compute_zoomed_bounds(struct gl_context *ctx, GLint imageX, GLint imageY,
                      GLint spanX, GLint spanY, GLint spanWidth,
                      GLint *x0, GLint *x1, GLint *y0, GLint *y1)

in s_zoom.h.  With the changes made in the attached changeset, that routine should be very accurate with something like

compute_zoomed_bounds(ctx, imageX, imageY, 0, 0, imageX, &x0, &x1, &y0, &y1);
Comment 7 Ilia Mirkin 2015-03-17 05:33:55 UTC
Would you be able to supply a short program that reproduces the issue that could be added to the piglit test suite? (http://cgit.freedesktop.org/piglit) There are two tests in there that make use of glPixelZoom, but very far from a thorough test.

Also, you're using swrast, which has seen very little activity of late -- do these issues happen with the more up-to-date and commonly used gallium-based softpipe or llvmpipe drivers?
Comment 8 Dan Sebald 2015-03-17 06:13:27 UTC
I cloned the piglit repository and will see if I can write a program.  It shouldn't be difficult once I figure out the source tree organization, but it might take me a couple days.

I see the transition happening away from swrast to Gallium, but I'm guessing linux bundles might still be relying on swrast as I know the other folks who mentioned the lines keep their systems more up-to-date.  There is also at least one driver in Gallium that will fall back on swrast under certain circumstances.

I was just in the process of creating a bug report for a related type of problem in the Gallium driver for glPixelZoom.  However, the Gallium organization isn't so straightforward and I've not yet figured out exactly which code gets run via Gallium.  Whatever is compiled is also named swrast_dri.so in the lib/gallium subdirectory, but it is clearly a different implementation of pixel zoom.  (Only the first 8192 pixels zoomed appear in the image while the rest is background-color, in both x and y dimensions.  So I can't tell if there are dropped pixels in that case just yet.)  I will look through softpipe and llvmpipe for the source code.
Comment 9 Ilia Mirkin 2015-03-17 06:32:14 UTC
(In reply to Dan Sebald from comment #8)
> I cloned the piglit repository and will see if I can write a program.  It
> shouldn't be difficult once I figure out the source tree organization, but
> it might take me a couple days.

Basically the way it works is... find a simple test, and copy it. I recently wrote tests/spec/ext_polygon_offset_clamp/*.c -- iirc 2 tests, both very simple. Not sure when glPixelZoom came about, but it would probably go into tests/spec/gl-1.0 or gl-2.0 or whatever. Add it to the CMakeFile.gl.txt and you're good to go -- rerun 'cmake .' and then you should be able to do 'make your-great-test-name'. Feel free to ask for help if you get stuck... it's all a bit annoying as it uses cmake. At least waffle has made it to most distro's packaging systems by now.

> 
> I see the transition happening away from swrast to Gallium, but I'm guessing
> linux bundles might still be relying on swrast as I know the other folks who
> mentioned the lines keep their systems more up-to-date.  There is also at
> least one driver in Gallium that will fall back on swrast under certain
> circumstances.

No. Gallium drivers can't fall back on swrast. There is a 'draw' module which is used by a few drivers though to handle some (or all, in case of i915g) vertex shader matters. Note that there's a lot of confusion with the 'swrast' name, since the gallium software renderers can also be called swrast, and there are 2 of them.

> 
> I was just in the process of creating a bug report for a related type of
> problem in the Gallium driver for glPixelZoom.  However, the Gallium
> organization isn't so straightforward and I've not yet figured out exactly
> which code gets run via Gallium.  Whatever is compiled is also named
> swrast_dri.so in the lib/gallium subdirectory, but it is clearly a different
> implementation of pixel zoom.  (Only the first 8192 pixels zoomed appear in
> the image while the rest is background-color, in both x and y dimensions. 
> So I can't tell if there are dropped pixels in that case just yet.)  I will
> look through softpipe and llvmpipe for the source code.

Take a look at src/mesa/state_tracker/st_cb_drawpixels.c. This is the "state tracker" which is effectively an adapter between the OpenGL and Gallium APIs.
Comment 10 Dan Sebald 2015-03-17 07:18:08 UTC
OK, src/mesa/state_tracker/st_cb_drawpixels.c is it, thanks.  Saved me a lot of time.

And here's a comment about image size in the code:

   /* Limit the size of the glDrawPixels to the max texture size.
    * Strictly speaking, that's not correct but since we don't handle
    * larger images yet, this is better than crashing.
    */

I simply removed the limit and the image displays fine with no segfaults.

There is a tiny discrepancy compared with the swrast/ result, however.  There may still be a bug floating about.  I will explain in a different bug report.
Comment 11 Dan Sebald 2015-03-19 19:21:18 UTC
"it's all a bit annoying as it uses cmake"

Yes, as bit, but eventually was able to make everything.  The one significant issue was making Waffles in /usr/local couldn't be found by Piglit ccmake.  Because cmake has no uninstall, I manually deleted all the files Waffles installed and then used the prefix "/usr" to make Waffles.  Piglit could then find Waffles.

Anyway, writing the C program shouldn't be difficult, but I can't seem to get the basics working correctly.  When I built Piglit all seemed OK, at least there was about 8 minutes to the build process as it methodically stepped through directories building things.  However, nothing I supply to ./piglit seems to be accepted:

[sebald@ piglit]$ ./piglit run
usage: piglit [-h] [-n <test name>] [-d] [-t <regex>] [-x <regex>]
              [-b {json,junit}] [-c | -1]
              [-p {glx,x11_egl,wayland,gbm,mixed_glx_egl}] [--valgrind]
              [--dmesg] [-s] [--junit_suffix JUNIT_SUFFIX] [-f config_file]
              [-v | -l {quiet,verbose,dummy}] [--test-list TEST_LIST]
              <Profile paths)> [<Profile path(s> ...] <Results Path>
piglit: error: too few arguments
[sebald@ piglit]$ ./piglit run -h
usage: piglit run
piglit run: error: unrecognized arguments: -h
[sebald@ piglit]$ ./piglit run sanity results/sanity
usage: piglit run
piglit run: error: unrecognized arguments: sanity results/sanity
[sebald@ piglit]$ ./piglit run tests/sanity.py results/sanity
usage: piglit run
piglit run: error: unrecognized arguments: tests/sanity.py results/sanity
Comment 12 Ian Romanick 2015-03-19 19:28:57 UTC
(In reply to Dan Sebald from comment #11)
> "it's all a bit annoying as it uses cmake"
> 
> Yes, as bit, but eventually was able to make everything.  The one
> significant issue was making Waffles in /usr/local couldn't be found by
> Piglit ccmake.  Because cmake has no uninstall, I manually deleted all the
> files Waffles installed and then used the prefix "/usr" to make Waffles. 
> Piglit could then find Waffles.
> 
> Anyway, writing the C program shouldn't be difficult, but I can't seem to
> get the basics working correctly.  When I built Piglit all seemed OK, at
> least there was about 8 minutes to the build process as it methodically
> stepped through directories building things.  However, nothing I supply to
> ./piglit seems to be accepted:

The individual programs are just in the bin/ directory, so you can run your test directly from there.  To run it from the piglit command, you'll have to add your test to tests/all.py.  './piglit run tests/quick.py results/' should work.
Comment 13 Dan Sebald 2015-03-19 19:48:03 UTC
OK, thanks.  I'll just run the executable binaries then:

[sebald@ piglit]$ bin/ext_polygon_offset_clamp-draw 
piglit: error: waffle_config_choose failed due to WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in order to request an OpenGL version not equal to the default value 1.0
piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility Context
piglit: info: Failed to create any GL context
PIGLIT: {"result": "skip" }

I'll look into this problem later today.  I suppose I missed a build switch somewhere along the way.
Comment 14 Ilia Mirkin 2015-03-19 19:51:36 UTC
(In reply to Dan Sebald from comment #11)
> "it's all a bit annoying as it uses cmake"
> 
> Yes, as bit, but eventually was able to make everything.  The one
> significant issue was making Waffles in /usr/local couldn't be found by
> Piglit ccmake.  Because cmake has no uninstall, I manually deleted all the
> files Waffles installed and then used the prefix "/usr" to make Waffles. 
> Piglit could then find Waffles.

Too late now, but you were looking for PKG_CONFIG_PATH="/usr/local/lib/pkgconfig:/usr/lib/pkgconfig"

> 
> Anyway, writing the C program shouldn't be difficult, but I can't seem to
> get the basics working correctly.  When I built Piglit all seemed OK, at
> least there was about 8 minutes to the build process as it methodically

Time to upgrade your CPU, or use "make -j8" :)

> stepped through directories building things.  However, nothing I supply to
> ./piglit seems to be accepted:
> 
> [sebald@ piglit]$ ./piglit run

As Ian said, don't worry about that. Just run 'bin/gl-1.0-pixelzoom -fbo -auto', or omit the -fbo -auto if you want it to see why it's not doing the thing you wanted it to do.

For reference, this is how I do full piglit runs on nouveau:

http://people.freedesktop.org/~imirkin/
Comment 15 Ilia Mirkin 2015-03-19 19:55:26 UTC
(In reply to Dan Sebald from comment #13)
> OK, thanks.  I'll just run the executable binaries then:
> 
> [sebald@ piglit]$ bin/ext_polygon_offset_clamp-draw 
> piglit: error: waffle_config_choose failed due to
> WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in
> order to request an OpenGL version not equal to the default value 1.0
> piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility
> Context
> piglit: info: Failed to create any GL context
> PIGLIT: {"result": "skip" }
> 
> I'll look into this problem later today.  I suppose I missed a build switch
> somewhere along the way.

As the error says, you appear to be missing GLX_ARB_create_context, and that particular test requires GL 2.1... are you running this on X? Can you provide the output of 'glxinfo' with any LD_LIBRARY_PATH's that you use to run the tests? [Also, the classic swrast doesn't have support for that extension, so you won't get too far if that's what you're testing... either softpipe or llvmpipe should have it though. Or pick a different test.]
Comment 16 Ian Romanick 2015-03-19 20:00:50 UTC
(In reply to Ilia Mirkin from comment #15)
> (In reply to Dan Sebald from comment #13)
> > OK, thanks.  I'll just run the executable binaries then:
> > 
> > [sebald@ piglit]$ bin/ext_polygon_offset_clamp-draw 
> > piglit: error: waffle_config_choose failed due to
> > WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in
> > order to request an OpenGL version not equal to the default value 1.0
> > piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility
> > Context
> > piglit: info: Failed to create any GL context
> > PIGLIT: {"result": "skip" }
> > 
> > I'll look into this problem later today.  I suppose I missed a build switch
> > somewhere along the way.
> 
> As the error says, you appear to be missing GLX_ARB_create_context, and that
> particular test requires GL 2.1... are you running this on X? Can you

GLX_ARB_create_context should not be necessary for an OpenGL 2.1 context... since OpenGL 2.1 predates that extension by many years.  If that test asks for something less than 3.0, Waffle should create the context then check the version.

> provide the output of 'glxinfo' with any LD_LIBRARY_PATH's that you use to
> run the tests? [Also, the classic swrast doesn't have support for that
> extension, so you won't get too far if that's what you're testing... either
> softpipe or llvmpipe should have it though. Or pick a different test.]
Comment 17 Ilia Mirkin 2015-03-19 20:06:19 UTC
(In reply to Ian Romanick from comment #16)
> (In reply to Ilia Mirkin from comment #15)
> > (In reply to Dan Sebald from comment #13)
> > > OK, thanks.  I'll just run the executable binaries then:
> > > 
> > > [sebald@ piglit]$ bin/ext_polygon_offset_clamp-draw 
> > > piglit: error: waffle_config_choose failed due to
> > > WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in
> > > order to request an OpenGL version not equal to the default value 1.0
> > > piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility
> > > Context
> > > piglit: info: Failed to create any GL context
> > > PIGLIT: {"result": "skip" }
> > > 
> > > I'll look into this problem later today.  I suppose I missed a build switch
> > > somewhere along the way.
> > 
> > As the error says, you appear to be missing GLX_ARB_create_context, and that
> > particular test requires GL 2.1... are you running this on X? Can you
> 
> GLX_ARB_create_context should not be necessary for an OpenGL 2.1 context...
> since OpenGL 2.1 predates that extension by many years.  If that test asks
> for something less than 3.0, Waffle should create the context then check the
> version.

Yeah.... that's what would make sense to me too, but I'm just reading the error output here :)

https://github.com/waffle-gl/waffle/blob/master/src/waffle/glx/glx_config.c#L72

Seems like the context creation logic should handle it just fine:

https://github.com/waffle-gl/waffle/blob/master/src/waffle/glx/glx_context.c#L164
Comment 18 Dan Sebald 2015-03-19 20:40:28 UTC
Oh, that's right.  Somehow I have to build or run the piglit tests using the local build of Mesa (which is repository-up-to-date), i.e., what is accomplished with

PKG_CONFIG_PATH=/home/sebald/local/pkgconfig ./configure

otherwise.  (I've left the distro version of Mesa intact for fear changing that will have a major consequence on apps on my system.)

I'm trying to get the local library recognized, no luck but I'll keep trying...


"
Too late now, but you were looking for PKG_CONFIG_PATH="/usr/local/lib/pkgconfig:/usr/lib/pkgconfig"
"

I thought that /usr/local is a directory that is automatically searched.  In any case, maybe I could use that to get the library recognized.  But it is cmake... oh, PKG_CONFIG_PATH= appears to work with ccmake . as well.  Ah, now that compiles quickly.  The first time around building piglit was showing all kinds of green and red comments.

However, I'm getting the same error.  I don't think the local version of libGL.so is being used still.  There is no package definition in ~/local/lib/pkgconfig for libGL.

I think I have enough to keep me going here.  Thanks.
Comment 19 Dan Sebald 2015-03-20 02:58:22 UTC
I've tried variations on the include and library directories but not much luck.  Remade Waffles in /usr/local/lib64.  I did get the local version of Mesa library in the test programs:

[sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64 bin/ext_polygon_offset_clamp-draw
libGL error: unable to load driver: nouveau_dri.so
libGL error: driver pointer missing
libGL error: failed to load driver: nouveau
piglit: error: waffle_config_choose failed due to WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in order to request an OpenGL version not equal to the default value 1.0
piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility Context
piglit: info: Failed to create any GL context
PIGLIT: {"result": "skip" }

as evidenced by the attempt to load nouveau_dri.so.  However, I think maybe my X11 is too old for Waffles.  I've not upgraded since the demise of Gnome 2 series.  FWIW, I looked at glxinfo and see no GLX_ARB_create_context in any of the library lists.
Comment 20 Ilia Mirkin 2015-03-20 03:04:34 UTC
(In reply to Dan Sebald from comment #19)
> I've tried variations on the include and library directories but not much
> luck.  Remade Waffles in /usr/local/lib64.  I did get the local version of
> Mesa library in the test programs:
> 
> [sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64
> bin/ext_polygon_offset_clamp-draw
> libGL error: unable to load driver: nouveau_dri.so
> libGL error: driver pointer missing
> libGL error: failed to load driver: nouveau
> piglit: error: waffle_config_choose failed due to
> WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in
> order to request an OpenGL version not equal to the default value 1.0
> piglit: error: Failed to create waffle_config for OpenGL 2.1 Compatibility
> Context
> piglit: info: Failed to create any GL context
> PIGLIT: {"result": "skip" }
> 
> as evidenced by the attempt to load nouveau_dri.so.  However, I think maybe
> my X11 is too old for Waffles.  I've not upgraded since the demise of Gnome
> 2 series.  FWIW, I looked at glxinfo and see no GLX_ARB_create_context in
> any of the library lists.

FYI, MATE is carrying on the Gnome2 banner (http://mate-desktop.org/). What version of X are you using? Do you have glx loaded? (In _very_ old versions of X, you had to explicitly say 'Load "glx"' in the config.)

Just edit the waffle code to remove that stupid check, it's likely BS:

https://github.com/waffle-gl/waffle/blob/master/src/waffle/glx/glx_config.c#L73

            if (!wcore_config_attrs_version_eq(attrs, 10) && !dpy->ARB_create_context) {

Make that say like

if (wcore_config_attrs_version_ge(attrs, 30) && !dpy->ARB_create_context)

If anything else fails in that function, just remove that check... it should work out.
Comment 21 Ilia Mirkin 2015-03-20 03:09:44 UTC
(In reply to Dan Sebald from comment #19)
> I've tried variations on the include and library directories but not much
> luck.  Remade Waffles in /usr/local/lib64.  I did get the local version of

[...]

> WAFFLE_ERROR_UNSUPPORTED_ON_PLATFORM: GLX_ARB_create_context is required in
> order to request an OpenGL version not equal to the default value 1.0

Or alternatively just pick a different test. Like bin/gl-1.0-<tab>. Those should request a 1.0 context and waffle should be perfectly happy.
Comment 22 Dan Sebald 2015-03-20 04:20:23 UTC
glxinfo indicates GLX Version 1.4.

I did find some tests that work without the GLX_ARB_create_context error.  There seems to be no correlation with 1.0, 2.0, etc.  For example, this one works:

[sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64 bin/gl-2.1-pbo 
libGL error: unable to load driver: nouveau_dri.so
libGL error: driver pointer missing
libGL error: failed to load driver: nouveau
PIGLIT: {"subtest": {"test_sanity" : "pass"}}
PIGLIT: {"subtest": {"test_draw_pixels" : "pass"}}
PIGLIT: {"subtest": {"test_pixel_map" : "pass"}}
PIGLIT: {"subtest": {"test_bitmap" : "pass"}}
PIGLIT: {"subtest": {"test_tex_image" : "pass"}}
PIGLIT: {"subtest": {"test_tex_sub_image" : "pass"}}
PIGLIT: {"subtest": {"test_polygon_stip" : "pass"}}
PIGLIT: {"subtest": {"test_error_handling" : "pass"}}

while most others don't.  The only thing that stands out is that in the C files for this test is a main() definition and in that definition there appears to be GLUT creating the context.  The example ext_polygon_offset_clamp-draw has no main so I assume that is worked out via #include "piglit-util-gl.h" and Waffles.

But you'd want me to write a glPixelZoom test with the Waffles framework, so I've made the change you suggested to glx_config.c and the program proceeds, but for some reason this particular test is failing:

[sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64 bin/ext_polygon_offset_clamp-draw
libGL error: unable to load driver: nouveau_dri.so
libGL error: driver pointer missing
libGL error: failed to load driver: nouveau
Test requires GL_EXT_polygon_offset_clamp
PIGLIT: {"result": "skip" }

However, I believe GL_EXT_polygon_offset_clamp is in the library:

[sebald@ waffle]$ grep GL_EXT_polygon_offset_clamp ~/local/lib/dri/*
Binary file /home/sebald/local/lib/dri/kms_swrast_dri.so matches
Binary file /home/sebald/local/lib/dri/r200_dri.so matches
Binary file /home/sebald/local/lib/dri/radeon_dri.so matches
Binary file /home/sebald/local/lib/dri/swrast_dri.so matches

Eh, at this point it's too much hacking to try and resolve anything.  I'll just put together some kind of test that is in the Waffle framework as best as possible and still compiles on my system, then let you or someone else refine the build aspects of it.
Comment 23 Ilia Mirkin 2015-03-20 04:34:56 UTC
(In reply to Dan Sebald from comment #22)
> glxinfo indicates GLX Version 1.4.

Not really relevant... I was looking for the version of X to determine the general age of the system, as well as what libglx shipped with it.

> 
> I did find some tests that work without the GLX_ARB_create_context error. 
> There seems to be no correlation with 1.0, 2.0, etc.  For example, this one
> works:
> 
> [sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64
> bin/gl-2.1-pbo 
> libGL error: unable to load driver: nouveau_dri.so
> libGL error: driver pointer missing
> libGL error: failed to load driver: nouveau
> PIGLIT: {"subtest": {"test_sanity" : "pass"}}
> PIGLIT: {"subtest": {"test_draw_pixels" : "pass"}}
> PIGLIT: {"subtest": {"test_pixel_map" : "pass"}}
> PIGLIT: {"subtest": {"test_bitmap" : "pass"}}
> PIGLIT: {"subtest": {"test_tex_image" : "pass"}}
> PIGLIT: {"subtest": {"test_tex_sub_image" : "pass"}}
> PIGLIT: {"subtest": {"test_polygon_stip" : "pass"}}
> PIGLIT: {"subtest": {"test_error_handling" : "pass"}}
> 
> while most others don't.  The only thing that stands out is that in the C
> files for this test is a main() definition and in that definition there
> appears to be GLUT creating the context. 

Probably, yeah. piglit is old. The nice thing about waffle is that it works on all platforms. Except, apparently, really old ones that don't have GLX_ARB_create_context.

> The example
> ext_polygon_offset_clamp-draw has no main so I assume that is worked out via
> #include "piglit-util-gl.h" and Waffles.
> 
> But you'd want me to write a glPixelZoom test with the Waffles framework, so
> I've made the change you suggested to glx_config.c and the program proceeds,
> but for some reason this particular test is failing:
> 
> [sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64
> bin/ext_polygon_offset_clamp-draw
> libGL error: unable to load driver: nouveau_dri.so
> libGL error: driver pointer missing
> libGL error: failed to load driver: nouveau
> Test requires GL_EXT_polygon_offset_clamp
> PIGLIT: {"result": "skip" }
> 
> However, I believe GL_EXT_polygon_offset_clamp is in the library:
> 
> [sebald@ waffle]$ grep GL_EXT_polygon_offset_clamp ~/local/lib/dri/*
> Binary file /home/sebald/local/lib/dri/kms_swrast_dri.so matches
> Binary file /home/sebald/local/lib/dri/r200_dri.so matches
> Binary file /home/sebald/local/lib/dri/radeon_dri.so matches
> Binary file /home/sebald/local/lib/dri/swrast_dri.so matches
> 
> Eh, at this point it's too much hacking to try and resolve anything.  I'll
> just put together some kind of test that is in the Waffle framework as best
> as possible and still compiles on my system, then let you or someone else
> refine the build aspects of it.

As I had mentioned earlier, swrast doesn't support the extension. You'll note that glxinfo won't list it under OpenGL extensions. llvmpipe should support it though. (As do all DX10-level GPUs supported by mesa.)
Comment 24 Dan Sebald 2015-03-20 04:42:59 UTC
Yes, Gallium version works:

[sebald@ piglit]$ LD_LIBRARY_PATH=/home/sebald/local/lib:/usr/local/lib64 bin/ext_polygon_offset_clamp-draw
libGL error: unable to load driver: nouveau_dri.so
libGL error: driver pointer missing
libGL error: failed to load driver: nouveau
PIGLIT: {"subtest": {"negative clamp" : "pass"}}
PIGLIT: {"subtest": {"positive clamp" : "pass"}}

I see a green rectangle in the display.  Thanks.
Comment 25 Dan Sebald 2015-03-24 19:07:29 UTC
I will attach an initial attempt at a Piglit test suite for glPixelZoom() in combination with glDrawPixels().  I tried (but failed) to set something up in which if a test fails the associated image is displayed for a couple seconds, but there is no good way in the framework of creating a delay.  As with modern design for program flow, there shouldn't be any type of linear pause.  Given that, it would take a bit to make registered callback functions work to recursively called tests because of the way the piglit_display is defined.  Changing that is too much work/modification for this new test.  Not even sure that is worth doing because big delays in rendering are usually a nuisance.  The thing to do would be making each test a separate rendering, but that could be a lot of C files.  In any case, if there is a failure, one has to manually pre-compile out most of the code, i.e., #if 0 .. #endif to make the failed test the last image displayed.  For now, I've done that and assembled the images in PNG files for convenience.

Originally I had put the test in tests/general/zoom-pixels.c to complement tests/general/draw-pixels.c, then moved it to where I.M. suggested, tests/spec/gl-1.0/pixelzoom.c.

I've created several tests, and I attempted to make them specific yet not too specific (such as matching exact expected values) as to not cause any false negatives due to numerical effects.  The tests are:

1) Monotonicity:  With an input image in which one dimension is 2.5 times GL_MAX_TEXTURE_SIZE there is a ramp function for the RGB components of RGBA.  Hence the output image should be a ramp.  If it isn't, then that is a failure.  This will catch the type of problem I originally ran across in which vertical lines are appearing in the image due to dropped pixels.  This test is done for both x and y dimensions and for both positive and negative scaling factors.

2) Exact value at the edge:  Using the OpenGL formulas, I derived the needed scaling factors to make the last pixel of the input image correspond to the last pixel of the output image.  With that same ramp image from the previous test, the last pixel of the ramp is 1.0, so the last pixel of the output should be 1.0.  Formulas are slightly different for positive and negative scaling.  We can discuss whether this test is accurate.  (More later.)

3) Scaling over or underrun:  One thing I want to test is what I'm calling "overrun" or "underrun", which means that the output image falls one pixel too long or one pixel too short of its expected size.  This effect can often be seen when resizing the output image (e.g., using the mouse to resize a window via a corner).  That is, for some non-whole number numerator/divisor scalings, it might be that the pixel zoom indeces aren't working out right because of some inexact formula (e.g., the problems I saw in the original problem for swrast-legacy).  What this test does is draw successive rectangles using an input image all of the same intensity but decrementing the zoom width or height and alternating the input image intensity.  The residue left for each iteration is a single line, so when the process nears the end, the result is a framebuffer image of alternating lines.  If that isn't correct, then for some zoom factor along the way there was a mistake.  This is done for both directions, both postive and negative scaling factors and both magnification and condensation...  I've had to put a small tolerance in the test for pixel values in the over/underrun tests.  Does it work that the driver sets the value as close to, say, 0.25 as possible, and then returns what the actual value is?  E.g., say the driver is 7 bits, so the resolution is 1/2^7.  (Incidently, every driver returns an exact value for 1.0?)

OK, please give this test suite a try and let me know what you think.  I'm sure adjustments need to be made, and we can discuss the validity of the tests with regard to the OpenGL standard formulas, and refine them if warranted.

Here is a summary of the results I'm currently seeing and a few comments.  I've made screen captures and assembled the results into collective bitmaps so there aren't so many individual files.  The four different drivers are as follows:

(1) swrast-legacy: The repository legacy swrast_dri.so driver for which my system and others is using and exibits vertical lines.

(2) swrast-legacy-patch: The legacy swrast_dri.co patched with the changeset I originally attached to this bug report.

(3) swrast-gallium: The repository Gallium/llvm swrast_dri.so driver.

(4) swrast-gallium-patch: The Gallium swrast_dri.co patched by removing the line of code that limits the size of the image.

TEST                       (1)   (2)   (3)   (4)
----
positive monotonic x       fail  pass  fail  fail
positive edge x            fail  pass  fail  fail
positive over/underrun x   fail  fail  pass  pass
negative monotonic x       pass  pass  fail  pass
negative edge x            fail  pass  fail  fail
negative over/underrun x   fail  fail  pass  pass
positive monotonic y       fail  pass  fail  fail
positive edge y            fail  pass  fail  fail
positive over/underrun y   fail  fail  pass  pass
negative monotonic y       pass  pass  fail  pass
negative edge y            pass  pass  fail  fail
negative over/underrun y   fail  fail  pass  pass

* Note that the Gallium driver behaves better than the legacy swrast for modifying the window size.  Try maximizing the Piglit display window, or dragging the corner.  Gallium driver scales quite nicely, but not legacy swrast.  Gallium driver is a little faster as well.

The image summary will appear labelled as follows (download the files and view as 1:1 or "normal size", otherwise there will be bad aliasing effects):

SWRAST_DRI.SO LEGACY

This is the current repository version of the legacy swrast driver.  The first image (upper left corner) shows the original problem I and others had experienced: vertical lines due to dropped pixels associated with GL_MAX_TEXTURE_SIZE transition.  Interestingly, this doesn't happen when xfactor is negative (one of the "pass" tests).  The Y-direction exhibits a good gradient ramp, except for the very last pixel in the positive direction, which is missing (look for the dark line near the top of the window), so this test fails for reasons different than in the X direction.

The positive over/underrun with magnification X test actually looks good, and I don't know why the test is failing.  These things can be looked at in greater detail later.  The other three over/underrun tests in the X direction clearly show that glDrawPixels isn't zooming to the right scale.  This is the other potential bug I've wondered about.

SWRAST_DRI.SO LEGACY PATCH

With the Mesa changeset I attached to this bug report, the legacy driver now produces good results as far as the monotonicity tests.  The vertical lines due to the GL_MAX_TEXTURE_SIZE boundary are gone in the X direction, and the positive Y-gradient image no longer has a missing last pixel (line).  So that is making progress.  However, it's clear that the changeset has solved the issue with glDrawPixels not scaling precisely, and I still don't see why the positive X/Y over/underrun tests are failing--the associated images look correct.

SWRAST_DRI.SO GALLIUM

The test images illustrate how the Gallium/LLVM driver is not displaying anything beyond the GL_MAX_TEXTURE_SIZE boundary of the input image.  For all four monotonicity tests, only the darker portion of the gradient appears and from there it is the last draw/render that remains in the frame buffer.  E.g., the Positive Monotonicity Y image still has the vertical alternating lines from the Negative Over/Underrun X test.  It's no wonder all the monotonicity and edge tests fail.  But the good news is that for input images with sizes less than GL_MAX_TEXTURE_SIZE, the Gallium driver is accurate for scaling.  That's very nice.

SWRAST_DRI.SO GALLIUM PATCH

By making that simple change in src/mesa/state_tracker/st_cb_drawpixels.c that allows the driver to proceed if the input image size is greater than GL_MAX_TEXTURE_SIZE, I've found that the Gallium driver will complete the monotonic image grandients and generally looks good except for the last pixel (line) missing for both positive X and Y tests.  Again, this is just an initial investigation, so we aren't sure yet whether there is a shortcoming in the test algorithm, or whether the Gallium driver might not be meeting the OpenGL standard for glDrawPixels() exactly, e.g., a boundary condition isn't being satisfied such as pixel centers on the left or bottom box boundary are considered in the box.
Comment 26 Dan Sebald 2015-03-24 19:24:57 UTC
Created attachment 114591 [details] [review]
Piglit pixelzoom test suite

See https://bugs.freedesktop.org/show_bug.cgi?id=89586#c25 for a description of this changeset.
Comment 27 Dan Sebald 2015-03-24 19:34:47 UTC
Created attachment 114592 [details]
swrast legacy Piglit test images

SWRAST_DRI.SO LEGACY

This is the current repository version of the legacy swrast driver.  The first image (upper left corner) shows the original problem I and others had experienced: vertical lines due to dropped pixels associated with GL_MAX_TEXTURE_SIZE transition.  Interestingly, this doesn't happen when xfactor is negative (one of the "pass" tests).  The Y-direction exhibits a good gradient ramp, except for the very last pixel in the positive direction, which is missing (look for the dark line near the top of the window), so this test fails for reasons different than in the X direction.

The positive over/underrun with magnification X test actually looks good, and I don't know why the test is failing.  These things can be looked at in greater detail later.  The other three over/underrun tests in the X direction clearly show that glDrawPixels isn't zooming to the right scale.  This is the other potential bug I've wondered about.
Comment 28 Dan Sebald 2015-03-24 19:36:26 UTC
Created attachment 114593 [details]
swrast legacy with patch Piglit test images

SWRAST_DRI.SO LEGACY PATCH

With the Mesa changeset I attached to this bug report, the legacy driver now produces good results as far as the monotonicity tests.  The vertical lines due to the GL_MAX_TEXTURE_SIZE boundary are gone in the X direction, and the positive Y-gradient image no longer has a missing last pixel (line).  So that is making progress.  However, it's clear that the changeset has solved the issue with glDrawPixels not scaling precisely, and I still don't see why the positive X/Y over/underrun tests are failing--the associated images look correct.
Comment 29 Dan Sebald 2015-03-24 19:37:39 UTC
Created attachment 114594 [details]
swrast Gallium Piglit test images

SWRAST_DRI.SO GALLIUM

The test images illustrate how the Gallium/LLVM driver is not displaying anything beyond the GL_MAX_TEXTURE_SIZE boundary of the input image.  For all four monotonicity tests, only the darker portion of the gradient appears and from there it is the last draw/render that remains in the frame buffer.  E.g., the Positive Monotonicity Y image still has the vertical alternating lines from the Negative Over/Underrun X test.  It's no wonder all the monotonicity and edge tests fail.  But the good news is that for input images with sizes less than GL_MAX_TEXTURE_SIZE, the Gallium driver is accurate for scaling.  That's very nice.
Comment 30 Dan Sebald 2015-03-24 19:38:36 UTC
Created attachment 114595 [details]
swrast Gallium with patch Piglit test images

SWRAST_DRI.SO GALLIUM PATCH

By making that simple change in src/mesa/state_tracker/st_cb_drawpixels.c that allows the driver to proceed if the input image size is greater than GL_MAX_TEXTURE_SIZE, I've found that the Gallium driver will complete the monotonic image grandients and generally looks good except for the last pixel (line) missing for both positive X and Y tests.  Again, this is just an initial investigation, so we aren't sure yet whether there is a shortcoming in the test algorithm, or whether the Gallium driver might not be meeting the OpenGL standard for glDrawPixels() exactly, e.g., a boundary condition isn't being satisfied such as pixel centers on the left or bottom box boundary are considered in the box.
Comment 31 Dan Sebald 2015-03-24 20:13:18 UTC
Created attachment 114596 [details]
swrast legacy Piglit test images

SWRAST_DRI.SO LEGACY

This is the current repository version of the legacy swrast driver.  The first image (upper left corner) shows the original problem I and others had experienced: vertical lines due to dropped pixels associated with GL_MAX_TEXTURE_SIZE transition.  Interestingly, this doesn't happen when xfactor is negative (one of the "pass" tests).  The Y-direction exhibits a good gradient ramp, except for the very last pixel in the positive direction, which is missing (look for the dark line near the top of the window), so this test fails for reasons different than in the X direction.

The positive over/underrun with magnification X test actually looks good, and I don't know why the test is failing.  These things can be looked at in greater detail later.  The other three over/underrun tests in the X direction clearly show that glDrawPixels isn't zooming to the right scale.  This is the other potential bug I've wondered about.
Comment 32 Dan Sebald 2015-03-24 20:14:06 UTC
Created attachment 114597 [details]
swrast legacy with patch Piglit test images

SWRAST_DRI.SO LEGACY PATCH

With the Mesa changeset I attached to this bug report, the legacy driver now produces good results as far as the monotonicity tests.  The vertical lines due to the GL_MAX_TEXTURE_SIZE boundary are gone in the X direction, and the positive Y-gradient image no longer has a missing last pixel (line).  So that is making progress.  However, it's clear that the changeset has solved the issue with glDrawPixels not scaling precisely, and I still don't see why the positive X/Y over/underrun tests are failing--the associated images look correct.
Comment 33 Dan Sebald 2015-03-24 20:15:40 UTC
Created attachment 114598 [details]
swrast Gallium Piglit test images

SWRAST_DRI.SO GALLIUM

The test images illustrate how the Gallium/LLVM driver is not displaying anything beyond the GL_MAX_TEXTURE_SIZE boundary of the input image.  For all four monotonicity tests, only the darker portion of the gradient appears and from there it is the last draw/render that remains in the frame buffer.  E.g., the Positive Monotonicity Y image still has the vertical alternating lines from the Negative Over/Underrun X test.  It's no wonder all the monotonicity and edge tests fail.  But the good news is that for input images with sizes less than GL_MAX_TEXTURE_SIZE, the Gallium driver is accurate for scaling.  That's very nice.
Comment 34 Dan Sebald 2015-03-24 20:17:28 UTC
Created attachment 114599 [details]
swrast Gallium with patch Piglit test images

SWRAST_DRI.SO GALLIUM PATCH

By making that simple change in src/mesa/state_tracker/st_cb_drawpixels.c that allows the driver to proceed if the input image size is greater than GL_MAX_TEXTURE_SIZE, I've found that the Gallium driver will complete the monotonic image grandients and generally looks good except for the last pixel (line) missing for both positive X and Y tests.  Again, this is just an initial investigation, so we aren't sure yet whether there is a shortcoming in the test algorithm, or whether the Gallium driver might not be meeting the OpenGL standard for glDrawPixels exactly, e.g., a boundary condition isn't being satisfy such as pixel centers on the left or bottom box boundary are considered in the box.
Comment 35 Dan Sebald 2015-03-25 17:00:06 UTC
Created attachment 114619 [details] [review]
Piglit pixelzoom test suite

Attached is an update to the piglit glPixelZoom()/glDrawPixels() tests.  I've added a red background before the tests are run so that it's obvious when the driver hasn't written to the frame buffer.  I also fixed a bug in the over/underrun tests.  I won't update the summary of images right now.
Comment 36 Dan Sebald 2015-03-25 22:37:10 UTC
What is the general thought regarding floating point numerical issues and the OpenGL formulas?  The reason the swrast-legacy driver is failing one the Piglit pixelzoom tests is that this computation:

     c1 = imageX + (GLint) ceil((spanX + spanWidth - imageX) * ctx->Pixel.ZoomX);

has numerical issues when ctx->Pixel.ZoomX is fractional and negative.  (Sign isn't at the heart of the issue, the fact it is fractional is, i.e., -1 < ctx->Pixel.ZoomX < 0.)  A few example numbers work out as follows (this is part of the succession of smaller filled rectangles test):

span.x=160 span.end=400
(c0=-0.000000 c1=-119.000008)=>(-0.000000,-119.000000)

span.x=160 span.end=400
(c0=-0.000000 c1=-117.999992)=>(-0.000000,-117.000000)

span.x=160 span.end=400
(c0=-0.000000 c1=-116.999992)=>(-0.000000,-116.000000)

span.x=160 span.end=400
(c0=-0.000000 c1=-116.000000)=>(-0.000000,-116.000000)

Note how there are two end columns that are shorter than they should be, and the scaling here isn't very drastic, 1/2.5.

Is there anything about the OpenGL standard that allows for, say, a band of numerical tolerance for ceil and floor operations?  Or is it supposed to be that formulas consider GLfloat xfactor and yfactor as though they are real numbers, i.e., need exactness?

I can make all the swrast tests pass if I put some tolerance in for the ceiling function.
Comment 37 Ilia Mirkin 2015-03-25 22:42:24 UTC
(In reply to Dan Sebald from comment #36)
> What is the general thought regarding floating point numerical issues and
> the OpenGL formulas?

In early GLSL versions, floating point accuracy was supposed to be to within 0.1 (i.e. pretty huge errors were allowable). However if the hw were to start introducing such errors left and right, it'd wreak huge havoc... with ARB_shader_precision (part of GL 4.1), it specifies these things fairly precisely.

Not sure how that carries over to the glPixelZoom-style formulas.
Comment 38 Dan Sebald 2015-03-25 22:58:40 UTC
0.1 tolerance is rather large (I'm guessing that applies to normalized numbers).  In this case the discrepancy doesn't come close to that big.

I looked up ARB_shader_precision and I'm seeing tolerance on the order of 2 to 3 units of the last place, generally, with a factor larger tolerance (16 ULP) for the pow() function.  Division is the source of the error in xfactor, which has 2.5 ULP tolerance.

I can experiment with some of the C "next nearest number" routines and get a feeling for the ULP.  Thanks.
Comment 39 Ilia Mirkin 2015-03-25 23:03:54 UTC
(In reply to Dan Sebald from comment #38)
> I can experiment with some of the C "next nearest number" routines and get a
> feeling for the ULP.  Thanks.

FWIW I think you're going way overboard with this. BTW, the proper way to submit tests (and any other patches), is by mailing them to the relevant list (piglit@lists.freedesktop.org in this case).
Comment 40 Dan Sebald 2015-03-30 04:33:08 UTC
Created attachment 114710 [details] [review]
Changeset to fix vertical lines and fine tune positive_unzoom_x() and negative_unzoom_x()

Attached is an update to the SWRAST legacy changeset.  With this change, the driver passes all tests in Piglit gl-1.0-pixelzoom check.

The main addition to the changeset over the last changeset is the inclusion of a tolerance for the ceil() and floor() functions.  The issue is that with single precision float division and multiplication the formula

(xz - xr) / xfactor

can be off by a fair amount, on the order of 10e-5.  I printed out some numbers the driver was using in cases there the gl-1.0-pixelzoom alternating-line test was failing.  The numbers agree exactly with this example result:

octave:3> single(53)/single(400) * single(400)
ans =  52.9999961853027

I put in a tolerance of 0.00004 on the rounding functions.  By my very rough estimate, I think it is possible to scale input images with dimension of about 100,000 down to typical screen sizes without the added tolerance causing its own sort of artifact.
Comment 41 Dan Sebald 2015-03-30 04:36:46 UTC
Created attachment 114711 [details] [review]
Piglit pixelzoom test suite

An update to test results after making some modifications to SWRAST legacy changeset and the Piglit gl-1.0-pixelzoom tests:

(1) swrast-legacy: The repository legacy swrast_dri.so driver for which my system and others is using and exibits vertical lines.

(2) swrast-legacy-patch: The legacy swrast_dri.co patched with the changeset I originally attached to this bug report.

(3) swrast-gallium: The repository Gallium/llvm swrast_dri.so driver.

(4) swrast-gallium-patch: The Gallium swrast_dri.co patched by removing the line of code that limits the size of the image.

TEST                       (1)   (2)   (3)   (4)
----
positive monotonic x       fail  pass  fail  fail
positive edge x            fail  pass  fail  fail
positive over/underrun x   fail  pass  pass  pass
negative monotonic x       pass  pass  fail  pass
negative edge x            fail  pass  fail  fail
negative over/underrun x   fail  pass  pass  pass
positive monotonic y       fail  pass  fail  fail
positive edge y            fail  pass  fail  fail
positive over/underrun y   fail  pass  pass  pass
negative monotonic y       pass  pass  fail  pass
negative edge y            pass  pass  fail  fail
negative over/underrun y   fail  pass  pass  pass
Comment 42 Dan Sebald 2015-03-30 04:38:59 UTC
Created attachment 114712 [details]
swrast legacy Piglit test images
Comment 43 Dan Sebald 2015-03-30 04:40:02 UTC
Created attachment 114713 [details]
swrast legacy with patch Piglit test images
Comment 44 Dan Sebald 2015-03-30 04:41:00 UTC
Created attachment 114714 [details]
swrast Gallium Piglit test images
Comment 45 Dan Sebald 2015-03-30 04:42:06 UTC
Created attachment 114715 [details]
swrast Gallium with patch Piglit test images
Comment 46 GitLab Migration User 2019-09-18 18:45:29 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/316.


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.