Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, 17 Jul 2012 12:28:14 -0400 Brian Hinz bph...@users.sourceforge.net wrote: So far everything looks good!! I'll continue testing today but in either case thank you for taking the time to work on this. Np. It's been on the todo list for a bit long already. Good to have it out of the way. :) Adam, do you know where the pCompositeClip thing comes from? I couldn't find anything in the archive. I don't want to commit something when I don't know why it's there. Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, 13 Jul 2012 15:36:28 +0200
Pierre Ossman oss...@cendio.se wrote:
Not sure what the fix is. Starting to activate hooks even for just the
screen pixmap still means we have to audit the code for assumptions
about windows. Not terribly appealing. I'll dig around and see if I can
find something cleaner...
Bah. Can't really find a clean way of doing this without hooking the GC
ops. So here is an attempt at making those pixmap safe, and hooking the
screen pixmap (other pixmaps are still ignored).
Adam, was there some background info to why we should use
pCompositeClip?
Rgds
--
Pierre Ossman Software Development
Cendio AB http://cendio.com
Teknikringen 8 http://twitter.com/ThinLinc
583 30 Linköpinghttp://facebook.com/ThinLinc
Phone: +46-13-214600http://plus.google.com/112509906846170010689
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Index: vncHooks.cc
===
--- vncHooks.cc (revision 25497)
+++ vncHooks.cc (working copy)
@@ -750,7 +750,7 @@
};
-// ValidateGC - wrap the ops if a viewable window
+// ValidateGC - wrap the ops if a viewable window OR the screen pixmap
static void vncHooksValidateGC(GCPtr pGC, unsigned long changes,
DrawablePtr pDrawable)
@@ -762,7 +762,9 @@
(*pGC-funcs-ValidateGC) (pGC, changes, pDrawable);
u.vncHooksGC-wrappedOps = 0;
- if (pDrawable-type == DRAWABLE_WINDOW ((WindowPtr) pDrawable)-viewable) {
+ if ((pDrawable-type == DRAWABLE_WINDOW
+ ((WindowPtr) pDrawable)-viewable) ||
+ (pDrawable == pGC-pScreen-GetScreenPixmap(pGC-pScreen)-drawable)) {
u.vncHooksGC-wrappedOps = pGC-ops;
DBGPRINT((stderr,vncHooksValidateGC: wrapped GC ops\n));
}
@@ -842,7 +844,7 @@
{
GC_OP_UNWRAPPER(pDrawable, pGC, FillSpans);
- RegionHelper changed(pScreen, ((WindowPtr)pDrawable)-borderClip);
+ RegionHelper changed(pScreen, pGC-pCompositeClip);
(*pGC-ops-FillSpans) (pDrawable, pGC, nInit, pptInit, pwidthInit, fSorted);
@@ -858,7 +860,7 @@
{
GC_OP_UNWRAPPER(pDrawable, pGC, SetSpans);
- RegionHelper changed(pScreen, ((WindowPtr)pDrawable)-borderClip);
+ RegionHelper changed(pScreen, pGC-pCompositeClip);
(*pGC-ops-SetSpans) (pDrawable, pGC, psrc, ppt, pwidth, nspans, fSorted);
@@ -910,16 +912,19 @@
RegionHelper src(pScreen);
- if ((pSrc-type == DRAWABLE_WINDOW) (pSrc-pScreen == pScreen)) {
+ if (pSrc-pScreen == pScreen) {
box.x1 = srcx + pSrc-x;
box.y1 = srcy + pSrc-y;
box.x2 = box.x1 + w;
box.y2 = box.y1 + h;
src.init(box, 0);
-if (REGION_NOTEMPTY(pScreen, ((WindowPtr)pSrc)-clipList)) {
- REGION_INTERSECT(pScreen, src.reg, src.reg, ((WindowPtr)pSrc)-clipList);
+
+if ((pSrc-type == DRAWABLE_WINDOW)
+REGION_NOTEMPTY(pScreen, ((WindowPtr)pSrc)-clipList)) {
+ REGION_INTERSECT(pScreen, src.reg, src.reg, ((WindowPtr)pSrc)-clipList);
}
+
REGION_TRANSLATE(pScreen, src.reg,
dstx + pDst-x - srcx - pSrc-x,
dsty + pDst-y - srcy - pSrc-y);
signature.asc
Description: PGP signature
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, 17 Jul 2012 07:48:11 -0400 Brian Hinz bph...@users.sourceforge.net wrote: I think this patch causes backgrounds to be drawn when they should not be (see the attached screen captures). The borders seem to be correct in fluxbox though. Ehm... That's extremely odd. The affected code just tracks changes. It should not influence the actual drawing in any way. I wonder if this is a different bug that just got exposed. Perhaps the background is drawn, followed by the menu, and we know somehow miss sending the update for the menu. How easily reproduced is this? And have you disabled deferred updates? Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, Jul 17, 2012 at 8:00 AM, Pierre Ossman wrote: On Tue, 17 Jul 2012 07:48:11 -0400 Brian Hinz wrote: I think this patch causes backgrounds to be drawn when they should not be (see the attached screen captures). The borders seem to be correct in fluxbox though. Ehm... That's extremely odd. The affected code just tracks changes. It should not influence the actual drawing in any way. I wonder if this is a different bug that just got exposed. Perhaps the background is drawn, followed by the menu, and we know somehow miss sending the update for the menu. How easily reproduced is this? And have you disabled deferred updates? Completely reproducible. I only sent a couple of screenshots, but it's nearly unusable because the background is painted over on almost any menu, textbox, etc. DeferUpdate=0 paints the background instantly, so that the menu is never seen, setting it high shows the menu briefly and then it's painted over. Just like with the borders, a client refresh request repaints everything correctly. -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, 17 Jul 2012 08:14:15 -0400
Brian Hinz bph...@users.sourceforge.net wrote:
Completely reproducible. I only sent a couple of screenshots, but it's
nearly unusable because the background is painted over on almost any menu,
textbox, etc. DeferUpdate=0 paints the background instantly, so that the
menu is never seen, setting it high shows the menu briefly and then it's
painted over. Just like with the borders, a client refresh request
repaints everything correctly.
Found it. The CopyArea operation can have any pixmap as the source,
meaning the coordinates might be wrong (did I mention using pixmaps was
problematic? :)).
Try the updated patch.
Rgds
--
Pierre Ossman Software Development
Cendio AB http://cendio.com
Teknikringen 8 http://twitter.com/ThinLinc
583 30 Linköpinghttp://facebook.com/ThinLinc
Phone: +46-13-214600http://plus.google.com/112509906846170010689
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Index: vncHooks.cc
===
--- vncHooks.cc (revision 25497)
+++ vncHooks.cc (working copy)
@@ -750,7 +750,7 @@
};
-// ValidateGC - wrap the ops if a viewable window
+// ValidateGC - wrap the ops if a viewable window OR the screen pixmap
static void vncHooksValidateGC(GCPtr pGC, unsigned long changes,
DrawablePtr pDrawable)
@@ -762,7 +762,9 @@
(*pGC-funcs-ValidateGC) (pGC, changes, pDrawable);
u.vncHooksGC-wrappedOps = 0;
- if (pDrawable-type == DRAWABLE_WINDOW ((WindowPtr) pDrawable)-viewable) {
+ if ((pDrawable-type == DRAWABLE_WINDOW
+ ((WindowPtr) pDrawable)-viewable) ||
+ (pDrawable == pGC-pScreen-GetScreenPixmap(pGC-pScreen)-drawable)) {
u.vncHooksGC-wrappedOps = pGC-ops;
DBGPRINT((stderr,vncHooksValidateGC: wrapped GC ops\n));
}
@@ -842,7 +844,7 @@
{
GC_OP_UNWRAPPER(pDrawable, pGC, FillSpans);
- RegionHelper changed(pScreen, ((WindowPtr)pDrawable)-borderClip);
+ RegionHelper changed(pScreen, pGC-pCompositeClip);
(*pGC-ops-FillSpans) (pDrawable, pGC, nInit, pptInit, pwidthInit, fSorted);
@@ -858,7 +860,7 @@
{
GC_OP_UNWRAPPER(pDrawable, pGC, SetSpans);
- RegionHelper changed(pScreen, ((WindowPtr)pDrawable)-borderClip);
+ RegionHelper changed(pScreen, pGC-pCompositeClip);
(*pGC-ops-SetSpans) (pDrawable, pGC, psrc, ppt, pwidth, nspans, fSorted);
@@ -910,16 +912,23 @@
RegionHelper src(pScreen);
- if ((pSrc-type == DRAWABLE_WINDOW) (pSrc-pScreen == pScreen)) {
+ // The source of the data has to be something that's on screen.
+ // This means either a window, or the screen pixmap.
+ if ((pSrc-pScreen == pScreen)
+ ((pSrc-type == DRAWABLE_WINDOW) ||
+ (pSrc == pScreen-GetScreenPixmap(pScreen)-drawable))) {
box.x1 = srcx + pSrc-x;
box.y1 = srcy + pSrc-y;
box.x2 = box.x1 + w;
box.y2 = box.y1 + h;
src.init(box, 0);
-if (REGION_NOTEMPTY(pScreen, ((WindowPtr)pSrc)-clipList)) {
- REGION_INTERSECT(pScreen, src.reg, src.reg, ((WindowPtr)pSrc)-clipList);
+
+if ((pSrc-type == DRAWABLE_WINDOW)
+REGION_NOTEMPTY(pScreen, ((WindowPtr)pSrc)-clipList)) {
+ REGION_INTERSECT(pScreen, src.reg, src.reg, ((WindowPtr)pSrc)-clipList);
}
+
REGION_TRANSLATE(pScreen, src.reg,
dstx + pDst-x - srcx - pSrc-x,
dsty + pDst-y - srcy - pSrc-y);
signature.asc
Description: PGP signature
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, Jul 17, 2012 at 10:14 AM, Pierre Ossman wrote: On Tue, 17 Jul 2012 08:14:15 -0400 Brian Hinz wrote: Completely reproducible. I only sent a couple of screenshots, but it's nearly unusable because the background is painted over on almost any menu, textbox, etc. DeferUpdate=0 paints the background instantly, so that the menu is never seen, setting it high shows the menu briefly and then it's painted over. Just like with the borders, a client refresh request repaints everything correctly. Found it. The CopyArea operation can have any pixmap as the source, meaning the coordinates might be wrong (did I mention using pixmaps was problematic? :)). Try the updated patch. So far everything looks good!! I'll continue testing today but in either case thank you for taking the time to work on this. -brian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Thu, 12 Jul 2012 18:04:40 +0200 Adam Tkac at...@redhat.com wrote: I dived into vncHooks and xserver sources and attached patch should fix both screen artefacts and XDrawArc crashes. It reverts r4220 and fixes vncHooksFillSpans hook which used wrong clip pointer. -Inf Hooking pixmaps is just asking for problems. You'll be using the pixmap's coordinates for tracking changes, which makes absolutely no sense there is no fundamental connection to screen coordinates (like there is for windows). We need to find a proper way to deal with this specific case (borders). It might be that it's impossible to hook the actual drawing itself, but then we'll have to look at its triggering conditions instead and duplicate some of the border logic. E.g. hooking expose events and checking window flags. Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On 7/13/12 3:09 AM, Pierre Ossman wrote: I dived into vncHooks and xserver sources and attached patch should fix both screen artefacts and XDrawArc crashes. It reverts r4220 and fixes vncHooksFillSpans hook which used wrong clip pointer. -Inf Hooking pixmaps is just asking for problems. You'll be using the pixmap's coordinates for tracking changes, which makes absolutely no sense there is no fundamental connection to screen coordinates (like there is for windows). We need to find a proper way to deal with this specific case (borders). It might be that it's impossible to hook the actual drawing itself, but then we'll have to look at its triggering conditions instead and duplicate some of the border logic. E.g. hooking expose events and checking window flags. Maybe I'm missing something, but isn't the goal to hook only drawing commands that actually produce visible information? That being the case, isn't the relevant data already being hooked when the pixmap gets copied to the window using XCopyArea()? -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, 13 Jul 2012 05:18:16 -0500 DRC dcomman...@users.sourceforge.net wrote: Maybe I'm missing something, but isn't the goal to hook only drawing commands that actually produce visible information? That being the case, isn't the relevant data already being hooked when the pixmap gets copied to the window using XCopyArea()? That was the assumption. Something is wrong about that assumption though as we're seeing missing updates. Only speculation as to what so far. Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, 13 Jul 2012 12:50:00 +0200 Pierre Ossman oss...@cendio.se wrote: On Fri, 13 Jul 2012 05:18:16 -0500 DRC dcomman...@users.sourceforge.net wrote: Maybe I'm missing something, but isn't the goal to hook only drawing commands that actually produce visible information? That being the case, isn't the relevant data already being hooked when the pixmap gets copied to the window using XCopyArea()? That was the assumption. Something is wrong about that assumption though as we're seeing missing updates. Only speculation as to what so far. Mystery somewhat clearer. The reason we're missing the update, and why the pixmap hack works, is because the borders are drawn directly on the _screen_ pixmap. That explains why the coordinates still make sense, even though we're hooking a pixmap instead of a window. Not sure what the fix is. Starting to activate hooks even for just the screen pixmap still means we have to audit the code for assumptions about windows. Not terribly appealing. I'll dig around and see if I can find something cleaner... Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, Jun 29, 2012 at 04:00:14PM +0200, Pierre Ossman wrote:
On Fri, 29 Jun 2012 09:17:39 -0400
Brian Hinz bph...@users.sourceforge.net wrote:
r4220 appears to be the cause of bug #3415308. When I revert the change
made in r4220 fluxbox and other apps that exhibit rendering artifacts along
their window decorations all behave properly. I have not seen any problems
with this so far (limited testing), however Adam's commit note states
that Don't
hook pixmaps in vncHooks, it fixes crash after XDrawArc call.. Attached is
the patch I'm using, can someone look over it and verify whether or not
there is still a problem with XDrawArc?
Hooking pixmaps is fundamentally wrong as they are not part of what's
displayed, and will therefore never be directly transferred to the
client.
The fact that your patch fixes something is an indication that we are
overlooking some screen updates somewhere else. Unfortunately it's not
really trivial to figure out what and where. You have to find a simple
test case, and then start following the chain of draw commands and see
where a hook is either missing, or miscalculating the damaged region.
You are right that hooking pixmaps is wrong but if r4220 caused this issue,
it should be treated as regression and reverted.
I dived into vncHooks and xserver sources and attached patch should fix both
screen artefacts and XDrawArc crashes. It reverts r4220 and fixes
vncHooksFillSpans hook which used wrong clip pointer.
Can you please test if it works in your case?
Regards, Adam
--
Adam Tkac, Red Hat, Inc.
diff -up tigervnc-1.1.0/unix/xserver/hw/vnc/vncHooks.cc.rh734424
tigervnc-1.1.0/unix/xserver/hw/vnc/vncHooks.cc
--- tigervnc-1.1.0/unix/xserver/hw/vnc/vncHooks.cc.rh734424 2012-07-12
17:40:32.491230021 +0200
+++ tigervnc-1.1.0/unix/xserver/hw/vnc/vncHooks.cc 2012-07-12
17:58:51.530006884 +0200
@@ -661,7 +661,9 @@ static void vncHooksValidateGC(GCPtr pGC
(*pGC-funcs-ValidateGC) (pGC, changes, pDrawable);
u.vncHooksGC-wrappedOps = 0;
- if (pDrawable-type == DRAWABLE_WINDOW ((WindowPtr) pDrawable)-viewable)
{
+ //if (pDrawable-type == DRAWABLE_WINDOW)
+ if (pDrawable-type == DRAWABLE_WINDOW || pDrawable-type == DRAWABLE_PIXMAP)
+ {
u.vncHooksGC-wrappedOps = pGC-ops;
DBGPRINT((stderr,vncHooksValidateGC: wrapped GC ops\n));
}
@@ -741,7 +743,7 @@ static void vncHooksFillSpans(DrawablePt
{
GC_OP_UNWRAPPER(pDrawable, pGC, FillSpans);
- RegionHelper changed(pScreen, ((WindowPtr)pDrawable)-borderClip);
+ RegionHelper changed(pScreen, pGC-pCompositeClip);
(*pGC-ops-FillSpans) (pDrawable, pGC, nInit, pptInit, pwidthInit, fSorted);
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Thu, Jul 12, 2012 at 12:04 PM, Adam Tkac wrote: On Fri, Jun 29, 2012 at 04:00:14PM +0200, Pierre Ossman wrote: On Fri, 29 Jun 2012 09:17:39 -0400 Brian Hinz wrote: r4220 appears to be the cause of bug #3415308. When I revert the change made in r4220 fluxbox and other apps that exhibit rendering artifacts along their window decorations all behave properly. I have not seen any problems with this so far (limited testing), however Adam's commit note states that Don't hook pixmaps in vncHooks, it fixes crash after XDrawArc call.. Attached is the patch I'm using, can someone look over it and verify whether or not there is still a problem with XDrawArc? Hooking pixmaps is fundamentally wrong as they are not part of what's displayed, and will therefore never be directly transferred to the client. The fact that your patch fixes something is an indication that we are overlooking some screen updates somewhere else. Unfortunately it's not really trivial to figure out what and where. You have to find a simple test case, and then start following the chain of draw commands and see where a hook is either missing, or miscalculating the damaged region. You are right that hooking pixmaps is wrong but if r4220 caused this issue, it should be treated as regression and reverted. I dived into vncHooks and xserver sources and attached patch should fix both screen artefacts and XDrawArc crashes. It reverts r4220 and fixes vncHooksFillSpans hook which used wrong clip pointer. I think that I see what the fundamental problem is. miPaintWindow in mi/miexpose.c draws the border to the pixmap (because what == PW_BORDER) and then calls ValidateGC. Because it's a pixmap, we don't hook and therefore desktop-add_changed is not called. I think that all of the drawing is correct, it's just not notifying the server that those areas are damaged. I checked all of the clip regions and as far as I can tell they are all correct (that is, the composite clip seems to include the border areas). It's ugly, but one workaround might be to go ahead and hook pixmaps but put an if statement into each hook function to basically bypass the hooks unless (as in the case of PolyFillRect) there was some reason not to. -brian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, 10 Jul 2012 23:41:55 -0400 Brian Hinz bph...@users.sourceforge.net wrote: I've been trying to trace back the source of the PolyFillRects and they definitely come from miHandleValidateExposures in mi/miwindow.c - miPaintWindow in mi/miexpose.c and then the PolyFillRect hook should be called from miPaintWindow. At the point where miPaintWindow should be calling the hook function, the rectangles are correct, but then the xRectangle that gets evaluated in vncHooksPolyFillRect is not the same. Sometimes vncHooksPolyFillRect is not even called after miPaintWindow at all... I'm utterly confused as to how this could be happening, any chance someone can take a peek and see if you can make some sense of it? I'm messing around in Xvnc for other reasons, so maybe I can have a quick look. Got a good test case? Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Wed, Jul 11, 2012 at 4:11 AM, Pierre Ossman wrote:
On Tue, 10 Jul 2012 23:41:55 -0400
Brian Hinz wrote:
I've been trying to trace back the source of the PolyFillRects and they
definitely come from miHandleValidateExposures in mi/miwindow.c -
miPaintWindow in mi/miexpose.c and then the PolyFillRect hook should be
called from miPaintWindow. At the point where miPaintWindow should be
calling the hook function, the rectangles are correct, but then the
xRectangle that gets evaluated in vncHooksPolyFillRect is not the same.
Sometimes vncHooksPolyFillRect is not even called after miPaintWindow at
all... I'm utterly confused as to how this could be happening, any
chance
someone can take a peek and see if you can make some sense of it?
I'm messing around in Xvnc for other reasons, so maybe I can have a
quick look. Got a good test case?
Hi Pierre,
Just starting the fluxbox window manager makes for a decent test case
because there are a couple of rectangles that are always drawn at the same
spot (for a given resolution). I've attached a screen shot of the desktop
showing the two rectangles around and inside the task bar at the bottom of
the screen (these borders aren't drawn initially, I had to request a screen
refresh from the client). Here is the output of some debug statements that
I put into miHandleValidateExposures, miPaintWindow, and
vncHooksPolyFillRect that I expected to show the progression of the borders:
...
miPaintWindow called from miHandleValidateExposures:
RegionNumRects(val-after.borderExposed) = 4
(val-after.borderExposed)-extents.x1 = 436
(val-after.borderExposed)-extents.y1 = 575
(val-after.borderExposed)-extents.x2 = 940
(val-after.borderExposed)-extents.y2 = 598
miPaintWindow:
RegionNumRects(prgn) = 4
prgn-extents.x1 = 436
prgn-extents.y1 = 575
prgn-extents.x2 = 940
prgn-extents.y2 = 598
vncHooksPolyFillRect:
nrects = 1
rects-x = 0
rects-y = 0
rects-width = 502
rects-height = 21
vncHooksPolyFillRect:
nrects = 1
regRects[0].x = 574
regRects[0].y = 576
regRects[0].width = 0
regRects[0].height = 0
miPaintWindow:
RegionNumRects(prgn) = 0
prgn-extents.x1 = 0
prgn-extents.y1 = 0
prgn-extents.x2 = 0
prgn-extents.y2 = 0
miPaintWindow:
...
This particular output appears to be the inner border on my 1280x600
desktop. The two print statements from vncHooksPolyFillRect are both from
the same iteration. Notice how nrects goes from 4 to 1 and the x, y, width
height change into something more like a line? It almost seems like the
data that's being passed to vncHooksPolyFillRect is the kind of data that
should be destined for Polylines...?
Another thing that I've noticed is that a lot of 0 rect regions are ending
up in miPaintWindow. I don't think they make it through to
vncHooksPolyFillRect because of this statement in miPaintWindow:
...
prect = malloc(RegionNumRects(prgn) * sizeof(xRectangle));
if (!prect)
return;
...
vncHooksFillRect seems to be equipped to deal with this case, however even
if that were to happen the hook returns without adding anything to the
server's changed region (of course what the heck should be added anyway?):
...
if (nrects == 0) {
(*pGC-ops-PolyFillRect) (pDrawable, pGC, nrects, rects);
return;
}
...
Hopefully this isn't too much random data, I'm just trying to wrap up as
much as I can all at once. Thanks for taking a
look.
-brian
attachment: fluxbox-windowborders.png--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, Jul 6, 2012 at 9:45 AM, Brian Hinz wrote: On Wed, Jul 4, 2012 at 3:35 AM, Pierre Ossman wrote: On Tue, 3 Jul 2012 16:42:11 -0400 Brian Hinz wrote: Hi Pierre, I'm not sure if this means anything to you, but I found that if I set 'AlwaysSetDeferUpdateTimer=1' then outline boxes are partially drawn and damaged regions are sometimes partially redrawn. The location and extent to which the lines are drawn is seemingly random, but it's never complete (except when a screen refresh is requested). Also, the value of 'DeferUpdate' does not seem to make any difference. Any thoughts? Nothing obvious comes to mind, no. I'm not sure what your test case is, but if you're dragging windows around then having a deferred update might work as a workaround for the problem. The server would in that case really be registering multiple changes of the working parts of the border, which then happen to overlap the areas that are overlooked. One idea is to ask on the X list for suggestions on where in the Xorg code that the server does the implicit rendering. That might make it easier to trace where we should have hooks. I stumbled onto this bug report for Xquartz: http://xquartz.macosforge.org/trac/ticket/290 The ticket is marked as closed but if you follow the thread, the Xorg bug is re-opened: https://bugs.freedesktop.org/show_bug.cgi?id=26124 It certainly appears to be the same or very similar problem. After applying the patches from http://cgit.freedesktop.org/xorg/xserver/commit/?id=2d6a8f668342a5190cdf43b5d385f592d10f5900 there does seem to be some improvement. Damaged regions are generally repaired correctly if the window is dragged around or over top of another area without borders (even if that area is not obscured by the window), but the initial drawing is not performed and if a window is redrawn (or shaded up down) the borders are cleared. I've appended a comment to the xorg bug as well. -brian I've been trying to trace back the source of the PolyFillRects and they definitely come from miHandleValidateExposures in mi/miwindow.c - miPaintWindow in mi/miexpose.c and then the PolyFillRect hook should be called from miPaintWindow. At the point where miPaintWindow should be calling the hook function, the rectangles are correct, but then the xRectangle that gets evaluated in vncHooksPolyFillRect is not the same. Sometimes vncHooksPolyFillRect is not even called after miPaintWindow at all... I'm utterly confused as to how this could be happening, any chance someone can take a peek and see if you can make some sense of it? Thanks, -brian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, 3 Jul 2012 16:42:11 -0400 Brian Hinz bph...@users.sourceforge.net wrote: Hi Pierre, I'm not sure if this means anything to you, but I found that if I set 'AlwaysSetDeferUpdateTimer=1' then outline boxes are partially drawn and damaged regions are sometimes partially redrawn. The location and extent to which the lines are drawn is seemingly random, but it's never complete (except when a screen refresh is requested). Also, the value of 'DeferUpdate' does not seem to make any difference. Any thoughts? Nothing obvious comes to mind, no. I'm not sure what your test case is, but if you're dragging windows around then having a deferred update might work as a workaround for the problem. The server would in that case really be registering multiple changes of the working parts of the border, which then happen to overlap the areas that are overlooked. One idea is to ask on the X list for suggestions on where in the Xorg code that the server does the implicit rendering. That might make it easier to trace where we should have hooks. Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Mon, Jul 2, 2012 at 9:55 AM, Pierre Ossman wrote: On Mon, 2 Jul 2012 08:31:41 -0400 Brian Hinz wrote: I put some debug print statements in and it seems to either in vncHooksPolyRectangle or vncHooksPolyFillRect (oddly, RHEL5 seems to call PolyRectangle and Fedora 16 calls PolyFillRect). It would make sense for it to be PolyRectangle since it's clearly the outlines of 1px wide boxes that are not being drawn, but after comparing to the TurboVNC (which doesn't exhibit the same problem) source code I have have not been able to figure out what's going on. I noticed that every once in a while interior box outlines are drawn, but never the outermost outline for a given window or box. Requesting a screen refresh from the client causes everything to be drawn correctly until some part of the outline is damaged and then that area is not re-drawn correctly unless another refresh is requested. This particular thing might be a bit more difficult to find since it is implicitly drawn by the X server, rather than as a result of a client request. Perhaps there are special draw functions, or that the hooks don't work properly for these implicit things. Hi Pierre, I'm not sure if this means anything to you, but I found that if I set 'AlwaysSetDeferUpdateTimer=1' then outline boxes are partially drawn and damaged regions are sometimes partially redrawn. The location and extent to which the lines are drawn is seemingly random, but it's never complete (except when a screen refresh is requested). Also, the value of 'DeferUpdate' does not seem to make any difference. Any thoughts? Thanks, -brian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On 7/3/12 3:42 PM, Brian Hinz wrote: I'm not sure if this means anything to you, but I found that if I set 'AlwaysSetDeferUpdateTimer=1' then outline boxes are partially drawn and damaged regions are sometimes partially redrawn. The location and extent to which the lines are drawn is seemingly random, but it's never complete (except when a screen refresh is requested). Also, the value of 'DeferUpdate' does not seem to make any difference. Any thoughts? Is there any chance that this is a viewer issue? When you mentioned partial updates, that brought to mind 3531487. Just a shot in the dark. https://sourceforge.net/tracker/?func=detailaid=3531487group_id=254363atid=1126848 -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Tue, Jul 3, 2012 at 5:32 PM, DRC wrote: On 7/3/12 3:42 PM, Brian Hinz wrote: I'm not sure if this means anything to you, but I found that if I set 'AlwaysSetDeferUpdateTimer=1' then outline boxes are partially drawn and damaged regions are sometimes partially redrawn. The location and extent to which the lines are drawn is seemingly random, but it's never complete (except when a screen refresh is requested). Also, the value of 'DeferUpdate' does not seem to make any difference. Any thoughts? Is there any chance that this is a viewer issue? When you mentioned partial updates, that brought to mind 3531487. Just a shot in the dark. https://sourceforge.net/tracker/?func=detailaid=3531487group_id=254363atid=1126848 I don't think so. The TurboVNC viewer + TigerVNC server exhibits the problem, but the TurboVNC server + TigerVNC viewer does not. -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, Jun 29, 2012 at 10:00 AM, Pierre Ossman oss...@cendio.se wrote: The fact that your patch fixes something is an indication that we are overlooking some screen updates somewhere else. Unfortunately it's not really trivial to figure out what and where. You have to find a simple test case, and then start following the chain of draw commands and see where a hook is either missing, or miscalculating the damaged region. I put some debug print statements in and it seems to either in vncHooksPolyRectangle or vncHooksPolyFillRect (oddly, RHEL5 seems to call PolyRectangle and Fedora 16 calls PolyFillRect). It would make sense for it to be PolyRectangle since it's clearly the outlines of 1px wide boxes that are not being drawn, but after comparing to the TurboVNC (which doesn't exhibit the same problem) source code I have have not been able to figure out what's going on. I noticed that every once in a while interior box outlines are drawn, but never the outermost outline for a given window or box. Requesting a screen refresh from the client causes everything to be drawn correctly until some part of the outline is damaged and then that area is not re-drawn correctly unless another refresh is requested. -brian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
Re: [Tigervnc-devel] [PATCH] revert r4220 to fix bug #3415308
On Fri, 29 Jun 2012 09:17:39 -0400 Brian Hinz bph...@users.sourceforge.net wrote: r4220 appears to be the cause of bug #3415308. When I revert the change made in r4220 fluxbox and other apps that exhibit rendering artifacts along their window decorations all behave properly. I have not seen any problems with this so far (limited testing), however Adam's commit note states that Don't hook pixmaps in vncHooks, it fixes crash after XDrawArc call.. Attached is the patch I'm using, can someone look over it and verify whether or not there is still a problem with XDrawArc? Hooking pixmaps is fundamentally wrong as they are not part of what's displayed, and will therefore never be directly transferred to the client. The fact that your patch fixes something is an indication that we are overlooking some screen updates somewhere else. Unfortunately it's not really trivial to figure out what and where. You have to find a simple test case, and then start following the chain of draw commands and see where a hook is either missing, or miscalculating the damaged region. Rgds -- Pierre Ossman Software Development Cendio AB http://cendio.com Teknikringen 8 http://twitter.com/ThinLinc 583 30 Linköpinghttp://facebook.com/ThinLinc Phone: +46-13-214600http://plus.google.com/112509906846170010689 A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/___ Tigervnc-devel mailing list Tigervnc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
