Re: [Mesa-dev] [PATCH] st/mesa: Add R8G8B8A8_SRGB case to st_pipe_format_to_mesa_format.

[Brian Paul]

On 03/07/2014 09:45 AM, Roland Scheidegger wrote:

Am 07.03.2014 05:47, schrieb Chia-I Wu:

On Fri, Mar 7, 2014 at 11:56 AM, Chia-I Wu <olva...@gmail.com> wrote:

On Fri, Mar 7, 2014 at 2:04 AM, Jose Fonseca <jfons...@vmware.com> wrote:

----- Original Message -----

Am 06.03.2014 18:32, schrieb Jose Fonseca:

----- Original Message -----

----- Original Message -----

On 03/06/2014 09:59 AM, jfons...@vmware.com wrote:

From: José Fonseca <jfons...@vmware.com>

With the recent SRGB changes all my automated OpenGL llvmpipe tests
(piglit, conform, glretrace) start asserting with the backtrace below.

I'm hoping this change will fix it.  I'm not entirely sure, as this
doesn't happen in my development machine (the bug probably depends on
the exact X visual).

Anyway, it seems the sensible thing to do here.

     Program terminated with signal 5, Trace/breakpoint trap.
     #0  _debug_assert_fail (expr=expr@entry=0x7fa324df2ed7 "0",
     file=file@entry=0x7fa324e3fc30 "src/mesa/state_tracker/st_format.c",
     line=line@entry=758, function=function@entry=0x7fa324e40160
     <__func__.34798> "st_pipe_format_to_mesa_format") at
     src/gallium/auxiliary/util/u_debug.c:281
     #0  _debug_assert_fail (expr=expr@entry=0x7fa324df2ed7 "0",
     file=file@entry=0x7fa324e3fc30 "src/mesa/state_tracker/st_format.c",
     line=line@entry=758, function=function@entry=0x7fa324e40160
     <__func__.34798> "st_pipe_format_to_mesa_format") at
     src/gallium/auxiliary/util/u_debug.c:281
     No locals.
     #1  0x00007fa3241d22b3 in st_pipe_format_to_mesa_format
     (format=format@entry=PIPE_FORMAT_R8G8B8A8_SRGB) at
     src/mesa/state_tracker/st_format.c:758
             __func__ = "st_pipe_format_to_mesa_format"
     #2  0x00007fa3241c8ec5 in st_new_renderbuffer_fb
     (format=format@entry=PIPE_FORMAT_R8G8B8A8_SRGB, samples=0,
     sw=<optimised out>) at src/mesa/state_tracker/st_cb_fbo.c:295
             strb = 0x19e8420
     #3  0x00007fa32409d355 in st_framebuffer_add_renderbuffer
     (stfb=stfb@entry=0x19e7fa0, idx=<optimised out>) at
     src/mesa/state_tracker/st_manager.c:314
             rb = <optimised out>
             format = PIPE_FORMAT_R8G8B8A8_SRGB
             sw = <optimised out>
     #4  0x00007fa32409e635 in st_framebuffer_create (st=0x19e7fa0,
     st=0x19e7fa0, stfbi=0x19e7a30) at
     src/mesa/state_tracker/st_manager.c:458
             stfb = 0x19e7fa0
             mode = {rgbMode = 1 '\001', floatMode = 0 '\000',
             colorIndexMode = 0 '\000', doubleBufferMode = 0, stereoMode
             =
             0, haveAccumBuffer = 0 '\000', haveDepthBuffer = 1 '\001',
             haveStencilBuffer = 1 '\001', redBits = 8, greenBits = 8,
             blueBits = 8, alphaBits = 8, redMask = 0, greenMask = 0,
             blueMask = 0, alphaMask = 0, rgbBits = 32, indexBits = 0,
             accumRedBits = 0, accumGreenBits = 0, accumBlueBits = 0,
             accumAlphaBits = 0, depthBits = 24, stencilBits = 8,
             numAuxBuffers = 0, level = 0, visualRating = 0,
             transparentPixel = 0, transparentRed = 0, transparentGreen =
             0, transparentBlue = 0, transparentAlpha = 0,
             transparentIndex
             = 0, sampleBuffers = 0, samples = 0, maxPbufferWidth = 0,
             maxPbufferHeight = 0, maxPbufferPixels = 0,
             optimalPbufferWidth = 0, optimalPbufferHeight = 0,
             swapMethod
             = 0, bindToTextureRgb = 0, bindToTextureRgba = 0,
             bindToMipmapTexture = 0, bindToTextureTargets = 0, yInverted
             =
             0, sRGBCapable = 1}
             idx = <optimised out>
     #5  st_framebuffer_reuse_or_create (st=st@entry=0x19dfce0,
     fb=<optimised out>, stfbi=stfbi@entry=0x19e7a30) at
     src/mesa/state_tracker/st_manager.c:728
     No locals.
     #6  0x00007fa32409e8cc in st_api_make_current (stapi=<optimised
     out>,
     stctxi=0x19dfce0, stdrawi=0x19e7a30, streadi=0x19e7a30) at
     src/mesa/state_tracker/st_manager.c:747
             st = 0x19dfce0
             stdraw = 0x640064
             stread = 0x1300000006
             ret = <optimised out>
     #7  0x00007fa324074a20 in XMesaMakeCurrent2 (c=c@entry=0x195bb00,
     drawBuffer=0x19e7e90, readBuffer=0x19e7e90) at
     src/gallium/state_trackers/glx/xlib/xm_api.c:1194
     No locals.
     #8  0x00007fa3240783c8 in glXMakeContextCurrent (dpy=0x194e900,
     draw=8388610, read=8388610, ctx=0x195bac0) at
     src/gallium/state_trackers/glx/xlib/glx_api.c:1177
             drawBuffer = <optimised out>
             readBuffer = <optimised out>
             xmctx = 0x195bb00
             glxCtx = 0x195bac0
             firsttime = 0 '\000'
             no_rast = 0 '\000'
     #9  0x00007fa32407852f in glXMakeCurrent (dpy=<optimised out>,
     drawable=<optimised out>, ctx=<optimised out>) at
     src/gallium/state_trackers/glx/xlib/glx_api.c:1211
     No locals.
