Hi, On Tue, May 31, 2016 at 08:48:32PM +0200, Francois Gouget wrote: > There are four parameters to consider: > * The stream's initial width / height. > > * The stream's initial dest_area. > > * The width / height of the RedDrawable.u.copy.src_area. > This value can only be sent to CAP_SIZED_STREAM clients. > > * The RedDrawable.bbox. > This value can only be sent to CAP_SIZED_STREAM clients. > In theory this could be different from the SpiceCopy.src_area in > which case the client will perform the scaling. This could happen if > the X server / QXL driver exposes some xv like scaling capabilities to > the application. > But in practice I have never seen it be different from > SpiceCopy.src_area. I've looked at the linux QXL driver (both kernel userland), and there they seem to be always the same indeed. It's less clear in the Windows driver though, so I don't know if we can trigger some corner cases there. > > > So assuming a !CAP_SIZED_STREAM client, an initial stream size of > 300x200 and a current src_area of 320x220, telling the video encoder to > encode a 300x200 area is wrong. It will lead to a 20 pixel band to the > right and bottom not getting refreshed, resulting in artifacts. Yes, actually CAP_SIZED_STREAM was added in order to fix a bug where artifacts were happening when watching a youtube video (see https://bugzilla.redhat.com/show_bug.cgi?id=813826 ). The important thing here, and which I did not realize initially is that as you say, the !CAP_SIZED_STREAM is broken anyway if the stream size changes, and CAP_SIZED_STREAM was added specifically to address that. So this patch might change the breakage in the !CAP_SIZED_STREAM case on Windows guests, at worse making it more noticeable, and our answer would be to upgrade the client anyway as this can't be fixed with older ones. > It should still be fixed though. I see two ways to do that: > > 1) Add the missing check to is_next_stream_frame(). > Pros: > - The check is done only once no matter how many clients are > connected. > Cons: > - The check is far from where it matters one should just trust > sized_stream. > > 2) Add the missing check to red_marshall_stream_data(). > Pros: > - The check is done in the only place where it matters. > - Drawable.sized_stream becomes useless so it can be removed which > simplifies the code. > Cons: > - The (computationally simple) check gets done for each client (which > 99.9% of the time means once). > > Option 2 is more invasive but I think it will be nicer in the end. Drawable.sized_stream indeed seems nice. However, given the above, I'm fine with not changing anything for now (a check would have been nice when SIZED_STREAM support was introduced, now I guess it does not really matter...). Feel free to explore 2) if you want to, but I'm fine if we keep this patch as is. However, if you explore 2), it's probably better to do it on top of the GStreamer branch as this would add again some delay with reviewing. Totally fine with me if things stay this way and you don't look at 2) :) Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel