Re: [PATCH] midispcur: Construct Picture objects lazily in case Render is disabled.

2010-05-25 Thread Pierre-Loup A. Griffais 

On 05/24/2010 07:05 PM, Jamey Sharp wrote:

On Mon, May 24, 2010 at 4:49 PM, Pierre-Loup A. Griffais
  wrote:

On 05/24/2010 09:34 AM, Jamey Sharp wrote:

These Pictures are not freed in miDCDeviceCleanup. Before the "don't
thrash resources" patch, there was a comment suggesting that
pRootPicture was already freed for some reason, but it did free
pTempPicture.


See http://lists.x.org/archives/xorg-devel/2010-April/007699.html


Ah, I get it. Thanks for the pointer; I've just sent a patch that
includes a comment explaining what's going on there.


The patch itself looks good to me.


Would you provide a "Reviewed-by" tag, and CC Keith, then?


Sure, sorry.

Reviewed-by: Pierre-Loup A. Griffais 



Thanks!
Jamey

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] midispcur: Construct Picture objects lazily in case Render is disabled.

2010-05-24 Thread Jamey Sharp 
On Mon, May 24, 2010 at 4:49 PM, Pierre-Loup A. Griffais
 wrote:
> On 05/24/2010 09:34 AM, Jamey Sharp wrote:
>> These Pictures are not freed in miDCDeviceCleanup. Before the "don't
>> thrash resources" patch, there was a comment suggesting that
>> pRootPicture was already freed for some reason, but it did free
>> pTempPicture.
>
> See http://lists.x.org/archives/xorg-devel/2010-April/007699.html

Ah, I get it. Thanks for the pointer; I've just sent a patch that
includes a comment explaining what's going on there.

> The patch itself looks good to me.

Would you provide a "Reviewed-by" tag, and CC Keith, then?

Thanks!
Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] midispcur: Construct Picture objects lazily in case Render is disabled.

2010-05-24 Thread Pierre-Loup A. Griffais 



On 05/24/2010 09:34 AM, Jamey Sharp wrote:

Reverts part of the effects of 518f3b189b6c8aa28b62837d14309fd06163ccbb,
"mi: don't thrash resources when displaying the software cursor across
screens". The per-screen cache is preserved, and the GCs are still
allocated eagerly, but now it doesn't construct pRootPicture until
somebody attempts to draw an ARGB cursor.

I noticed crashes in Xnest, which doesn't support the RENDER extension,
but I suspect other DDXes that support disabling that extension would
have had issues as well.

Signed-off-by: Jamey Sharp
---
These Pictures are not freed in miDCDeviceCleanup. Before the "don't
thrash resources" patch, there was a comment suggesting that
pRootPicture was already freed for some reason, but it did free
pTempPicture.


See http://lists.x.org/archives/xorg-devel/2010-April/007699.html

The patch itself looks good to me.

Thanks,
 - Pierre-Loup



Is there some reason not to free them, like that freeing the pixmaps
they're bound to automatically frees the pictures too, or something?

  mi/midispcur.c |   13 ++---
  1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/mi/midispcur.c b/mi/midispcur.c
index 16495e4..f2b2229 100644
--- a/mi/midispcur.c
+++ b/mi/midispcur.c
@@ -141,6 +141,7 @@ miDCRealizeCursor (ScreenPtr pScreen, CursorPtr pCursor)
  }

  #ifdef ARGB_CURSOR
+#define EnsurePicture(picture,draw,win) (picture || 
miDCMakePicture(&picture,draw,win))

  static VisualPtr
  miDCGetWindowVisual (WindowPtr pWin)
