On Thu, 2015-07-23 at 11:40 -0700, Anuj Phogat wrote: > On Wed, Jul 22, 2015 at 7:10 AM, Iago Toral <ito...@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 <ito...@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.pho...@gmail.com> > >> Cc: <mesa-sta...@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; > > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
