[Spice-devel] [client 1/2] streaming: Send a special stream report to signal streaming errors

[Francois Gouget]

On Thu, 1 Sep 2016, Christophe Fergeau wrote:
[...]
> > You'll notice that this patch does not directly check for 
> > create_xxx_decoder() errors. Instead it relies on the previous patchset 
> > [2] deleting broken streams so that the following messages will run into 
> > an unknown stream.
> 
> I can't help but wonder why it's better this way? Is it so that you can
> reuse the stream report code as mentioned below?

Yes, it's simpler this way: the client already has to be able to deal 
with invalid stream ids so this just reuses the existing mechanism.
Do you have a better proposal?


> > Of course this could already happen in case of a malicious server 
> > sending garbage to the client. So this patchset is quite independent 
> > from the previous one.
> > 
> > I don't know what the consequences of receiving an unknown message would 
> > be for the server so I chose to hook into the 
> > display_handle_stream_activate_report() as that's where we get notified 
> > that the server supports the stream reports.
> 
> Have you considered using a dedicated client -> server message for that
> rather than using magic values in a stream report message?

Yes but I don't see the benefit to adding one more message type and the 
unavoidable capability and checks that go with it.

Plus I think the stream report is a natural fit for this: it's meant to 
report dropped frames and if the client cannot handle the stream at all 
then all frames are in effect dropped. And this is exactly what the 
stream report we send claims: "the dropped frame count is maxed out"; 
all your frames are dropped. Setting "the received frame count to zero" 
even matches the case where the video decoder initialization fails since 
no frame has been received yet at that point.

Also the server already needs to validate data sent by the client so 
these values should not cause it any trouble.

-- 
Francois Gouget <fgouget at codeweavers.com>