[Spice-devel] [PATCH spice 3/11] server: Refactor the Spice server video encoding so alternative implementations can be added.

[Francois Gouget]

On Tue, 19 May 2015, Marc-André Lureau wrote:
[...]
> > +struct VideoEncoder {
> > +    /* Releases the video encoder's resources */
> > +    void (*destroy)(VIDEO_ENCODER_T *encoder);
> > +
> > +    /* Compresses the specified src image area into the outbuf buffer.
> > +     *
> > +     * @encoder:   The video encoder.
> > +     * @bitmap:    The Spice screen.
> > +     * @width:     The width of the Spice screen.
> > +     * @height:    The heigth of the Spice screen.
> >
> 
> It's not the screen, it's the video area. It's not clear to me what 
> would happen if the "src" size doesn't match the width&height.

Ah right. I'll fix the comment. I was confused about that but after 
looking again I must say I'm still unsure as to why we get both src and 
width+height.

My understanding is that this has been introduced to deal with the 
YouTube progress bar:
https://bugzilla.redhat.com/show_bug.cgi?id=813826

To sum up, the size of the video area detected by the Spice server 
changes when the progress bar pops up or down. As a result some of the 
'video frames' it gets don't match the size it detected when it created 
the video stream.

 * It used to be that this caused such frames to go through the regular 
   update mechanism instead of through the video stream. This is still 
   the case with the spicec client for instance and results in the video 
   seeming to jump back and forth because the video stream is delayed 
   due to buffering while the regular path is not.

   Looking at the impact on src & height, either the size of the video 
   frame, aka src, matches the stream size and then so do width and 
   height (lines 9 & 10 below), or it does not and then we don't go 
   through the video encoder (line 6 below).


 * Now clients can advertise support for changing stream sizes with the 
   SIZED_STREAM capability. This does not totally fix the back and forth 
   issue but restricts it to the progress bar area as it comes in and 
   out of the video frame.

   In that case width & height are either computed from the actual video 
   frame size, aka src, (lines 3 & 4 below), or default to the initial 
   stream size when the video frame size matches (lines 9 & 10 below).

From red_marshall_stream_data():
 1 if (drawable->sized_stream) {
 2   if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
 3     width = src_rect->right - src_rect->left;
 4     height = src_rect->bottom - src_rect->top;
 5   } else {
 6     return FALSE;
 7   }
 8 } else {
 9   width = stream->width;
10   height = stream->height;
11 }

So in the end width and height *always* match src. So they're redundant, 
unless some larger purpose is planned for them.

Now must say having a continuously changing stream size is quite 
annoying in the context of GStreamer:
 - At a minimum it forces a reconfiguration of the pipeline which, 
   depending on the encoder, may require rebuilding it. Either cases 
   seem to cause a reset of the GStreamer's encoder bitrate control 
   algorithm which tends to cause it to overshoot and may cause frame 
   drops.
 - Ultimately the GStreamer video encoder should support inter-frame 
   compression (with VP8 or H264 for instance) and a changing stream 
   size will also cause some disruption in this context.

Things can still work as is so there's not hurry but maybe a better 
solution can be devised in time.


For instance it may be better to increase the stream size as the video 
frame size increases but then peg it so it does not shrink again. This 
would solve the remaining back and forth issue with the YouTube progress 
bar, and limit the number of resizes the video encoders have to go 
through. It supposes that regular frame updates know to ignore changes 
happening inside the stream area even if they don't go through the 
regular 'video frame' update path but I have no idea if that's easy to 
do.

Alternatively one could split oversize video frames in two: the stream 
area which would go through the video encoder, and the remainder which 
would go through the regular path. This would also solve the remaining 
back and forth issue with the YouTube progress bar and would even be 
compatible with non SIZED_STREAM capable clients.

-- 
Francois Gouget <fgouget at codeweavers.com>