[Spice-devel] [spice v15 01/21] mjpeg: Use src_area as the authoritative source for the frame dimensions

[Francois Gouget]

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 at codeweavers.com>
> > ---
> >  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 at codeweavers.com>