[PATCH 05/13] glamor: Add glamor_program based 0-width dashed lines

[Keith Packard]

Markus Wick <markus at selfnet.de> writes:

> Am 2014-05-06 00:02, schrieb Keith Packard:
>> +static const char on_off_fs_exec[] =
>> +    "       float pattern = texture2D(dash, vec2(dash_offset, 
>> 0.5)).w;\n"
>> +    "       if (pattern == 0.0)\n"
>> +    "               discard;\n";
>
> Did you care about the sampler configuration? eg with linear filtering, 
> you don't have to get exactly 0.0. Maybe it's better to check in this 
> way:
> if (pattern < 0.5)

If the sampler is filtering, then we're going to get the wrong answer. I
do note that I'm not explicitly setting the filtering for the dash
texture; do I need to?


>
>> +static glamor_program *
>> +glamor_dash_setup(DrawablePtr drawable, GCPtr gc)
> ...
>> +bail_ctx:
>> +    glDisable(GL_COLOR_LOGIC_OP);
>
> Isn't this already disabled in the caller?

Doesn't seem to be. I think the plan post-1.16 is to standardize on the
set of parameters that need to be set by each entry point and then
eliminate all of the resetting paths. If we assume that GL optimizes
setting a parameter to the current value, then this should be more
efficient than our current scheme.

> max(dx,dy)

How about:

static int
glamor_line_length(short x1, short y1, short x2, short y2)
{
    return max(abs(x2 - x1), abs(y2 - y1));
}

> Are you allowed to overwrite "points"? I'd inline this code in the next 
> loop to not iterate twice.

Yes, you're allowed to overwrite the coordinates, but it's not ideal. In
practice, applications "never" use CoordModePrevious, but as you say,
it's probably neater to just inline like this:

@@ -277,25 +273,21 @@ glamor_poly_lines_dash_gl(DrawablePtr drawable, GCPtr gc,
     glVertexAttribPointer(GLAMOR_VERTEX_POS, 3, GL_SHORT, GL_FALSE,
                           3 * sizeof (short), vbo_offset);
 
-    if (mode == CoordModePrevious) {
-        DDXPointRec here = { 0, 0 };
-
-        for (i = 0; i < n; i++) {
-            here.x += points[i].x;
-            points[i].x = here.x;
-            here.y += points[i].y;
-            points[i].y = here.y;
-        }
-    }
-
     dash_pos = gc->dashOffset;
     prev_x = prev_y = 0;
     for (i = 0; i < n; i++) {
-        if (i)
+        int this_x = points[i].x;
+        int this_y = points[i].y;
+        if (i) {
+            if (mode == CoordModePrevious) {
+                this_x += prev_x;
+                this_y += prev_y;
+            }
             dash_pos += glamor_line_length(prev_x, prev_y,
-                                           points[i].x, points[i].y);
-        v[0] = prev_x = points[i].x;
-        v[1] = prev_y = points[i].y;
+                                           this_x, this_y);
+        }
+        v[0] = prev_x = this_x;
+        v[1] = prev_y = this_y;
         v[2] = dash_pos;
         v += 3;
     }

> Isn't it easier to init prev_xy with points[0]?

Then I'd have to think harder about whether n was always > 0.

> You aren't allowed to access v.
> Thx for careing about offsets to be multiple of 4 bytes, but you have to 
> overwrite the buffer completely (v[3]=0). Else write-combining won't 
> like this code.

Eric pointed that out, and also suggested that there wasn't any reason
to force 4-byte alignment. If you can show a machine which makes 4-byte
aligned accesses faster, then we can switch, but reducing memory usage
seems like it will win most of the time.

> I don't think it's worth to create such a kind of util functions. It 
> doesn't remove code as you still need 4 lines in the caller. But it 
> hides the vertex format which is set up directly above the caller :/

It doesn't save any LOC, but I think it documents what we're doing to
get X semantics out of GL.

> Also accessing v.

Eric pointed this out and it's been fixed already.

> Of course, the same rasterization issues as in the last patch.

I don't think there are rasterization issues; the GL semantics do what
we want given the 0.5, 0.5 offset for each coordinate.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140513/84118395/attachment.sig>