[Spice-devel] [PATCH v7 2/3] gstreamer: Avoid memory copy if strides are different

[Frediano Ziglio]

> 
> On Tue, Jan 24, 2017 at 11:50:04AM +0000, Frediano Ziglio wrote:
> > If bitmap stride and stream stride are different copy was used.
> > Using GStreamer 1.0 you can avoid the copy setting correctly
> > image offset and stride.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/gstreamer-encoder.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index ba81377..35573bd 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -1338,8 +1338,8 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
> >                            gpointer bitmap_opaque)
> >  {
> >      uint32_t height = src->bottom - src->top;
> > -    uint32_t stream_stride = (src->right - src->left) *
> > encoder->format->bpp / 8;
> > -    uint32_t len = stream_stride * height;
> > +    uint32_t len;
> > +    uint32_t chunk_index = 0;
> >      GstBuffer *buffer = gst_buffer_new();
> >      /* TODO Use GST_MAP_INFO_INIT once GStreamer 1.4.5 is no longer
> >      relevant */
> >      GstMapInfo map = { .memory = NULL };
> > @@ -1350,6 +1350,10 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
> >      uint32_t skip_lines = top_down ? src->top : bitmap->y - (src->bottom -
> >      0);
> >      uint32_t chunk_offset = bitmap->stride * skip_lines;
> >  
> > +#ifndef DO_ZERO_COPY
> > +    uint32_t stream_stride = (src->right - src->left) *
> > encoder->format->bpp / 8;
> > +
> > +    len = stream_stride * height;
> >      if (stream_stride != bitmap->stride) {
> >          /* We have to do a line-by-line copy because for each we have to
> >           * leave out pixels on the left or right.
> > @@ -1365,9 +1369,19 @@ static int push_raw_frame(SpiceGstEncoder *encoder,
> >              return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> >          }
> >      } else {
> > +#else
> > +    {
> > +        /* using GStreamer 1.0 we can avoid to cropping the image if the
> 
> I think something like
> "using GStreamer 1.0, we can avoid cropping the image by simply passing
> the appropriate offset and stride as metadata" would be easier to read.
> 

ok

> > +         * stride is different simply passing appropriate offset and
> > stride */
> > +        gsize offset = src->left * encoder->format->bpp / 8;
> > +        gint stride = bitmap->stride;
> > +        gst_buffer_add_video_meta_full(buffer, GST_VIDEO_FRAME_FLAG_NONE,
> > +                                       encoder->format->gst_format,
> > bitmap->x, bitmap->y,
> > +                                       1, &offset, &stride);
> 
> and here, I'd go with something inbetween the initial patch and my first
> suggestion, ie
> gsize offset[] = { src->left * encoder->format->bpp / 8 };
> gint stride[] = { bitmap->stride };
> Sorry for that :-/
> 

I don't see this as much as an improvement, unless you don't
know much C language.
And at least I'd use plural for the arrays.

> As far as I'm concerned
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> (ie no need to send a new version if you only make these changes). I'd
> still be interested in Francois's feedback on these, so maybe wait until
> end of the week before pushing :)
> 
> Christophe
> 

Frediano