On 25 September 2012 06:53, Brian Paul <bri...@vmware.com> wrote:
> On 09/24/2012 05:49 PM, Paul Berry wrote:
>
>> 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);
>> }
>>
>> + if (state& MESA_META_FRAMEBUFFER_SRGB) {
>>
>> + 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);
>> }
>>
>> + if (state& MESA_META_FRAMEBUFFER_SRGB) {
>>
>> + 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);
>> }
>> - if ((_mesa_is_desktop_gl(ctx)&& ctx->Extensions.EXT_**
>> framebuffer_sRGB)
>>
>> - || _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);
>> }
>> - if (ctx->Extensions.EXT_**framebuffer_sRGB&& fbo_srgb_save) {
>>
>> - _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;
>>
>> if (buffers& BUFFER_BITS_COLOR) {
>>
>> @@ -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);
>> }
>>
>> - if (ctx->Extensions.EXT_**framebuffer_sRGB&& srgbBufferSave) {
>>
>> - _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.*/
>> - if ((_mesa_is_desktop_gl(ctx)&& ctx->Extensions.EXT_**
>> framebuffer_sRGB)
>>
>> - || _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
>>
>
> 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 <bri...@vmware.com>
>
>
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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
