[Spice-devel] [spice v13 03/29] server: Add a GStreamer 1.0 MJPEG video encoder and use it by default

[Francois Gouget]

On Wed, 25 May 2016, Francois Gouget wrote:

> On Tue, 24 May 2016, Francois Gouget wrote:
> [...]
> > Now if the content of SpiceCopy cannot be trusted, the GStreamer encoder 
> > is the wrong place to check for issues : there are at least three places 
> > where the SpiceCopy bitmap can be used: the MJPEG encoder, the GStreamer 
> > encoder and the regular screen update code (which may or may not involve 
> > the JPEG encoder) [1].
> > 
> > Duplicating checks all over the place can only lead to maintenance 
> > issues and incomplete checks. So validating the SpiceCopy structure 
> > should be done before it is sent down one or the other path.
> 
> To nuance this, in the non-streaming case it seems like the SpiceCopy 
> structure is sent to the client without being interpreted. That is the 
> image buffer is sent but it does not need to be consistent with its x 
> and y dimensions, nor with the src_area struct. If the same is true for 
> most other structures, then it would make sense to only perform these 
> consistency checks on the server for the streaming code, but before the 
> individual video encoders.

I take that back, partially at least. The x and y dimensions on the 
SpiceImage must be correct otherwise the server crashes in 
dcc_compress_image().

Here's a test for this:

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 0dafbef..5eddaa1 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -681,6 +681,7 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
     if (!red->src_bitmap) {
         return 1;
     }
+    red->src_bitmap->u.bitmap.y += 10;
     red_get_rect_ptr(&red->src_area, &qxl->src_area);
     red->rop_descriptor  = qxl->rop_descriptor;
     red->scale_mode      = qxl->scale_mode;

So various aspects of SpiceImage should be validated in red_get_image(), 
while validating the consistency of the SpiceCopy's src_area field with 
the src_bitmap one can be restricted to the streaming case in 
red_marshall_stream_data(). However I'm not entirely convinced delaying 
that check that much is a good idea.


-- 
Francois Gouget <fgouget at codeweavers.com>