| Message ID | 20210816043138.957984-8-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. > 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;
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@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; >
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, 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;
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@chromium.org> --- src/gstreamer/gstlibcameraallocator.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)