[Glamor] [PATCH 7/7] Fix the problem because of GLAMOR_CREATE_PIXMAP_FIXUP deleted when pixman createing.

[Zhigang Gong]

On Tue, May 15, 2012 at 01:43:57PM +0800, He Junyan wrote:
> 于 2012/5/15 13:10, Zhigang Gong 写道:
> >On Tue, May 08, 2012 at 08:45:20AM +0800, junyan.he at linux.intel.com wrote:
> >>From: Junyan He<junyan.he at linux.intel.com>
> >>
> >>  A bug is caused by change the pixmap's creating logic.
> >>  The flag GLAMOR_CREATE_PIXMAP_FIXUP is deleted when
> >>  creates the pixman which will be used to contain the
> >>  gradient picture. Without this flag, the FBO can be
> >>  re-used and the performance will be improved but the
> >>  size of FBO may be bigger than the picture's real size.
> >>  This will cause the caculation for xscale and yscale
> >>  using pixmap_priv_get_scale get a wrong scale. The
> >>  result is that the gradient picture seems to have some
> >>  offset comparing to the right one. Fix this bug.
> >>
> >>
> >>Signed-off-by: Junyan He<junyan.he at linux.intel.com>
> >>---
> >>  src/glamor_gradient.c |   19 +++++++++++++------
> >>  1 files changed, 13 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/src/glamor_gradient.c b/src/glamor_gradient.c
> >>index ebedb45..c57260b 100644
> >>--- a/src/glamor_gradient.c
> >>+++ b/src/glamor_gradient.c
> >>@@ -799,12 +799,19 @@ _glamor_gradient_set_pixmap_destination(ScreenPtr screen,
> >>
> >>  	dispatch = glamor_get_dispatch(glamor_priv);
> >>
> >>-	glamor_set_destination_pixmap_priv_nc(pixmap_priv);
> >>-
> >>-	pixmap_priv_get_scale(pixmap_priv, xscale, yscale);
> >>-
> >>-	glamor_priv->has_source_coords = 1;
> >>-	glamor_priv->has_mask_coords = 0;
> >>+	dispatch->glBindFramebuffer(GL_FRAMEBUFFER, pixmap_priv->fbo->fb);
> >>+#ifndef GLAMOR_GLES2
> >>+	dispatch->glMatrixMode(GL_PROJECTION);
> >>+	dispatch->glLoadIdentity();
> >>+	dispatch->glMatrixMode(GL_MODELVIEW);
> >>+	dispatch->glLoadIdentity();
> >>+#endif
> >>+	dispatch->glViewport(0, 0,
> >>+	                     dst_picture->pDrawable->width,
> >>+	                     dst_picture->pDrawable->height);
> >>+
> >>+	*xscale = 1.0 / (dst_picture->pDrawable->width);
> >>+	*yscale = 1.0 / (INT16)(dst_picture->pDrawable->height);
> >
> >This patch looks incorrect to me. Now the texture may larger than
> >the pDrawable->width*pDrawable->height. Then if you set a viewport
> >to pDrawable->width*pDrawable->height, then the texture coords will
> >not match to the pixel by a 1:1 manner which may be not what you want.
> >
> >For example, if a pDrawable has a 10x10 pixel array, but it get a 100x100
> >texture. Then here if you set the viewport to 10x10, then it means 10 pixels
> >in the texture map to one logical pixel. This may not efficient and sometimes
> >may cause some unaligned issue.
> >
> >I wonder does this patch really fix some cairo test failures?
> >
> In my consideration, the bugs cause by the fbo's height and width is
> bigger than
> the gradient's Drawable need. So it causes the xscale and yscale be
> a wrong value.
> I think set glViewport to (0, 0) --->
> (dst_picture->pDrawable->width, dst_picture->pDrawable->height)
> will make the  X Y coordinate from (0,0) --> (width, height) to (-1,
> -1), (1, 1),
> that is what I need? 
If your algorithm requires the drawable map to (-1,-1) (1, 1) of the fbo/texture,
then you do need the fixup flag to get an eaxct size fbo/texture for your drawable.

As to the viewport, you'd better always set it to align with the texture's
actual size, that means fbo->width * fbo->height not drawable's width and height,
to avoid any unecessary non 1:1 sampling. And once the viewport is determined,
xscale and yscale are also determined by the viewport's width and height. And
also, it should be fbo's widht and fbo's height.

> Is my understanding right?

May not correct here. Please think about whether your algorithm relies on a
exact fbo/texture or not. If the anwser is yes, then please add FIXUP flag when
create the gradient picture.

> 
> 
> >>
> >>  	DEBUGF("xscale = %f, yscale = %f,"
> >>  	       " x_source = %d, y_source = %d, width = %d, height = %d\n",
> >>--
> >>1.7.7.6
> >>
> >
> _______________________________________________
> Glamor mailing list
> Glamor at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor