[Spice-devel] [PATCH 1/2] server: Remove the width and height parameters of encode_frame()
Frediano Ziglio
fziglio at redhat.com
Sat Feb 27 18:53:16 UTC 2016
> 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 at codeweavers.com>
> > > ---
> > > 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 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 at redhat.com>
>
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...
> 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list