On 25 September 2012 06:53, Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">On 09/24/2012 05:49 PM, Paul Berry wrote: </div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> Previously, meta logic was saving and restoring the value of GL_FRAMEBUFFER_SRGB in an ad-hoc fashion. As a result, it was not properly disabled and/or restored for some meta operations. This patch causes GL_FRAMEBUFFER_SRGB to be saved/restored in the conventional way of meta-ops (using _mesa_meta_begin() and _mesa_meta_end()). It is now reliably saved/restored for _mesa_meta_BlitFramebuffer, _mesa_meta_GenerateMipmap, and decompress_texture_image, and preserved for all other meta ops. Fixes piglit tests "ARB_framebuffer_sRGB/blit renderbuffer {linear_to_srgb,srgb} scaled {disabled,enabled}". --- src/mesa/drivers/common/meta.c | 47 ++++++++++++++++++------------------------ src/mesa/drivers/common/meta.h | 1 + 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c index 28a79b0..6689337 100644 --- a/src/mesa/drivers/common/meta.c +++ b/src/mesa/drivers/common/meta.c @@ -187,6 +187,9 @@ struct save_state /** MESA_META_MULTISAMPLE */ GLboolean MultisampleEnabled; + /** MESA_META_FRAMEBUFFER_SRGB */ + GLboolean sRGBEnabled; + /** Miscellaneous (always disabled) */ GLboolean Lighting; GLboolean RasterDiscard; @@ -773,6 +776,12 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield state) _mesa_set_multisample(ctx, GL_FALSE); } </div></div> + if (state& MESA_META_FRAMEBUFFER_SRGB) {<div class="im"> + save->sRGBEnabled = ctx->Color.sRGBEnabled; + if (ctx->Color.sRGBEnabled) + _mesa_set_framebuffer_srgb(ctx, GL_FALSE); + } + /* misc */ { save->Lighting = ctx->Light.Enabled; @@ -1075,6 +1084,11 @@ _mesa_meta_end(struct gl_context *ctx) _mesa_set_multisample(ctx, save->MultisampleEnabled); } </div> + if (state& MESA_META_FRAMEBUFFER_SRGB) {<div><div class="h5"> + if (ctx->Color.sRGBEnabled != save->sRGBEnabled) + _mesa_set_framebuffer_srgb(ctx, save->sRGBEnabled); + } + /* misc */ if (save->Lighting) { _mesa_set_enable(ctx, GL_LIGHTING, GL_TRUE); @@ -1394,7 +1408,6 @@ blitframebuffer_texture(struct gl_context *ctx, const GLuint srcLevel = readAtt->TextureLevel; const GLint baseLevelSave = texObj->BaseLevel; const GLint maxLevelSave = texObj->MaxLevel; - const GLenum fbo_srgb_save = ctx->Color.sRGBEnabled; const GLenum target = texObj->Target; GLuint sampler, samplerSave = ctx->Texture.Unit[ctx->Texture.CurrentUnit].Sampler ? @@ -1433,15 +1446,14 @@ blitframebuffer_texture(struct gl_context *ctx, _mesa_SamplerParameteri(sampler, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); _mesa_SamplerParameteri(sampler, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); - /* Always do our blits with no sRGB decode or encode.*/ + /* Always do our blits with no sRGB decode or encode. Note that + * GL_FRAMEBUFFER_SRGB has already been disabled by + * _mesa_meta_begin(). + */ if (ctx->Extensions.EXT_texture_sRGB_decode) { _mesa_SamplerParameteri(sampler, GL_TEXTURE_SRGB_DECODE_EXT, GL_SKIP_DECODE_EXT); } </div></div> - if ((_mesa_is_desktop_gl(ctx)&& ctx->Extensions.EXT_framebuffer_sRGB)<div class="im"> - || _mesa_is_gles3(ctx)) { - _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_FALSE); - } _mesa_TexEnvi(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_REPLACE); _mesa_set_enable(ctx, target, GL_TRUE); @@ -1500,9 +1512,6 @@ blitframebuffer_texture(struct gl_context *ctx, _mesa_TexParameteri(target, GL_TEXTURE_BASE_LEVEL, baseLevelSave); _mesa_TexParameteri(target, GL_TEXTURE_MAX_LEVEL, maxLevelSave); } </div> - if (ctx->Extensions.EXT_framebuffer_sRGB&& fbo_srgb_save) {<div class="im"> - _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_TRUE); - } _mesa_BindSampler(ctx->Texture.CurrentUnit, samplerSave); _mesa_DeleteSamplers(1,&sampler); @@ -1713,7 +1722,8 @@ _mesa_meta_Clear(struct gl_context *ctx, GLbitfield buffers) GLbitfield metaSave = (MESA_META_ALL - MESA_META_SCISSOR - MESA_META_PIXEL_STORE - - MESA_META_CONDITIONAL_RENDER); + MESA_META_CONDITIONAL_RENDER - + MESA_META_FRAMEBUFFER_SRGB); const GLuint stencilMax = (1<< ctx->DrawBuffer->Visual.stencilBits) - 1; </div> if (buffers& BUFFER_BITS_COLOR) {<div class="im"> @@ -3236,7 +3246,6 @@ _mesa_meta_GenerateMipmap(struct gl_context *ctx, GLenum target, const GLuint maxLevel = texObj->MaxLevel; const GLint maxLevelSave = texObj->MaxLevel; const GLboolean genMipmapSave = texObj->GenerateMipmap; - const GLenum srgbBufferSave = ctx->Color.sRGBEnabled; const GLuint fboSave = ctx->DrawBuffer->Name; const GLuint currentTexUnitSave = ctx->Texture.CurrentUnit; const GLboolean use_glsl_version = ctx->Extensions.ARB_vertex_shader&& @@ -3330,12 +3339,6 @@ _mesa_meta_GenerateMipmap(struct gl_context *ctx, GLenum target, else assert(!genMipmapSave); - if ((ctx->Extensions.EXT_framebuffer_sRGB&& - _mesa_is_desktop_gl(ctx)) || - _mesa_is_gles3(ctx)) { - _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_FALSE); - } - /* Setup texture coordinates */ setup_texture_coords(faceTarget, slice, @@ -3452,10 +3455,6 @@ _mesa_meta_GenerateMipmap(struct gl_context *ctx, GLenum target, _mesa_DrawArrays(GL_TRIANGLE_FAN, 0, 4); } </div> - if (ctx->Extensions.EXT_framebuffer_sRGB&& srgbBufferSave) {<div class="im"> - _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_TRUE); - } - _mesa_lock_texture(ctx, texObj); /* relock */ _mesa_BindSampler(ctx->Texture.CurrentUnit, samplerSave); @@ -3734,12 +3733,6 @@ decompress_texture_image(struct gl_context *ctx, _mesa_TexParameteri(target, GL_TEXTURE_MAX_LEVEL, texImage->Level); } - /* No sRGB decode or encode.*/ </div> - if ((_mesa_is_desktop_gl(ctx)&& ctx->Extensions.EXT_framebuffer_sRGB)<div class="im"> - || _mesa_is_gles3(ctx)) { - _mesa_set_enable(ctx, GL_FRAMEBUFFER_SRGB_EXT, GL_FALSE); - } - /* render quad w/ texture into renderbuffer */ _mesa_DrawArrays(GL_TRIANGLE_FAN, 0, 4); diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h index d8dfb56..6ffc5b5 100644 --- a/src/mesa/drivers/common/meta.h +++ b/src/mesa/drivers/common/meta.h @@ -56,6 +56,7 @@ #define MESA_META_CLIP 0x40000 #define MESA_META_SELECT_FEEDBACK 0x80000 #define MESA_META_MULTISAMPLE 0x100000 +#define MESA_META_FRAMEBUFFER_SRGB 0x200000 /**\}*/ extern void </div></blockquote> Assuming you've checked all the _mesa_meta_begin() calls to check if the new MESA_META_FRAMEBUFFER_SRGB flag is needed, Reviewed-by: Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>> </blockquote></div> I have checked all the _mesa_meta_begin() calls, but if someone wants to verify that my reasoning is correct, I would appreciate it. Specifically, I reasoned that: - The calls to _mesa_meta_begin() in _mesa_meta_CopyPixels(), _mesa_meta_DrawPixels(), _mesa_meta_Bitmap(), and _mesa_meta_DrawTex() should *not* include MESA_META_FRAMEBUFFER_SRGB, because fragments produced by these calls are supposed to undergo normal fragment processing. - The call to _mesa_meta_begin() in _mesa_meta_GenerateMipmap() *should* include MESA_META_FRAMEBUFFER_SRGB, because sRGB encoding during mipmap generation would cause different miplevels to have different brightness. - The calls to _mesa_meta_begin() in _mesa_meta_CopyTexSubImage() don't need to include MESA_META_FRAMEBUFFER_SRGB, because the only operations that this function performs while meta is active are ReadPixels() and TexSubImage(), which won't be affected by the FRAMEBUFFER_SRGB setting. - The call to _mesa_meta_begin() in decompress_texture_image() *should* include MESA_META_FRAMEBUFFER_SRGB, since the behaviour of textures shouldn't depend on the setting of MESA_META_FRAMEBUFFER_SRGB. Do those sound right? I'm confident about the others.