Re: [PATCH 1/2] server: Remove the width and height parameters of encode_frame()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Sat, Feb 27, 2016 at 01:53:16PM -0500, Frediano Ziglio wrote:
> > Hi,
> > 
> > On Fri, Feb 26, 2016 at 10:03:40PM +0100, Victor Toso wrote:
> > > Hi,
> > >
> > > On Wed, Feb 24, 2016 at 06:30:34PM +0100, Francois Gouget wrote:
> > > > They always match the size of the source bitmap area:
> > > > * because for sized frames they are set from the source area,
> > > > * and because otherwise the initial stream's size is that of regular
> > > >   frames by definition.
> > > > Note also that we do not lose any flexibility since the source area
> > > > parameter can be used to encode a specific bitmap area.
> > > >
> > > > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  server/dcc-send.c      | 24 +++++++-----------------
> > > >  server/mjpeg-encoder.c |  8 +++++---
> > > >  server/mjpeg-encoder.h |  2 +-
> > > >  3 files changed, 13 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > > > index 4180cc1..e280383 100644
> > > > --- a/server/dcc-send.c
> > > > +++ b/server/dcc-send.c
> > > > @@ -1658,7 +1658,6 @@ static int
> > > > red_marshall_stream_data(RedChannelClient *rcc,
> > > >      SpiceImage *image;
> > > >      uint32_t frame_mm_time;
> > > >      int n;
> > > > -    int width, height;
> > > >      int ret;
> > > >
> > > >      if (!stream) {
> > > > @@ -1673,21 +1672,13 @@ static int
> > > > red_marshall_stream_data(RedChannelClient *rcc,
> > > >          return FALSE;
> > > >      }
> > > >
> > > > -    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;
> > > This.
> > >
> > > I think it makes sense and could actually be the proper fix for 1274575
> > 
> > Well, 'proper' fix would be not changing the stream-size if I change the
> > transparency of video player, right?
> > 
> 
> For this I would have a question. Why a transparent video create a copy
> (which is not transparent) ?

I did get what you mean here, are you referring to
drawable->red_drawable->u.copy.src_area ?

> 
> > I have backported this patch to rhel6 (spice 0.12.4 + patches) and it
> > seems much better solution than the proposal patch that I had mainly
> > because, now, does not really matter if the sized stream is correct or
> > not but jpeg would always use the correct parameters in the encoder.
> > 
> > I did not try this upstream yet but so far, patch looks good to me.
> > Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
> > 
> 
> After Francois comments I think it's initial patch is fine (and I would ack).
> I would however add some comment (like the fact that encode_frame is using
> these sizes so it would be an error if the frame was created with different
> size).
>
> However if the bug happens it means that some other code does not set
> sized_stream but set stream even if the size/position is different.
> Possibly we are sending the image as SPICE_MSG_DISPLAY_STREAM_DATA (which
> does not specify sizes or position) with wrong data. An additional check
> to see if the size/position are correct (and switch to
> SPICE_MSG_DISPLAY_STREAM_DATA_SIZED or refuse to compress) would possibly
> prevent some glitches...

With and without those two patches, following bug happens:
https://bugs.freedesktop.org/show_bug.cgi?id=94372

I tested this a bit and did not triggered virt-viewer crash mentioned
before (https://bugs.freedesktop.org/show_bug.cgi?id=94318) but that was
after backporting this patch.

As I don't see any new issues and also due the fact that stream-video is
off by default, I'd ack this two patches

Acked-by: Victor Toso <victortoso@xxxxxxxxxx>

>
> > Btw, now spice-server on rhel6 looks good while virt-viewer crashed ;)
> > https://bugs.freedesktop.org/show_bug.cgi?id=94318
> > 
> 
> ... or perhaps this bug!
> 
> Frediano
> 
> > >
> > > You can see on that mjpeg_encoder_encode_frame() uses this width and
> > > heigh for jpeg_start_compress() but on encode_frame() it uses the
> > > drawable->red_drawable->u.copy.src_area directly.
> > >
> > > For some reason, it was entering in the else below and using
> > > stream->width and stream->height directly.
> > >
> > > I recall doing tests here but I'll try this tomorrow.
> > >
> > > Thank you for this patches, it does simplify quite a bit.
> > >
> > > Cheers,
> > >   toso
> > >
> > > > -        } else {
> > > > -            return FALSE;
> > > > -        }
> > > > -    } else {
> > > > -        width = stream->width;
> > > > -        height = stream->height;
> > > > +    if (drawable->sized_stream &&
> > > > +        !red_channel_client_test_remote_cap(rcc,
> > > > SPICE_DISPLAY_CAP_SIZED_STREAM)) {
> > > > +        return FALSE;
> > > >      }
> > > >
> > > >      StreamAgent *agent = &dcc->stream_agents[get_stream_id(display,
> > > >      stream)];
> > > > +    const SpiceRect *src_area =
> > > > &drawable->red_drawable->u.copy.src_area;
> > > >      uint64_t time_now = spice_get_monotonic_time_ns();
> > > >      size_t outbuf_size;
> > > >
> > > > @@ -1707,8 +1698,7 @@ static int
> > > > red_marshall_stream_data(RedChannelClient *rcc,
> > > >                          reds_get_mm_time();
> > > >      outbuf_size = dcc->send_data.stream_outbuf_size;
> > > >      ret = mjpeg_encoder_encode_frame(agent->mjpeg_encoder,
> > > > -                                     &image->u.bitmap, width, height,
> > > > -
> > > > &drawable->red_drawable->u.copy.src_area,
> > > > +                                     &image->u.bitmap, src_area,
> > > >                                       stream->top_down, frame_mm_time,
> > > >                                      &dcc->send_data.stream_outbuf,
> > > >                                       &outbuf_size, &n);
> > > > @@ -1747,8 +1737,8 @@ static int
> > > > red_marshall_stream_data(RedChannelClient *rcc,
> > > >          stream_data.base.id = get_stream_id(display, stream);
> > > >          stream_data.base.multi_media_time = frame_mm_time;
> > > >          stream_data.data_size = n;
> > > > -        stream_data.width = width;
> > > > -        stream_data.height = height;
> > > > +        stream_data.width = src_area->right - src_area->left;
> > > > +        stream_data.height = src_area->bottom - src_area->top;
> > > >          stream_data.dest = drawable->red_drawable->bbox;
> > > >  
> > > >          spice_debug("stream %d: sized frame: dest ==> ",
> > > >          stream_data.base.id);
> > > > diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> > > > index 3bd1a04..1086b53 100644
> > > > --- a/server/mjpeg-encoder.c
> > > > +++ b/server/mjpeg-encoder.c
> > > > @@ -925,15 +925,17 @@ static int encode_frame(MJpegEncoder *encoder,
> > > > const SpiceRect *src,
> > > >  }
> > > >  
> > > >  int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
> > > > -                               const SpiceBitmap *bitmap, int width, int
> > > > height,
> > > > +                               const SpiceBitmap *bitmap,
> > > >                                 const SpiceRect *src,
> > > >                                 int top_down, uint32_t frame_mm_time,
> > > >                                 uint8_t **outbuf, size_t *outbuf_size,
> > > >                                 int *data_size)
> > > >  {
> > > >      int ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
> > > > -                                    width, height, outbuf, outbuf_size,
> > > > -                                    frame_mm_time);
> > > > +                                        src->right - src->left,
> > > > +                                        src->bottom - src->top,
> > > > +                                        outbuf, outbuf_size,
> > > > +                                        frame_mm_time);
> > > >      if (ret != MJPEG_ENCODER_FRAME_ENCODE_DONE) {
> > > >          return ret;
> > > >      }
> > > > diff --git a/server/mjpeg-encoder.h b/server/mjpeg-encoder.h
> > > > index 8223a7f..31e7cb3 100644
> > > > --- a/server/mjpeg-encoder.h
> > > > +++ b/server/mjpeg-encoder.h
> > > > @@ -54,7 +54,7 @@ MJpegEncoder *mjpeg_encoder_new(uint64_t
> > > > starting_bit_rate,
> > > >  void mjpeg_encoder_destroy(MJpegEncoder *encoder);
> > > >  
> > > >  int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
> > > > -                               const SpiceBitmap *bitmap, int width, int
> > > > height,
> > > > +                               const SpiceBitmap *bitmap,
> > > >                                 const SpiceRect *src,
> > > >                                 int top_down, uint32_t frame_mm_time,
> > > >                                 uint8_t **outbuf, size_t *outbuf_size,
> > > > --
> > > > 2.7.0
> > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]