[PATCH 2/3] libcamera: v4l2_videodevice: Report offset in dequeueBuffer

[Laurent Pinchart]

On Fri, Mar 28, 2025 at 04:51:12PM -0400, Nicolas Dufresne wrote:
> Le mercredi 26 mars 2025 à 16:51 +0900, Paul Elder a écrit :
> > Fill in the offset field of the frame metadata so that it the
> > application can read it.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp
> > b/src/libcamera/v4l2_videodevice.cpp
> > index f5b3fa09d9a0..5d163f7fd853 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1957,10 +1957,13 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >  		 * The number of planes in the frame buffer and in the
> >  		 * V4L2 buffer is guaranteed to be equal at this point.
> >  		 */
> > -		for (unsigned int i = 0; i < numV4l2Planes; ++i)
> > +		for (unsigned int i = 0; i < numV4l2Planes; ++i) {
> >  			metadata.planes()[i].bytesused = planes[i].bytesused;
> > +			metadata.planes()[i].offset = planes[i].data_offset;
> 
> In V4L2, data_offset is included in the total bytesused. Once you hit a
> non-zero offset, you will endup with byteused possibly bigger then
> FrameBuffer.length, which to my reading contradicts the doc.
> 
> https://www.linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/buffer.html?highlight=data_offset#struct-v4l2-plane
> 
> > +		}
> >  	} else {
> >  		metadata.planes()[0].bytesused = buf.bytesused;
> > +		metadata.planes()[0].offset = buf.m.offset;
> 
> Should be:
> 
> +		metadata.planes()[0].offset = 0;
> 
> There is no support for data_offset in the original v4l2_buffer API.
> This offset match planes[i].mem_offset and is only useful for CPU
> access using mmap() and the video node FD.
> 
> I think some introspection is needed, since this offset was expected to
> be in the framebuffer, which apparently needs to be constant. Once
> these issue resovled, do you mind fixing GStreamer code to use the
> right one, it reads like this:
> 
> 		GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.get(),
> 							plane.offset + plane.length,
> 							GST_FD_MEMORY_FLAG_DONT_CLOSE);
> 		gst_memory_resize(mem, plane.offset, plane.length);
> 
> Plane.offset is never set (always zero atm), but if its constant, its
> the wrong one, this code should use the metadata once (the variable
> one). You will also notice the semantic difference, we assume length
> does not include the offset, this might need adjustment.

Outside of codec drivers, data_offset is a bit of a problematic API. The
issue with reporting a dynamic offset is that it make it impossible to
use fences and queue the buffer with a consumer before we know the
offset value. The concern is of course theoretical, given that V4L2
doesn't have fence support, but I would really not to see widespread
data_offset usage before we figure this issue out.

Not counting the vivid test driver and codec drivers, data_offset is
only used by the omap3isp driver, for entirely historical reasons (we
didn't know what we were doing at the time). I'd like to understand your
use cases better in order to determine the best way forward.

Note that there is a different, and in my opinion entirely valid, use of
offsets, which is indicating where a plane is located within a dmabuf.
This isn't supported by V4L2 at the moment (there are old patches on the
list for this, but they died out), but is I think a useful and needed
feature (DRM supports it). The FrameBuffer::Plane::offset field is meant
for this.

> >  	}
> >  
> >  	return buffer;

-- 
Regards,

Laurent Pinchart