---
   src/mesa/state_tracker/st_format.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/src/mesa/state_tracker/st_format.c
b/src/mesa/state_tracker/st_format.c
index 25577ac..0311a2b 100644
--- a/src/mesa/state_tracker/st_format.c
+++ b/src/mesa/state_tracker/st_format.c
@@ -753,6 +753,8 @@ st_pipe_format_to_mesa_format(enum pipe_format
format)

      case PIPE_FORMAT_B8G8R8X8_SRGB:
         return MESA_FORMAT_B8G8R8X8_SRGB;
+   case PIPE_FORMAT_R8G8B8A8_SRGB:
+      return MESA_FORMAT_B8G8R8A8_SRGB;

Hmm, we don't have a MESA_FORMAT_R8G8B8A8_SRGB to match that pipe format
so I'm not sure this is correct.

But if this fixes the crash, it's
better than nothing.   Acked-by: Brian Paul <bri...@vmware.com>

I'm hoping it does. I'm actually not sure as I couln't repro. I hope it
doesn't only moves the symptom away...

I think you're right, the problem won't go completely away so soon.

I think the problem is the st_manager.c changes in commit
4c68c6dcffe6c738d563eb0e0650bb865a5457b2 . In particular these lines:

    if (_mesa_is_desktop_gl(st->ctx)) {
       struct pipe_screen *screen = st->pipe->screen;
       const enum pipe_format srgb_format =
          util_format_srgb(stfbi->visual->color_format);

       if (srgb_format != PIPE_FORMAT_NONE &&
           screen->is_format_supported(screen, srgb_format,
                                       PIPE_TEXTURE_2D,
                                       stfbi->visual->samples,
                                       PIPE_BIND_RENDER_TARGET))
          mode.sRGBCapable = GL_TRUE;
    }

We are adding the sRGBCapable visual and using SRGB format if the pipe
drivers supports the SRGB variant, but we are not checking that the SRGB
format variant actually exists in Mesa.

I think we'll need to comment out this `mode.sRGBCapable = GL_TRUE`
assignment until this is addressed.

Hmm yes that looks messy. I guess an option (other than adding this
format to mesa which probably would be a good idea) could be to ditch
the assert from st_pipe_format_to_mesa_format() (the reverse function
doesn't have it neither), then run that format through this function
here and don't set the sRGBCapable bit if the return value was
PIPE_FORMAT_NONE.

Though I still don't understand why conform suddenly needs srgb formats.

I wonder if it is a matter of luck: some visuals have srgb, others don't, 
conform doesn't specify, and gets one at random?

Promoting a linear format to the corresponding sRGB format gives the
framebuffer the capability for sRGB writes.  Whether sRGB writes are
enabled still depends on GL_FRAMEBUFFER_SRGB.  But yeah, you will get
a random value when querying the winsys fbo for
GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING (instead of always
GL_LINEAR).

How about the attached patch?  It gives sRGB capability only to
framebuffers with formats recognized by mesa core.

It looks like there is a workaround/fix for this specific format.  To
avoid other breakages, there are more to fix.

Going over the possible return values of util_format_srgb(), there are
three formats

       PIPE_FORMAT_A8R8G8B8_SRGB
       PIPE_FORMAT_X8B8G8R8_SRGB
       PIPE_FORMAT_X8R8G8B8_SRGB

that have no corresponding mesa formats.  One solution is to add them,
and in st_framebuffer_create(), also add

   assert(st_pipe_format_to_mesa_format(srgb_format) != MESA_FORMAT_NONE);

to make debugging simpler in the future.

Looks like you're right. I guess we just got lucky because we don't get
the corresponding non-srgb formats so we don't hit those.

Having the state tracker support all possible sRGB formats has the
benefit that GLX_ARB_framebuffer_sRGB or EGL_KHR_gl_colorspace will be
easier to implement.  When the pipe driver supports
util_format_srgb(format), st/dri (or other state tracker managers) can
mark visuals with the format sRGB-capable without consulting st/mesa.

It seems without adding those formats for now we could just set the sRGB
capable bit only when st_pipe_format_to_mesa_format(srgb_format) !=
MESA_FORMAT_NONE.

I think that's the right solution too.  I'll post a patch...

-Brian

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev