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

[Markus Wick]

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)

> +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?

> +static int
> +glamor_line_length(short x1, short y1, short x2, short y2)
> +{
> +    int dx = abs(x2 - x1);
> +    int dy = abs(y2 - y1);
> +    if (dx > dy)
> +        return dx;
> +    return dy;

max(dx,dy)

> +Bool
> +glamor_poly_lines_dash_gl(DrawablePtr drawable, GCPtr gc,
...
> +    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;

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

> +        }
> +    }
> +
> +    dash_pos = gc->dashOffset;
> +    prev_x = prev_y = 0;
> +    for (i = 0; i < n; i++) {
> +        if (i)
> +            dash_pos += glamor_line_length(prev_x, prev_y,
> +                                           points[i].x, points[i].y);

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

> +        v[0] = prev_x = points[i].x;
> +        v[1] = prev_y = points[i].y;
> +        v[2] = dash_pos;
> +        v += 4;
> +    }
> +
> +    if (add_last) {
> +        v[0] = v[-4] + 1;
> +        v[1] = v[-3];
> +        v[2] = dash_pos + 1;
> +    }

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.


> +static short *
> +glamor_add_segment(short *v, short x1, short y1, short x2, short y2,
> int dash_start)
> +{
> +    v[0] = x1;
> +    v[1] = y1;
> +    v[2] = dash_start;
> +
> +    v[4] = x2;
> +    v[5] = y2;
> +    v[6] = dash_start + glamor_line_length(x1, y1,
> +                                           x2, y2);
> +    return v + 8;
> +}

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 :/

> +Bool
> +glamor_poly_segment_dash_gl(DrawablePtr drawable, GCPtr gc,
...
> +    for (i = 0; i < nseg; i++) {
> +        v = glamor_add_segment(v,
> +                               segs[i].x1, segs[i].y1,
> +                               segs[i].x2, segs[i].y2,
> +                               dash_start);
> +        if (add_last)
> +            v = glamor_add_segment(v,
> +                                   segs[i].x2, segs[i].y2,
> +                                   segs[i].x2 + 1, segs[i].y2,
> +                                   v[-8 + 6]);

Also accessing v.

--------------------

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