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?
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>
Btw, now spice-server on rhel6 looks good while virt-viewer crashed ;)
https://bugs.freedesktop.org/show_bug.cgi?id=94318
>
> 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