Re: [Mesa-dev] [PATCH 1/2] meta: Add a state flag for the GL_DITHER

2014-07-10 Thread Pohjolainen, Topi 
On Mon, Jul 07, 2014 at 12:27:29PM +0100, Neil Roberts wrote:
 Pohjolainen, Topi topi.pohjolai...@intel.com writes:
 
  All the other state flags considered in _mesa_meta_begin() are
  explicitly set as disabled. And having noticed that you
  unconditionally disable dithering also in cleartexsubimage_using_fbo()
  I'm wondering if I'm missing something.
 
 My understanding is that _mesa_meta_begin is supposed to reset the state
 to the GL defaults and dithering is enabled by default in GL.
 
 If I were to set it to disabled then it would also affect things like
 _mesa_meta_GenerateMipmap which uses MESA_META_ALL. Presumably that
 function will currently either use dithering or not depending on whether
 the user has toggled the state. I think it makes sense for that function
 to always use dithering so this patch arguably fixes a bug with that
 function (and others).

Thanks for the explanation, that makes perfect sense to me at least.

 
 Thanks for the review.
 
 Regards,
 - Neil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] meta: Add a state flag for the GL_DITHER

2014-07-07 Thread Pohjolainen, Topi 
On Fri, Jul 04, 2014 at 05:26:43PM +0100, Neil Roberts wrote:
 The Meta implementation of glClearTexSubImage is going to want to ensure that
 dithering is disabled so that it can get a consistent color across the whole
 texture when clearing. This adds a state flag to easily save it and set it to
 the default value when performing meta operations.
 ---
  src/mesa/drivers/common/meta.c | 8 
  src/mesa/drivers/common/meta.h | 4 
  2 files changed, 12 insertions(+)
 
 diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
 index f1f5729..10dd418 100644
 --- a/src/mesa/drivers/common/meta.c
 +++ b/src/mesa/drivers/common/meta.c
 @@ -505,6 +505,11 @@ _mesa_meta_begin(struct gl_context *ctx, GLbitfield 
 state)
   _mesa_set_enable(ctx, GL_COLOR_LOGIC_OP, GL_FALSE);
 }
  
 +   if (state  MESA_META_DITHER) {
 +  save-DitherFlag = ctx-Color.DitherFlag;
 +  _mesa_set_enable(ctx, GL_DITHER, GL_TRUE);

All the other state flags considered in _mesa_meta_begin() are explicitly set
as disabled. And having noticed that you unconditionally disable dithering
also in cleartexsubimage_using_fbo() I'm wondering if I'm missing something.

Otherwise looks good to me:

Reviewed-by: Topi Pohjolainen topi.pohjolai...@intel.com

 +   }
 +
 if (state  MESA_META_COLOR_MASK) {
memcpy(save-ColorMask, ctx-Color.ColorMask,
   sizeof(ctx-Color.ColorMask));
 @@ -875,6 +880,9 @@ _mesa_meta_end(struct gl_context *ctx)
   _mesa_set_enable(ctx, GL_COLOR_LOGIC_OP, save-ColorLogicOpEnabled);
 }
  
 +   if (state  MESA_META_DITHER)
 +  _mesa_set_enable(ctx, GL_DITHER, save-DitherFlag);
 +
 if (state  MESA_META_COLOR_MASK) {
GLuint i;
for (i = 0; i  ctx-Const.MaxDrawBuffers; i++) {
 diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h
 index 765f8df..b8d992c 100644
 --- a/src/mesa/drivers/common/meta.h
 +++ b/src/mesa/drivers/common/meta.h
 @@ -59,6 +59,7 @@
  #define MESA_META_FRAMEBUFFER_SRGB 0x20
  #define MESA_META_OCCLUSION_QUERY  0x40
  #define MESA_META_DRAW_BUFFERS 0x80
 +#define MESA_META_DITHER  0x100
  /**\}*/
  
  /**
 @@ -84,6 +85,9 @@ struct save_state
 GLbitfield BlendEnabled;
 GLboolean ColorLogicOpEnabled;
  
 +   /** MESA_META_DITHER */
 +   GLboolean DitherFlag;
 +
 /** MESA_META_COLOR_MASK */
 GLubyte ColorMask[MAX_DRAW_BUFFERS][4];
  
 -- 
 1.9.3
 
 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/2] meta: Add a state flag for the GL_DITHER

2014-07-07 Thread Neil Roberts 
Pohjolainen, Topi topi.pohjolai...@intel.com writes:

 All the other state flags considered in _mesa_meta_begin() are
 explicitly set as disabled. And having noticed that you
 unconditionally disable dithering also in cleartexsubimage_using_fbo()
 I'm wondering if I'm missing something.

My understanding is that _mesa_meta_begin is supposed to reset the state
to the GL defaults and dithering is enabled by default in GL.

If I were to set it to disabled then it would also affect things like
_mesa_meta_GenerateMipmap which uses MESA_META_ALL. Presumably that
function will currently either use dithering or not depending on whether
the user has toggled the state. I think it makes sense for that function
to always use dithering so this patch arguably fixes a bug with that
function (and others).

Thanks for the review.

Regards,
- Neil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev