[Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
Predut, Marius
marius.predut at intel.com
Tue Mar 17 12:52:40 PDT 2015
> -----Original Message-----
> From: Ian Romanick [mailto:idr at freedesktop.org]
> Sent: Tuesday, March 17, 2015 8:27 PM
> To: Predut, Marius; mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest
> width lines - GEN6
>
> On 03/17/2015 11:29 AM, Marius Predut wrote:
> > On SNB and IVB hw, for 1 pixel line thickness or less, the general anti-
> aliasing algorithm give up - garbage line is generated.
> > Setting a Line Width of 0.0 specifies the rasterization of the “thinnest”
> (one-pixel-wide), non-antialiased lines.
> > Lines rendered with zero Line Width are rasterized using Grid Intersection
> Quantization rules as specified by bspec section 6.3.12.1 Zero-Width
> (Cosmetic) Line Rasterization.
>
> You'll need to re-word wrap the commit message. You'll get the commit message
> right one of these days. :)
Yes right, also Brian Paul notice it.
>
> Also... when you send out new versions of a patch, please change the patch
> subject to be something like "[PATCH v3] ...". This makes it easier for
> people to know which is the latest version to review.
>
> You should also add notes to the commit message that explain what changed from
> version to version. For example, this commit message should have something
> like:
>
> v3: Fix = used instead of == in an if-statement. Noticed by Daniel Stone.
>
> This is helps people know that their review comments have been applied.
> It is also important to do this when the review changes are applied and the
> patch committed without re-sending to the list. Maintaining history like this
> in commit messages helps us understand code in the future.
>
Yes indeed.
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832
>
> There are a number of bugs that have been closed as duplicates of this bug.
> Two of these, bug #27007 and bug #60797, have test cases. Does this fix also
> fix those?
Yes , was fixed.
Ok I try put all defects here.
>
> I also have a more general question: How are you testing this? Daniel
> noticed a bug in an earlier version of the patch that piglit should have
> caught. If you're doing a full piglit run and that didn't catch the previous
> assignment-instead-of-comparison bug, it would be helpful if you could craft a
> test that would detect that bug. That may be difficult, so don't spend a huge
> amount of time on it.
>
I used the piglit test /bin/line-aa-width -auto
Also I don’t observer pilgit test regressions when I run /piglit-run.py tests/quick.py.
I used this https://bugs.freedesktop.org/attachment.cgi?id=33930 test case and checked visually.
(an interrupted line rendered)
I used this https://bugs.freedesktop.org/attachment.cgi?id=8675 test case and checked visually.
( a triangle for witch a line is missing)
I don't know more about assignment-instead-of-comparison and Daniel noticed.
> > Signed-off-by: Marius Predut <marius.predut at intel.com>
> > ---
> > src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > index f9d8d27..1bed444 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw)
> > float line_width =
> > roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth));
> > uint32_t line_width_u3_7 = U_FIXED(line_width, 7);
> > - /* TODO: line width of 0 is not allowed when MSAA enabled */
> > - if (line_width_u3_7 == 0)
> > - line_width_u3_7 = 1;
> > +
> > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) {
>
> In Mesa there are often Enabled and _Enabled fields. The Enabled field is the
> setting made by the application via the OpenGL API. The _Enabled field means
> that feature in question is actually enabled, and this may depend on other
> state.
>
> In this case, the application may enable multisampling, but multisampling may
> not occur of there is not a multisample buffer. This means
> gl_multisample_attrib::Enabled would be set but
> gl_multisample_attrib::_Enabled would not. Instead of open-coding that check,
> just check gl_multisample_attrib::_Enabled directly:
>
> if (!ctx->Multisample._Enabled) {
>
> I actually think it's more clear if you leave the original comment and
> implement this as:
>
> /* Line width of 0 is not allowed when MSAA enabled */
> if (ctx->Multisample._Enabled) {
> if (line_width_u3_7 == 0)
> line_width_u3_7 = 1;
> } else if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1.0) {
> /* For lines less than 1 pixel thick, the general
> * anti-aliasing algorithm gives up, and a garbage line is
> * generated. Setting a Line Width of 0.0 specifies the
> * rasterization of the "thinnest" (one-pixel-wide),
> * non-antialiased lines.
> *
> * Lines rendered with zero Line Width are rasterized using
> * Grid Intersection Quantization rules as specified by
> * bspec section 6.3.12.1 Zero-Width (Cosmetic) Line
> * Rasterization.
> */
> line_width_u3_7 = 0;
> }
>
> I reworded the comment a bit (from the commit message), and I changed the
> Line.Width comparison to compare with 1.0.
>
I can use your version.
> One final question. Does it produce better results to use 0 or 1.0? It
> sounds like using 1.0 would still enable line antialiasing, and the resulting
> line shouldn't be appreciably thicker.
>
> > + if (ctx->Line.SmoothFlag && ctx->Line.Width <=1)
> > + line_width_u3_7 = 0;
> > + } else {
> > + if (line_width_u3_7 == 0)
> > + line_width_u3_7 = 1;
> > + }
> > +
> > dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT;
> > }
> > if (ctx->Line.SmoothFlag) {
Using 1.0 or 1 line width ,the anti-aliasing give -up, or may be is not clear for me your question
Can be a test case for line width between 1 and 1.5(not 1 or 1.5) ? in this case the better value instead
1.0 can be 1.499 .
More information about the mesa-dev
mailing list