[Mesa-dev,02/11] i965/blorp: round to nearest when converting float to integer

Submitted by Eduardo Lima Mitev on Feb. 10, 2015, 3:40 p.m.

Details

Message ID 1423582848-18526-3-git-send-email-elima@igalia.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Eduardo Lima Mitev Feb. 10, 2015, 3:40 p.m.
From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>

Round floating point values to nearest integer to avoid "off by one texel"
kind of errors when blitting.

Fixes:

dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_linear
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_y_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_y_linear
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_y_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_y_linear
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_x_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_x_linear
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_y_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_y_linear

No piglit regressions.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
index 10a53dc..469baf7 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
@@ -1994,10 +1994,13 @@  brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,
 
    wm_prog_key.src_tiled_w = src.map_stencil_as_y_tiled;
    wm_prog_key.dst_tiled_w = dst.map_stencil_as_y_tiled;
-   x0 = wm_push_consts.dst_x0 = dst_x0;
-   y0 = wm_push_consts.dst_y0 = dst_y0;
-   x1 = wm_push_consts.dst_x1 = dst_x1;
-   y1 = wm_push_consts.dst_y1 = dst_y1;
+   /* Round floating point values to nearest integer to avoid "off by one texel"
+    * kind of errors when blitting.
+    */
+   x0 = wm_push_consts.dst_x0 = dst_x0 + 0.5;
+   y0 = wm_push_consts.dst_y0 = dst_y0 + 0.5;
+   x1 = wm_push_consts.dst_x1 = dst_x1 + 0.5;
+   y1 = wm_push_consts.dst_y1 = dst_y1 + 0.5;
    wm_push_consts.rect_grid_x1 = (minify(src_mt->logical_width0, src_level) *
                                   wm_prog_key.x_scale - 1.0);
    wm_push_consts.rect_grid_y1 = (minify(src_mt->logical_height0, src_level) *

Comments

On Tue, Feb 10, 2015 at 7:40 AM, Eduardo Lima Mitev <elima@igalia.com> wrote:
> From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
>
> Round floating point values to nearest integer to avoid "off by one texel"
> kind of errors when blitting.
>
> Fixes:
>
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_nearest
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_linear
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_y_nearest
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_y_linear
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_y_nearest
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_y_linear
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_x_nearest
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_x_linear
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_y_nearest
> dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_dst_y_linear
>
> No piglit regressions.
>
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> ---
>  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> index 10a53dc..469baf7 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> @@ -1994,10 +1994,13 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,
>
>     wm_prog_key.src_tiled_w = src.map_stencil_as_y_tiled;
>     wm_prog_key.dst_tiled_w = dst.map_stencil_as_y_tiled;
> -   x0 = wm_push_consts.dst_x0 = dst_x0;
> -   y0 = wm_push_consts.dst_y0 = dst_y0;
> -   x1 = wm_push_consts.dst_x1 = dst_x1;
> -   y1 = wm_push_consts.dst_y1 = dst_y1;
> +   /* Round floating point values to nearest integer to avoid "off by one texel"
> +    * kind of errors when blitting.
> +    */
> +   x0 = wm_push_consts.dst_x0 = dst_x0 + 0.5;
> +   y0 = wm_push_consts.dst_y0 = dst_y0 + 0.5;
> +   x1 = wm_push_consts.dst_x1 = dst_x1 + 0.5;
> +   y1 = wm_push_consts.dst_y1 = dst_y1 + 0.5;

Can we use round(dst_??) here instead?

x + 0.5 has the surprising property that nextafter(0.5, 0.0) (i.e.,
the largest value less than 0.5) + 0.5 is exactly half way between the
largest value less than 1.0 and 1.0, so it gets rounded to 1.0 instead
of down to 0.0. It's an uncommon case, but round() should better
describe what we want to do anyway.
On Tue, Feb 10, 2015 at 10:52 AM, Matt Turner <mattst88@gmail.com> wrote:
>> +   /* Round floating point values to nearest integer to avoid "off by one texel"
>> +    * kind of errors when blitting.
>> +    */
>> +   x0 = wm_push_consts.dst_x0 = dst_x0 + 0.5;
>> +   y0 = wm_push_consts.dst_y0 = dst_y0 + 0.5;
>> +   x1 = wm_push_consts.dst_x1 = dst_x1 + 0.5;
>> +   y1 = wm_push_consts.dst_y1 = dst_y1 + 0.5;
>
> Can we use round(dst_??) here instead?
>
> x + 0.5 has the surprising property that nextafter(0.5, 0.0) (i.e.,
> the largest value less than 0.5) + 0.5 is exactly half way between the
> largest value less than 1.0 and 1.0, so it gets rounded to 1.0 instead
> of down to 0.0. It's an uncommon case, but round() should better
> describe what we want to do anyway.

And just to short circuit the process, assuming round() works have a

Reviewed-by: Matt Turner <mattst88@gmail.com>

and feel free to commit. Shouldn't be necessary to resend.
On Tuesday 10 February 2015 11:32:32 Matt Turner wrote:
> On Tue, Feb 10, 2015 at 10:52 AM, Matt Turner <mattst88@gmail.com> wrote:
> >> +   /* Round floating point values to nearest integer to avoid "off by
> >> one texel" +    * kind of errors when blitting.
> >> +    */
> >> +   x0 = wm_push_consts.dst_x0 = dst_x0 + 0.5;
> >> +   y0 = wm_push_consts.dst_y0 = dst_y0 + 0.5;
> >> +   x1 = wm_push_consts.dst_x1 = dst_x1 + 0.5;
> >> +   y1 = wm_push_consts.dst_y1 = dst_y1 + 0.5;
> > 
> > Can we use round(dst_??) here instead?
> > 
> > x + 0.5 has the surprising property that nextafter(0.5, 0.0) (i.e.,
> > the largest value less than 0.5) + 0.5 is exactly half way between the
> > largest value less than 1.0 and 1.0, so it gets rounded to 1.0 instead
> > of down to 0.0. It's an uncommon case, but round() should better
> > describe what we want to do anyway.
> 
> And just to short circuit the process, assuming round() works have a
> 
> Reviewed-by: Matt Turner <mattst88@gmail.com>
> 
> and feel free to commit. Shouldn't be necessary to resend.

Thanks for your review, Matt. I'll do the change and check that it works as 
expected.

Sam
On Tue, Feb 10, 2015 at 11:00 PM, Samuel Iglesias Gonsálvez
<siglesias@igalia.com> wrote:
> On Tuesday 10 February 2015 11:32:32 Matt Turner wrote:
>> On Tue, Feb 10, 2015 at 10:52 AM, Matt Turner <mattst88@gmail.com> wrote:
>> >> +   /* Round floating point values to nearest integer to avoid "off by
>> >> one texel" +    * kind of errors when blitting.
>> >> +    */
>> >> +   x0 = wm_push_consts.dst_x0 = dst_x0 + 0.5;
>> >> +   y0 = wm_push_consts.dst_y0 = dst_y0 + 0.5;
>> >> +   x1 = wm_push_consts.dst_x1 = dst_x1 + 0.5;
>> >> +   y1 = wm_push_consts.dst_y1 = dst_y1 + 0.5;
>> >
>> > Can we use round(dst_??) here instead?
>> >
>> > x + 0.5 has the surprising property that nextafter(0.5, 0.0) (i.e.,
>> > the largest value less than 0.5) + 0.5 is exactly half way between the
>> > largest value less than 1.0 and 1.0, so it gets rounded to 1.0 instead
>> > of down to 0.0. It's an uncommon case, but round() should better
>> > describe what we want to do anyway.
>>
>> And just to short circuit the process, assuming round() works have a
>>
>> Reviewed-by: Matt Turner <mattst88@gmail.com>
>>
>> and feel free to commit. Shouldn't be necessary to resend.
>
> Thanks for your review, Matt. I'll do the change and check that it works as
> expected.

Sorry for the late comment -- I should have said round*f* :)