On Tue, 31 May 2016, Christophe Fergeau wrote:
> On Thu, May 26, 2016 at 05:15:35PM +0200, Francois Gouget wrote:
> > Video frames correspond to QXL_DRAW_COPY operations where the frame area
> > is defined by the SpiceCopy.src_area field.
> >
> > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> > ---
> > server/mjpeg-encoder.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> > index e3646db..7dcea50 100644
> > --- a/server/mjpeg-encoder.c
> > +++ b/server/mjpeg-encoder.c
> > @@ -706,7 +706,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
> > */
> > static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
> > SpiceBitmapFmt format,
> > - int width, int height,
> > + const SpiceRect *src,
> > uint8_t **dest, size_t *dest_len,
> > uint32_t frame_mm_time)
> > {
> > @@ -777,10 +777,12 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
> > return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> > }
> >
> > + encoder->cinfo.image_width = src->right - src->left;
> > + encoder->cinfo.image_height = src->bottom - src->top;
>
> I don't think this is correct in the !SIZED_STREAM case. dcc-send.c has:
>
> if (drawable->sized_stream) {
> if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
> SpiceRect *src_rect = &drawable->red_drawable->u.copy.src_area;
>
> width = src_rect->right - src_rect->left;
> height = src_rect->bottom - src_rect->top;
> } else {
> return FALSE;
> }
> } else {
> width = stream->width;
> height = stream->height;
> }
>
> so if the client has SPICE_DISPLAY_CAP_SIZED_STREAM, we can use src_rect.
> However, in the !CAP_SIZED_STREAM case, we are not using the size of the current bitmap
> during encoding, but rather the initial size of the stream. Ie I'm worried that
> with this simplification, the stream we are sending is not of the right resolution in
> the !CAP_SIZED_STREAM case.
There are four parameters to consider:
* The stream's initial width / height.
* The stream's initial dest_area.
* The width / height of the RedDrawable.u.copy.src_area.
This value can only be sent to CAP_SIZED_STREAM clients.
* The RedDrawable.bbox.
This value can only be sent to CAP_SIZED_STREAM clients.
In theory this could be different from the SpiceCopy.src_area in
which case the client will perform the scaling. This could happen if
the X server / QXL driver exposes some xv like scaling capabilities to
the application.
But in practice I have never seen it be different from
SpiceCopy.src_area.
So assuming a !CAP_SIZED_STREAM client, an initial stream size of
300x200 and a current src_area of 320x220, telling the video encoder to
encode a 300x200 area is wrong. It will lead to a 20 pixel band to the
right and bottom not getting refreshed, resulting in artifacts.
The question is whether that can happen. In both the old and new code
the answer hinges on whether drawable->sized_stream is a reliable
indicator of a frame with a size different from the stream width /
height.
To answer that question it's necessary to figure out how
is_next_stream_frame() determines whether drawable->stream or
drawable->sized_stream will eventually be set in
red_marshall_stream_data().
But first note that only one of the calls to is_next_stream_frame() in
stream_trace_update() and stream_maintenance() matter. The reason is
that the one based on the ItemTrace structure corresponds to the case
where the stream has not been created yet. It's there to validate each
would-be new stream frame until we have seen enough good candidates to
decide to create the stream. Since the stream does not exist yet we can
ignore these calls.
As a result all the calls we care about have container_candidate_allowed
== TRUE. The reason for this is that there is one Stream object but
potentially multiple clients, some of which may have CAP_SIZED_STREAM,
and some of which may not. So is_next_stream_frame() just checks whether
the current frame is acceptable for CAP_SIZED_STREAM clients and it's up
to red_marshall_stream_data() to reject frames that are unsuitable for
!CAP_SIZED_STREAM clients.
So it turns out that for our purposes is_next_stream_frame()
distinguishes between regular and extra large frames only based on
RedDrawable.bbox.
It does not check whether the src_area width / height matches the
initial stream width / height. So in theory red_marshall_stream_data()
could get drawable->sized_stream == NULL in that case. However that
would mean that the src_area and bbox have different sizes which as I
mentioned before does not seem to happen.
It should still be fixed though. I see two ways to do that:
1) Add the missing check to is_next_stream_frame().
Pros:
- The check is done only once no matter how many clients are
connected.
Cons:
- The check is far from where it matters one should just trust
sized_stream.
2) Add the missing check to red_marshall_stream_data().
Pros:
- The check is done in the only place where it matters.
- Drawable.sized_stream becomes useless so it can be removed which
simplifies the code.
Cons:
- The (computationally simple) check gets done for each client (which
99.9% of the time means once).
Option 2 is more invasive but I think it will be nicer in the end.
--
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel