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

[Frediano Ziglio]

> 
> On Tue, Jan 24, 2017 at 07:35:25AM -0500, Frediano Ziglio wrote:
> > > > +         * 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.
> 
> When I see
> int some_int;
> foo(&some_int);
> it usually means that foo() is going to modify some_int, it rarely means
> that foo() expects an array of int, hence the suggestions to make the
> array more explicit. Feel free to ignore it.
> 
> Christophe
> 

I think the most confusing is GStreamer documentation.
>From https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideometa.html#gst-buffer-add-video-meta-full
gst_buffer_add_video_meta_full() is declared as

GstVideoMeta *
gst_buffer_add_video_meta_full (GstBuffer *buffer,
                                GstVideoFrameFlags flags,
                                GstVideoFormat format,
                                guint width,
                                guint height,
                                guint n_planes,
                                gsize offset[GST_VIDEO_MAX_PLANES],
                                gint stride[GST_VIDEO_MAX_PLANES]);

usually declaring an array like this means that
- you should pass an array of GST_VIDEO_MAX_PLANES elements;
- gst_buffer_add_video_meta_full can modify the values of the
  arrays items.
Both are false, would be better as

GstVideoMeta *
gst_buffer_add_video_meta_full (GstBuffer *buffer,
                                GstVideoFrameFlags flags,
                                GstVideoFormat format,
                                guint width,
                                guint height,
                                guint n_planes,
                                const gsize *offsets,
                                const gint *strides);

documenting that offsets and strides should contains n_planes
values.

Frediano