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

[Ian Romanick]

On 10/07/2013 04:31 PM, 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>
> ---
>  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);
>        }

This is at least the second time this code block has appeared.  inline
member function?

>        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));
>