[Mesa-dev] [PATCH] st/mesa: Add R8G8B8A8_SRGB case to st_pipe_format_to_mesa_format.
Jose Fonseca
jfonseca at vmware.com
Fri Mar 7 12:12:19 PST 2014
----- Original Message -----
> 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 <olvaffe at gmail.com> wrote:
> >>> On Fri, Mar 7, 2014 at 2:04 AM, Jose Fonseca <jfonseca at 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, jfonseca at vmware.com wrote:
> >>>>>>>>> From: José Fonseca <jfonseca at 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 at entry=0x7fa324df2ed7 "0",
> >>>>>>>>> file=file at entry=0x7fa324e3fc30
> >>>>>>>>> "src/mesa/state_tracker/st_format.c",
> >>>>>>>>> line=line at entry=758, function=function at 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 at entry=0x7fa324df2ed7 "0",
> >>>>>>>>> file=file at entry=0x7fa324e3fc30
> >>>>>>>>> "src/mesa/state_tracker/st_format.c",
> >>>>>>>>> line=line at entry=758, function=function at 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 at 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 at 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 at 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 at entry=0x19dfce0,
> >>>>>>>>> fb=<optimised out>, stfbi=stfbi at 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 at 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 <brianp at 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...
Sounds good to me too.
But if SRGB is assumed nowadays, we'll really need to add all this SRGB variants, or blacklist the Mesa formats / visuals which don't have SRGB counterpart.
Jose
More information about the mesa-dev
mailing list