@@ -413,6 +414,8 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
  #ifdef ARGB_CURSOR
  if (pPriv->pPicture)
  {
+   if (!EnsurePicture(pBuffer->pRootPicture,&pWin->drawable, pWin))
+   return FALSE;
CompositePicture (PictOpOver,
  pPriv->pPicture,
  NULL,
@@ -695,9 +698,8 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
  #ifdef ARGB_CURSOR
  if (pPriv->pPicture)
  {
-   if (!pBuffer->pTempPicture)
-miDCMakePicture(&pBuffer->pTempPicture,&pTemp->drawable, pWin);
-
+   if (!EnsurePicture(pBuffer->pTempPicture,&pTemp->drawable, pWin))
+   return FALSE;
CompositePicture (PictOpOver,
  pPriv->pPicture,
  NULL,
@@ -781,10 +783,7 @@ miDCDeviceInitialize(DeviceIntPtr pDev, ScreenPtr pScreen)
  goto failure;

  #ifdef ARGB_CURSOR
-miDCMakePicture(&pBuffer->pRootPicture,&pWin->drawable, pWin);
-if (!pBuffer->pRootPicture)
-goto failure;
-
+pBuffer->pRootPicture = NULL;
  pBuffer->pTempPicture = NULL;
  #endif


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] midispcur: Construct Picture objects lazily in case Render is disabled.

2010-05-24 Thread Jamey Sharp 
Reverts part of the effects of 518f3b189b6c8aa28b62837d14309fd06163ccbb,
"mi: don't thrash resources when displaying the software cursor across
screens". The per-screen cache is preserved, and the GCs are still
allocated eagerly, but now it doesn't construct pRootPicture until
somebody attempts to draw an ARGB cursor.

I noticed crashes in Xnest, which doesn't support the RENDER extension,
but I suspect other DDXes that support disabling that extension would
have had issues as well.

Signed-off-by: Jamey Sharp 
---
These Pictures are not freed in miDCDeviceCleanup. Before the "don't
thrash resources" patch, there was a comment suggesting that
pRootPicture was already freed for some reason, but it did free
pTempPicture.

Is there some reason not to free them, like that freeing the pixmaps
they're bound to automatically frees the pictures too, or something?

 mi/midispcur.c |   13 ++---
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/mi/midispcur.c b/mi/midispcur.c
index 16495e4..f2b2229 100644
--- a/mi/midispcur.c
+++ b/mi/midispcur.c
@@ -141,6 +141,7 @@ miDCRealizeCursor (ScreenPtr pScreen, CursorPtr pCursor)
 }
 
 #ifdef ARGB_CURSOR
+#define EnsurePicture(picture,draw,win) (picture || 
miDCMakePicture(&picture,draw,win))
 
 static VisualPtr
 miDCGetWindowVisual (WindowPtr pWin)
@@ -413,6 +414,8 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
 #ifdef ARGB_CURSOR
 if (pPriv->pPicture)
 {
+   if (!EnsurePicture(pBuffer->pRootPicture, &pWin->drawable, pWin))
+   return FALSE;
CompositePicture (PictOpOver,
  pPriv->pPicture,
  NULL,
@@ -695,9 +698,8 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, 
CursorPtr pCursor,
 #ifdef ARGB_CURSOR
 if (pPriv->pPicture)
 {
-   if (!pBuffer->pTempPicture)
-miDCMakePicture(&pBuffer->pTempPicture, &pTemp->drawable, pWin);
-
+   if (!EnsurePicture(pBuffer->pTempPicture, &pTemp->drawable, pWin))
+   return FALSE;
CompositePicture (PictOpOver,
  pPriv->pPicture,
  NULL,
@@ -781,10 +783,7 @@ miDCDeviceInitialize(DeviceIntPtr pDev, ScreenPtr pScreen)
 goto failure;
 
 #ifdef ARGB_CURSOR
-miDCMakePicture(&pBuffer->pRootPicture, &pWin->drawable, pWin);
-if (!pBuffer->pRootPicture)
-goto failure;
-
+pBuffer->pRootPicture = NULL;
 pBuffer->pTempPicture = NULL;
 #endif
 
-- 
1.7.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel