[libcamera-devel] [PATCH v2 00/27] libcamera: Handle fallout of FrameBuffer offset support
David Plowman
david.plowman at raspberrypi.com
Mon Sep 6 19:22:10 CEST 2021
Hi Laurent
On Mon, 6 Sept 2021 at 15:16, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Mon, Sep 06, 2021 at 02:13:58PM +0100, David Plowman wrote:
> > On Mon, 6 Sept 2021 at 13:14, Laurent Pinchart wrote:
> > > On Mon, Sep 06, 2021 at 12:22:25PM +0100, David Plowman wrote:
> > > > Hi Laurent, everyone
> > > >
> > > > Thanks for all the efforts to get this working! I had just a little
> > > > question or two...
> > > >
> > > > 1. Is it easy to tell if a FrameBuffer is actually single plane or
> > > > multi plane? If not, could we add a public API function that would
> > > > tell us?
> > >
> > > You can use FrameBuffer::planes().size() to get the number of planes. Or
> > > did you mean checking if the different planes in a multi-planar frame
> > > buffer are stored contiguously in the same dmabuf ? There's a private
> > > helper for that (FrameBuffer::Private::isContiguous()), I haven't made
> > > it public yet as I wanted to evaluate the use cases first.
> >
> > Yes, isContiguous() sounds pretty much like what I want. I'm
> > interested in functions that make it more convenient for me to know
> > how I pass a buffer (for example) to my V4L2 h.264 encoder.
>
> I wish the V4L2 multi-planar support had been better designed. It's
> painful :-S
>
> > > > 2. Is it easy to get the full size of the buffer for the single plane
> > > > case (rather than having to add all the bits up)? And again, if the
> > > > answer is no, could we add such a thing?
> > > >
> > > > I'm thinking of trying to make life easy for applications that might
> > > > want to pass these buffers to codecs where the driver might only
> > > > support single planes. Not thinking of any platform in particular...
> > > > :)
> > >
> > > It again depends what you mean :-) If the FrameBuffer has a single
> > > plane, FrameBuffer::planes()[0].length (and
> > > FrameMetaData::planes()[0].bytesused) will give you what you need. I
> > > suspect you're however consider the case of a multi-planar FrameBuffer
> > > with planes stored contiguously in memory, using the single-planar V4L2
> > > formats (e.g. V4L2_PIX_FMT_NV12, as opposed to V4L2_PIX_FMT_NV12M). I'm
> > > a bit worried that a helper function in that case would be used by
> > > applications to ignore that the buffer can be truly multi-planar.
> >
> > Perhaps we could define a "safe" version of the function that
> > complains if it was called on a truly multi planar buffer? It might
> > return zero, or print a warning to the console - would that help?
>
> Is [0] what you're essentially looking for ? How would you like to
> handle bytesused vs. length ?
Yes, I think that looks right. I guess we could end up in a situation
where the last "plane" (in a single plane buffer) would carry any
extra bytes that aren't really needed, but that seems OK. The rule is
always that length can be bigger than you really need. But obviously
it must always be at least as big as the hardware required to write
out that plane (whether part of a single plane, or proper mult plane).
That important thing is that it's easy to get back the "true" buffer
sizes.
>
> On a side note, I'm increasingly starting to dislike bytesused, as it
> should only differ from length for compressed formats. Having redundant
> information in the most common case isn't nice.
Sounds right to me. I don't think I ever pay it any attention except
when dealing with the h.264 encoder.
Thanks!
David
>
> [0] https://git.libcamera.org/libcamera/pinchartl/libcamera.git/tree/src/libcamera/v4l2_videodevice.cpp?h=fb/offset&id=f46dffea24149ceaf14e12432db97aed116ac0d4#n1530
>
> > > > On Mon, 6 Sept 2021 at 03:01, Laurent Pinchart wrote:
> > > > >
> > > > > Hello everybody,
> > > > >
> > > > > This patch series started as an investigation of a qcam failure with
> > > > > IPU3 after the merge of the FrameBuffer offset support. While a hack in
> > > > > qcam would be possible, I decided to instead address the core issue and
> > > > > fix it in V4L2VideoDevice.
> > > > >
> > > > > Compared to v1, the series now includes fixes for cam and qcam in
> > > > > addition to the changes needed in the libcamera core. They have been
> > > > > tested with the Raspberry Pi, IPU3, VIMC and UVC pipeline handlers.
> > > > >
> > > > > The GStreamer element seems to work fine without any change required.
> > > > > The V4L2 compatibility layer is still broken, and I haven't tested the
> > > > > Android HAL yet (any volunteer ?).
> > > > >
> > > > > The most important change is in patches 13/27 and 14/27, which translate
> > > > > between V4L2 buffers and libcamera FrameBuffer to handle the case where
> > > > > a multi-planar frame buffer is used with the V4L2 single-planar API.
> > > > > It's working more or less by chance at the moment (except in qcam where
> > > > > it's broken, and possibly in other places I haven't tested). Patches
> > > > > 01/27 to 12/27 are cleanups and additions to prepare for the work in
> > > > > V4L2VideoDevice, and patch 15/27 is a small cleanup on top. Patches
> > > > > 16/27 and 17/27 then improve the FrameBuffer class API as a cleanup.
> > > > >
> > > > > Patches 18/27 to 27/27 fix the cam and qcam applications, as well as an
> > > > > issue in the Android HAL. Worth being noted is patch 19/27 that
> > > > > introduces an Image class shared by cam and qcam. The class duplicates
> > > > > the MappedFrameBuffer implementation private to libcamera. I've tried to
> > > > > rework MappedFrameBuffer into something I would be happy to see in the
> > > > > public API, but failed to do so in a reasonable amount of time, and I
> > > > > didn't want to delay this important regression fix.
> > > > >
> > > > > This series doesn't break any unit test, as vimc doesn't support NV12.
> > > > > Addition of NV12 support to the vimc kernel driver would be very nice,
> > > > > in order to test multi-planar support in our unit tests. Volunteers are
> > > > > welcome ;-)
> > > > >
> > > > > Laurent Pinchart (27):
> > > > > libcamera: base: utils: Use size_t for index in utils::enumerate()
> > > > > libcamera: file_descriptor: Add a function to retrieve the inode
> > > > > libcamera: v4l2_videodevice: Drop toV4L2PixelFormat()
> > > > > libcamera: Use V4L2PixelFormat::fromPixelFormat()
> > > > > libcamera: formats: Move plane info structure to PixelFormatInfo
> > > > > libcamera: formats: Add planeSize() helpers to PixelFormatInfo
> > > > > libcamera: formats: Support V4L2 non-contiguous formats
> > > > > libcamera: framebuffer: Move planes check to constructor
> > > > > libcamera: framebuffer: Add a function to check if planes are
> > > > > contiguous
> > > > > libcamera: v4l2_videodevice: Cache PixelFormatInfo
> > > > > libcamera: v4l2_videodevice: Document plane handling in createBuffer()
> > > > > libcamera: v4l2_videodevice: Take stride into account to compute
> > > > > offsets
> > > > > libcamera: v4l2_videodevice: Coalesce planes when queuing buffer
> > > > > libcamera: v4l2_videodevice: Split planes when dequeuing buffer
> > > > > libcamera: v4l2_videodevice: Use utils::enumerate()
> > > > > libcamera: framebuffer: Allocate metadata planes at construction time
> > > > > libcamera: framebuffer: Prevent modifying the number of metadata
> > > > > planes
> > > > > android: camera_device: Don't assume all planes use the same fd
> > > > > cam: Add Image class
> > > > > cam: file_sink: Use Image class to access pixel data
> > > > > cam: drm: Support per-plane stride values
> > > > > cam: drm: Set per-plane offsets when creating DRM frame buffer
> > > > > cam: drm: Avoid importing the same dmabuf multiple times
> > > > > qcam: Print bytesused for all planes
> > > > > qcam: Use Image class to access pixel data
> > > > > qcam: viewfinder_gl: Support multi-planar buffers
> > > > > qcam: viewfinder_qt: Support multi-planar buffers
> > > > >
> > > > > include/libcamera/base/utils.h | 4 +-
> > > > > include/libcamera/file_descriptor.h | 3 +
> > > > > include/libcamera/framebuffer.h | 19 +-
> > > > > include/libcamera/internal/formats.h | 22 +-
> > > > > include/libcamera/internal/framebuffer.h | 2 +
> > > > > include/libcamera/internal/v4l2_pixelformat.h | 2 +-
> > > > > include/libcamera/internal/v4l2_videodevice.h | 3 +-
> > > > > src/android/camera_device.cpp | 25 +-
> > > > > src/android/mm/generic_camera_buffer.cpp | 11 +-
> > > > > src/android/yuv/post_processor_yuv.cpp | 10 +-
> > > > > src/cam/camera_session.cpp | 4 +-
> > > > > src/cam/drm.cpp | 38 +-
> > > > > src/cam/drm.h | 7 +-
> > > > > src/cam/file_sink.cpp | 44 +--
> > > > > src/cam/file_sink.h | 6 +-
> > > > > src/cam/image.cpp | 107 +++++
> > > > > src/cam/image.h | 52 +++
> > > > > src/cam/kms_sink.cpp | 28 +-
> > > > > src/cam/meson.build | 1 +
> > > > > src/libcamera/file_descriptor.cpp | 26 ++
> > > > > src/libcamera/formats.cpp | 373 ++++++++++++++----
> > > > > src/libcamera/framebuffer.cpp | 57 ++-
> > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 4 +-
> > > > > src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 8 +-
> > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +-
> > > > > src/libcamera/pipeline/simple/converter.cpp | 8 +-
> > > > > src/libcamera/pipeline/simple/simple.cpp | 4 +-
> > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 6 +-
> > > > > src/libcamera/pipeline/vimc/vimc.cpp | 8 +-
> > > > > src/libcamera/v4l2_pixelformat.cpp | 11 +-
> > > > > src/libcamera/v4l2_videodevice.cpp | 196 ++++++---
> > > > > src/qcam/format_converter.cpp | 18 +-
> > > > > src/qcam/format_converter.h | 9 +-
> > > > > src/qcam/main_window.cpp | 38 +-
> > > > > src/qcam/main_window.h | 4 +-
> > > > > src/qcam/meson.build | 1 +
> > > > > src/qcam/viewfinder.h | 6 +-
> > > > > src/qcam/viewfinder_gl.cpp | 45 +--
> > > > > src/qcam/viewfinder_gl.h | 4 +-
> > > > > src/qcam/viewfinder_qt.cpp | 20 +-
> > > > > src/qcam/viewfinder_qt.h | 2 +-
> > > > > src/v4l2/v4l2_camera_proxy.cpp | 11 +-
> > > > > test/libtest/buffer_source.cpp | 3 +-
> > > > > test/utils.cpp | 10 +-
> > > > > 45 files changed, 911 insertions(+), 357 deletions(-)
> > > > > create mode 100644 src/cam/image.cpp
> > > > > create mode 100644 src/cam/image.h
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list