bilboed mentioned performance implications, so this patch adds a 'remote' property to enable that change, so by default XGetImage will still be used:

http://git.collabora.co.uk/?p=user/vincent/gst-plugins-good;a=commitdiff;h=100e27d70d220c384470771ac2491a108e495dd3

First of all, attaching the patches to the bug makes reviewing much easier :) You can use git bz for that


I'm not sure these off-by-one changes are correct.

> if (rects[i].x + rects[i].width - 1 < ximagesrc->startx

So, if we have a rect x=0,width=2 and startx=2, there would be no overlap. The rect would be on the pixel 0 and the pixel 1 but not on the pixel 2, where the ximagesrc window starts. Anything I'm missing here?   (same of the y/height/starty change)

But OTOH this looks wrong too

> rects[i].x > ximagesrc->endx

Shouldn't this be a >= instead?

And the same questions for the width/height assignment below too :)



Why is XGetSubImage slower than XGetImage btw?

> > if (rects[i].x + rects[i].width - 1 < ximagesrc->startx
> 
> So, if we have a rect x=0,width=2 and startx=2, there would be no overlap. The
> rect would be on the pixel 0 and the pixel 1 but not on the pixel 2, where the
> ximagesrc window starts. Anything I'm missing here? 

That's right, and the test will yield (0+2-1 < 2), which is true, so the code earlies out. Unless I'm missing your point, the case you mention seems to do as it should.

XGetSubImage makes an extra copy (the code I've seen calls XGetImage, then copied part of the buffer it returns). I've no idea how significant the speed hit is.

Same for the second example here: if the rect and ximagesrc window share their boundary pixel, then they overlap (for one single pixel), so we musn't reject.

Ah, yes:
The name 'endx' refers to the last pixel (eg, inclusive), from reading the description of the property, rather than the STL-like "1 after". Which might explain the confusion ?
Though this would be irrelevant for the 'start' properties. But would explain the off by ones in calculating width/height.

(In reply to comment #3)
> > > if (rects[i].x + rects[i].width - 1 < ximagesrc->startx
> > 
> > So, if we have a rect x=0,width=2 and startx=2, there would be no overlap. The
> > rect would be on the pixel 0 and the pixel 1 but not on the pixel 2, where the
> > ximagesrc window starts. Anything I'm missing here? 
> 
> That's right, and the test will yield (0+2-1 < 2), which is true, so the code
> earlies out. Unless I'm missing your point, the case you mention seems to do as
> it should.

Hm, yes. You're right. I must've been confused, my comment explained why your patch is correct :)

I'll push your changes now

commit 7ac31b3d226feba3d46e48679a8097fb30e86f04
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Nov 29 12:36:06 2010 +0000

    ximagesrc: change from XGetImage to XGetSubImage dependant on a property
    
    ximagesrc: change from XGetImage to XGetSubImage dependant on a property
     to avoid unnecessary performance hits by default.

commit ac872f61cf8679f0e1f07c6fe8564e25e9a5f352
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Sun Nov 28 16:04:35 2010 +0000

    ximagesrc: use XGetSubImage instead of XGetImage, works with remote X
    
    ximagesrc: use XGetSubImage instead of XGetImage, works with remote X
     (on my setup anyway...)

commit fbd9581f87570df1b55c11eda9c84c992a7e783f
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Sat Nov 27 17:15:32 2010 +0000

    ximagesrc: fix various width/height calculations being off by one,
    
    ximagesrc: fix various width/height calculations being off by one,
     and make it so a single pixel width/height can be captured (except
     the top left one, as 0,0,0,0 is reserved for full screen as per
     the property comments).