[Mesa-dev] [PATCH 6/7] i965/blorp: Rework sRGB override behavior.

[Daniel Vetter]

On Mon, Oct 07, 2013 at 04:31:22PM -0700, Kenneth Graunke wrote:
> The previous code for sRGB overrides assumes that the source and
> destination formats are equal, other than the color space.  This won't
> be feasible when we add support for format conversions.
> 
> Here are a few cases, and how the old code handled them:
> 
> 1.  RGB8 -> SRGB8, MSAA     ==>   SRGB8 -> SRGB8
> 2.  RGB8 -> SRGB8, single   ==>    RGB8 -> RGB8
> 3. SRGB8 ->  RGB8, MSAA     ==>    RGB8 -> RGB8
> 4. SRGB8 ->  RGB8, single   ==>   SRGB8 -> SRGB8
> 
> Apparently, preserving the behavior of #1 is important.  When doing a
> multisample to single-sample resolve, blending the samples together in
> an sRGB correct fashion results in a noticably higher quality image.
> It also is necessary to pass Piglit's EXT_framebuffer_multisample
> accuracy color tests.
> 
> Paul, Eric, Anuj, and I talked about this, and aren't sure that it
> matters in the other cases.
> 
> This patch preserves the behavior of #1, but otherwise reverts to
> doing everything in linear space, changing the behavior of case #4.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>

Now I pretty seriously lack clue here, but I guess the difference between
the new logic and the old one (for blts that don't convert the format,
ignoring srgb/linear) when we have to deal with arb_multisample_texture?
If so maybe a note/comment or so might be good.

Otherwise I've had plenty fun reading around in blorp and the format
tables, so fwiw (and hey, it's very little!)

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

It's a bit hairy how the outermost blt code needs to check the depth
stuff, I'd have prefered to keep that logic in one place. But bubbling
that error up the layers looks like a pain, blorp has other similar
conditions already and the code seems to be guarded sufficiently with
asserts.

Cheers, Daniel
> ---
>  src/mesa/drivers/dri/i965/brw_blorp.cpp      | 10 ++++++----
>  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 14 +++++++++-----
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> index c59bb66..91df346 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
> @@ -104,15 +104,17 @@ brw_blorp_surface_info::set(struct brw_context *brw,
>     case MESA_FORMAT_Z16:
>        this->brw_surfaceformat = BRW_SURFACEFORMAT_R16_UNORM;
>        break;
> -   default:
> +   default: {
> +      gl_format linear_format = _mesa_get_srgb_format_linear(mt->format);
>        if (is_render_target) {
> -         assert(brw->format_supported_as_render_target[mt->format]);
> -         this->brw_surfaceformat = brw->render_target_format[mt->format];
> +         assert(brw->format_supported_as_render_target[linear_format]);
> +         this->brw_surfaceformat = brw->render_target_format[linear_format];
>        } else {
> -         this->brw_surfaceformat = brw_format_for_mesa_format(mt->format);
> +         this->brw_surfaceformat = brw_format_for_mesa_format(linear_format);
>        }
>        break;
>     }
> +   }
>  }
>  
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> index f581871..2427085 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp
> @@ -2077,13 +2077,17 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,
>      * proprietary OpenGL driver also follow this approach. So, we choose to
>      * follow it in our driver.
>      *
> -    * Following if-else block takes care of this exception made for
> -    * multisampled resolves.
> +    * When multisampling, if the source and destination formats are equal
> +    * (aside from the color space), we choose to blit in sRGB space to get
> +    * this higher quality image.
>      */
> -   if (src.num_samples > 1)
> +   if (src.num_samples > 1 &&
> +       _mesa_get_format_color_encoding(dst_mt->format) == GL_SRGB &&
> +       _mesa_get_srgb_format_linear(src_mt->format) ==
> +       _mesa_get_srgb_format_linear(dst_mt->format)) {
> +      dst.brw_surfaceformat = brw_format_for_mesa_format(dst_mt->format);
>        src.brw_surfaceformat = dst.brw_surfaceformat;
> -   else
> -      dst.brw_surfaceformat = src.brw_surfaceformat;
> +   }
>  
>     use_wm_prog = true;
>     memset(&wm_prog_key, 0, sizeof(wm_prog_key));
> -- 
> 1.8.3.2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch