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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]