[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 02:11:00PM +0800, Zhigang Gong wrote:
> 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.

After a short discussion, I agree that glViewport to the pixmap's size
is correct here. I was thinking about the source pixmap case which we
don't have a glViewport there and have to deal with partial texture's
usage. This also indicates me to refine the glamor_set_destination_pixmap_priv_nc.
Set viewport to the fbo's size is not efficient. And we may need two
type of pixmap_priv_get_scale, one is for the fbo's size which is used by
source/mask pixmap, and the other is for the pixmap's size which is used by
destination pixmap.

So Reviewed-by: Zhigang Gong <zhigang.gong at gmail.com>

> 
> > 
> > 
> > >>
> > >>  	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
> _______________________________________________
> Glamor mailing list
> Glamor at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor