[Spice-devel] [spice v15 01/21] mjpeg: Use src_area as the authoritative source for the frame dimensions

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160609/4421069b/attachment.sig>