[Mesa-stable] [Mesa-dev] [PATCH V2 12/14] meta: Fix reading luminance texture as rgba in _mesa_meta_pbo_GetTexSubImage()

[Iago Toral]

On Thu, 2015-07-23 at 11:40 -0700, Anuj Phogat wrote:
> On Wed, Jul 22, 2015 at 7:10 AM, Iago Toral <itoral at igalia.com> wrote:
> > The problem here is that the _mesa_meta_BlitFramebuffer is not setting
> > G/B channels to 0.0 when doing Luminance/Intensity to RGBA conversions,
> > so why not implement the fix in _mesa_meta_BlitFramebuffer directly? The
> > GL spec expects frambuffer blits to handle these conversions properly,
> > so it looks like a win for all uses of that function.
> >
> I couldn't find an OpenGL spec reference suggesting this conversion in case
> of glBlitFrameBuffer.

It is not explicitly stated, however the OpenGL 4.5 spec, section 18.3.1
Blitting Pixel Rectangle says:

"An INVALID_OPERATION error is generated if format conversions are not
supported, which occurs under any of the following conditions:
• The read buffer contains fixed-point or floating-point values and any
draw buffer contains neither fixed-point nor floating-point values.
• The read buffer contains unsigned integer values and any draw buffer
does not contain unsigned integer values.
• The read buffer contains signed integer values and any draw buffer
does not contain signed integer values."

However, I realize now that luminance/intensity are not color-renderable
formats, so that text is probably not considering these formats anyway.

> What I found supports the current behavior of glBlitFrameBuffer:
> See table 3.23 on page 220 (of pdf) of glspec30.20080811.
> 
> Some relevant text from https://www.opengl.org/wiki/Image_Format:
> "When a GL_RED format is sampled in a shader, the resulting vec4 is
>  (Red, 0, 0, 1). When a GL_INTENSITY format is sampled, the resulting
>   vec4 is (I, I, I, I). The single intensity value is read into all four
>   components. For GL_LUMINANCE, the result is (L, L, L, 1). There is
>   also a two-channel GL_LUMINANCE_ALPHA format, which gives
>   (L, L, L, A)."
>
> I think glBlitFrameBuffer should also follow this being a drawing operation.
> What do you think?

Yes, it makes sense. You can add:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

to the patch.

> >
> > On Fri, 2015-07-17 at 10:28 -0700, Anuj Phogat wrote:
> >> After recent addition of pbo testing in piglit test getteximage-luminance,
> >> it fails on i965. This patch makes a sub test pass.
> >>
> >> This patch adds a clear color operation to meta pbo path, which I think is
> >> better than falling back to software path.
> >>
> >> V2: Fix color mask for GL_LUMINANCE_ALPHA
> >>
> >> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> >> Cc: <mesa-stable at lists.freedesktop.org>
> >> ---
> >>  src/mesa/drivers/common/meta_tex_subimage.c | 36 +++++++++++++++++++++++++++--
> >>  1 file changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c
> >> index 13f8292..f4d5ac3 100644
> >> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> >> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> >> @@ -28,6 +28,7 @@
> >>  #include "blend.h"
> >>  #include "bufferobj.h"
> >>  #include "buffers.h"
> >> +#include "clear.h"
> >>  #include "fbobject.h"
> >>  #include "glformats.h"
> >>  #include "glheader.h"
> >> @@ -278,8 +279,9 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> >>     int full_height, image_height;
> >>     struct gl_texture_image *pbo_tex_image;
> >>     struct gl_renderbuffer *rb = NULL;
> >> -   GLenum status;
> >> -   bool success = false;
> >> +   GLenum status, src_base_format;
> >> +   bool success = false, clear_channels_to_zero = false;
> >> +   float save_clear_color[4];
> >>     int z;
> >>
> >>     if (!_mesa_is_bufferobj(packing->BufferObj))
> >> @@ -380,6 +382,27 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> >>                                    GL_COLOR_BUFFER_BIT, GL_NEAREST))
> >>        goto fail;
> >>
> >> +   src_base_format = tex_image ?
> >> +                     tex_image->_BaseFormat :
> >> +                     ctx->ReadBuffer->_ColorReadBuffer->_BaseFormat;
> >> +
> >> +   /* Depending on the base formats involved we might need to rebase some
> >> +    * values. For example if we download from a Luminance format to RGBA
> >> +    * format, we want G=0 and B=0.
> >> +    */
> >> +   clear_channels_to_zero =
> >> +      _mesa_need_luminance_to_rgb_conversion(src_base_format,
> >> +                                             pbo_tex_image->_BaseFormat);
> >> +
> >> +   if (clear_channels_to_zero) {
> >> +      memcpy(save_clear_color, ctx->Color.ClearColor.f, 4 * sizeof(float));
> >> +      /* Clear the Green, Blue channels. */
> >> +      _mesa_ColorMask(GL_FALSE, GL_TRUE, GL_TRUE,
> >> +                      src_base_format != GL_LUMINANCE_ALPHA);
> >> +      _mesa_ClearColor(0.0, 0.0, 0.0, 1.0);
> >> +      _mesa_Clear(GL_COLOR_BUFFER_BIT);
> >> +   }
> >> +
> >>     for (z = 1; z < depth; z++) {
> >>        _mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
> >>                                  tex_image, zoffset + z);
> >> @@ -392,6 +415,15 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> >>                                   0, z * image_height,
> >>                                   width, z * image_height + height,
> >>                                   GL_COLOR_BUFFER_BIT, GL_NEAREST);
> >> +      if (clear_channels_to_zero)
> >> +         _mesa_Clear(GL_COLOR_BUFFER_BIT);
> >> +   }
> >> +
> >> +   /* Unmask the color channels and restore the saved clear color values. */
> >> +   if (clear_channels_to_zero) {
> >> +      _mesa_ColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);
> >> +      _mesa_ClearColor(save_clear_color[0], save_clear_color[1],
> >> +                       save_clear_color[2], save_clear_color[3]);
> >>     }
> >>
> >>     success = true;
> >
> >
>