[Mesa-dev] [PATCH] llvmpipe: Honour pipe_rasterizer::point_quad_rasterization.

[Marek Olšák]

On Wed, Jan 8, 2014 at 7:31 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 08.01.2014 19:12, schrieb Jose Fonseca:
>>
>>
>> ----- Original Message -----
>>> Am 08.01.2014 18:27, schrieb jfonseca at vmware.com:
>>>> From: José Fonseca <jfonseca at vmware.com>
>>>>
>>>> Commit eda21d2a3010d9fc5a68b55a843c5e44b2abf8dd fixed the rasterization
>>>> of points for Direct3D but ended up breaking the rasterization of OpenGL
>>>> non-sprite points, in particular conform's pntrast.c test.
>>>>
>>>> The only way to get both working is to properly honour
>>>> pipe_rasterizer::point_quad_rasterization, and follow the weird OpenGL
>>>> rule when it is false.
>>>> ---
>>>>  src/gallium/drivers/llvmpipe/lp_setup_point.c | 64
>>>>  ++++++++++++++++++++++-----
>>>>  1 file changed, 54 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_point.c
>>>> b/src/gallium/drivers/llvmpipe/lp_setup_point.c
>>>> index 988e0c5..e5ce4ee 100644
>>>> --- a/src/gallium/drivers/llvmpipe/lp_setup_point.c
>>>> +++ b/src/gallium/drivers/llvmpipe/lp_setup_point.c
>>>> @@ -338,9 +338,13 @@ try_setup_point( struct lp_setup_context *setup,
>>>>     int fixed_width = MAX2(FIXED_ONE,
>>>>                            (subpixel_snap(size) + FIXED_ONE/2 - 1) &
>>>>                            ~(FIXED_ONE-1));
>>>>
>>>> -   const int x0 = subpixel_snap(v0[0][0] - setup->pixel_offset) -
>>>> fixed_width/2;
>>>> -   const int y0 = subpixel_snap(v0[0][1] - setup->pixel_offset) -
>>>> fixed_width/2;
>>>> -
>>>> +   /* Yes this is necessary to accurately calculate bounding boxes
>>>> +    * with the two fill-conventions we support.  GL (normally) ends
>>>> +    * up needing a bottom-left fill convention, which requires
>>>> +    * slightly different rounding.
>>>> +    */
>>>> +   int adj = (setup->bottom_edge_rule != 0) ? 1 : 0;
>>>> +
>>>>     struct lp_scene *scene = setup->scene;
>>>>     struct lp_rast_triangle *point;
>>>>     unsigned bytes;
>>>> @@ -363,13 +367,14 @@ try_setup_point( struct lp_setup_context *setup,
>>>>        print_point(setup, v0);
>>>>
>>>>     /* Bounding rectangle (in pixels) */
>>>> -   {
>>>> -      /* Yes this is necessary to accurately calculate bounding boxes
>>>> -       * with the two fill-conventions we support.  GL (normally) ends
>>>> -       * up needing a bottom-left fill convention, which requires
>>>> -       * slightly different rounding.
>>>> +   if (!lp_context->rasterizer ||
>>>> +       lp_context->rasterizer->point_quad_rasterization) {
>>>> +      /*
>>>> +       * Rasterize points as quads.
>>>>         */
>>>> -      int adj = (setup->bottom_edge_rule != 0) ? 1 : 0;
>>>> +
>>>> +      const int x0 = subpixel_snap(v0[0][0] - setup->pixel_offset) -
>>>> fixed_width/2;
>>>> +      const int y0 = subpixel_snap(v0[0][1] - setup->pixel_offset) -
>>>> fixed_width/2;
>>>>
>>>>        bbox.x0 = (x0 + (FIXED_ONE-1)) >> FIXED_ORDER;
>>>>        bbox.x1 = (x0 + fixed_width + (FIXED_ONE-1)) >> FIXED_ORDER;
>>>> @@ -380,8 +385,47 @@ try_setup_point( struct lp_setup_context *setup,
>>>>         */
>>>>        bbox.x1--;
>>>>        bbox.y1--;
>>>> +   } else {
>>>> +      /*
>>>> +       * OpenGL rasterization rules for non-sprite points.
>>> Maybe add "legacy" here before OpenGL?
>>
>> Sure.
>>
>>>> +       *
>>>> +       * Per OpenGL 2.1 spec, section 3.3.1, "Basic Point Rasterization".
>>>> +       */
>>>> +
>>>> +      const int x0 = subpixel_snap(v0[0][0]);
>>>> +      const int y0 = subpixel_snap(v0[0][1]) - adj;
>>>> +
>>>> +      int int_width = fixed_width >> FIXED_ORDER;
>>>> +
>>>> +      assert(setup->pixel_offset != 0);
>>>> +
>>>> +      if (int_width == 1) {
>>>> +         bbox.x0 = x0 >> FIXED_ORDER;
>>>> +         bbox.y0 = y0 >> FIXED_ORDER;
>>>> +         bbox.x1 = bbox.x0;
>>>> +         bbox.y1 = bbox.y0;
>>>> +      } else {
>>>> +         if (int_width & 1) {
>>>> +            /* Odd width */
>>>> +            bbox.x0 = (x0 >> FIXED_ORDER) - (int_width - 1)/2;
>>>> +            bbox.y0 = (y0 >> FIXED_ORDER) - (int_width - 1)/2;
>>>> +         } else {
>>>> +            /* Even width */
>>>> +            bbox.x0 = ((x0 + FIXED_ONE/2) >> FIXED_ORDER) - int_width/2;
>>>> +            bbox.y0 = ((y0 + FIXED_ONE/2) >> FIXED_ORDER) - int_width/2;
>>>> +         }
>>>> +
>>>> +         bbox.x1 = bbox.x0 + int_width - 1;
>>>> +         bbox.y1 = bbox.y0 + int_width - 1;
>>>> +      }
>>>>     }
>>>> -
>>>> +
>>>> +   if (0) {
>>>> +      debug_printf("  bbox: (%i, %i) - (%i, %i)\n",
>>>> +                   bbox.x0, bbox.y0,
>>>> +                   bbox.x1, bbox.y1);
>>>> +   }
>>>> +
>>>>     if (!u_rect_test_intersection(&setup->draw_regions[viewport_index],
>>>>     &bbox)) {
>>>>        if (0) debug_printf("offscreen\n");
>>>>        LP_COUNT(nr_culled_tris);
>>>>
>>>
>>> This looks good to me. Note though this type of point rasterization is
>>> only available in pre 3.0 contexts (or compatibilility contexts which we
>>> don't support) anyway.
>>
>> Oh really!?  Argh..  It though it was still relevant nowadays, or I wouldn't have wasted my afternoon on trying to figure out the regression and the proper fix...
> Well it seems at least some hw (nvidia for one) still can follow these
> rules, so maybe there are apps out there which rely on it (radeonsi otoh
> ignores the state). At least conform relies on it apparently so it's not
> really a waste :-).

I have never understood what point_quad_rasterization means and how we
should use it. I think Radeons always rasterize points as axis-aligned
rectangles (width and height can be different) and there is no support
for round/smooth points as far as I know.

Marek