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