[libcamera-devel] [RFC PATCH 07/10] gstreamer: gstlibcameraallocator: Use offset in creating a buffer

[Nicolas Dufresne]

Le mercredi 18 août 2021 à 03:51 +0300, Laurent Pinchart a écrit :
> Hi Hiro,
> 
> (CC'ing Nicolas)
> 
> Thank you for the patch.
> 
> On Mon, Aug 16, 2021 at 01:31:35PM +0900, Hirokazu Honda wrote:
> > The plane length is the length of the plane size. The buffer length
> > to be allocated for a plane is the offset and the length of
> > FrameBuffer::Plane.
> > 
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/gstreamer/gstlibcameraallocator.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > index 7bd8ba2d..a3ffec5b 100644
> > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -52,7 +52,8 @@ FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> >  	  outstandingPlanes_(0)
> >  {
> >  	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > -		GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.fd(), plane.length,
> > +		GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.fd(),
> > +							plane.offset + plane.length,
> 
> I'm not familiar enough with GStreamer to know the details, but it seems
> that there's something problematic here. There's no offset passed to
> gst_fd_allocator_alloc(), so the GstMemory object will "point" to the
> beginning of the dmabuf. When it will then be used, I don't think it
> will access the plane memory at plane.offset.

Indeed, the offset needs to be set using gst_memory_resize() call. Please check
correct the libcamera code, in V4L2, length includes the data offset, so careful
review of the v4l2 code seems needed if libcamera uses the opposite semantic.

For the reference:

https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/blob/master/sys/v4l2/gstv4l2allocator.c#L929

> 
> >  							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> >  		gst_mini_object_set_qdata(GST_MINI_OBJECT(mem), getQuark(), this, nullptr);
> >  		GST_MINI_OBJECT(mem)->dispose = gst_libcamera_allocator_release;
>