[libcamera-devel] [PATCH v2 00/27] libcamera: Handle fallout of FrameBuffer offset support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 6 19:28:45 CEST 2021
Hi David,
On Mon, Sep 06, 2021 at 06:22:10PM +0100, David Plowman wrote:
> On Mon, 6 Sept 2021 at 15:16, Laurent Pinchart wrote:
> > 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.
By the way, is the hardware for the ISP and the H.264 encoder restricted
to contiguous NV12 planes, or does it support disjoint planes ?
> > [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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list