[libcamera-devel] [PATCH v2 19/27] cam: Add Image class

[Laurent Pinchart]

Hi Jean-Michel,

On Mon, Sep 06, 2021 at 10:35:50AM +0200, Jean-Michel Hautbois wrote:
> On 06/09/2021 04:00, Laurent Pinchart wrote:
> > The new Image class represents a multi-planar image with direct access
> > to pixel data. It currently duplicates the function of the
> > MappedFrameBuffer class which is internal to libcamera, and will serve
> > as a design playground to improve the API until it is considered ready
> > to be made part of the libcamera public API.
> 
> I like the idea, maybe add some documentation already in the class ?

That's a good idea, but in the specific case I'd like to get this series
merged ASAP to fix the breakage in the master branch, so I'd prefer
adding the documentation on top.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/cam/image.cpp   | 107 ++++++++++++++++++++++++++++++++++++++++++++
> >  src/cam/image.h     |  52 +++++++++++++++++++++
> >  src/cam/meson.build |   1 +
> >  3 files changed, 160 insertions(+)
> >  create mode 100644 src/cam/image.cpp
> >  create mode 100644 src/cam/image.h
> > 
> > diff --git a/src/cam/image.cpp b/src/cam/image.cpp
> > new file mode 100644
> > index 000000000000..7ae5f52dccb4
> > --- /dev/null
> > +++ b/src/cam/image.cpp
> > @@ -0,0 +1,107 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * image.cpp - Multi-planar image with access to pixel data
> > + */
> > +
> > +#include "image.h"
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <iostream>
> > +#include <map>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <unistd.h>
> > +
> > +using namespace libcamera;
> > +
> > +std::unique_ptr<Image> Image::fromFrameBuffer(const FrameBuffer *buffer, MapMode mode)
> 
> Can you see a use case for Image::toFrameBuffer not implemented yet ?
> What would be in this class apart from a conversion from a FrameBuffer
> to a Image ?

I don't think so. The Image class is an interface to provide access to
pixel data. In its current form it's constructed from a FrameBuffer, but
I'd like the ability to construct it from a byte array as well. This is
related to the MappedVectorBuffer class that Hiro has proposed, it would
allow the JPEG compression in the Android HAL to use an Image as the
source, regardless of whether the compresses the still capture (coming
from libcamera in a FrameBuffer) or the thumbnail (downscaled in
software and stored in a std::vector<uint8_t>).

What I still haven't determined is whether the Image class should be an
interface with pure virtual functions only, implemented by subclasses
such as FrameBufferImage or Memory Image, or if it should contain the
data as well, populated by the different constructors.

I've also started to think about how to perform the mapping. For
FrameBuffer objects constructed from Android buffers, the mapping should
be delegated to gralloc on Android and to the CameraBufferManager on
Chrome OS. For FrameBuffer objects constructed internally by the
V4L2VideoDevice (and in particular the ones exposes to applications with
FrameBufferAllocator), the code below should be correct. For other types
of FrameBuffer objects supplied by applications, another method of
mapping may be needed. I'm not sure yet how to best handle that, and if
we'll need a FrameBufferMapper object that FrameBuffer instances will
reference.

> > +{
> > +	std::unique_ptr<Image> image{ new Image() };
> > +
> > +	assert(!buffer->planes().empty());
> > +
> > +	int mmapFlags = 0;
> > +
> > +	if (mode & MapMode::ReadOnly)
> > +		mmapFlags |= PROT_READ;
> > +
> > +	if (mode & MapMode::WriteOnly)
> > +		mmapFlags |= PROT_WRITE;
> > +
> > +	struct MappedBufferInfo {
> > +		uint8_t *address = nullptr;
> > +		size_t mapLength = 0;
> > +		size_t dmabufLength = 0;
> > +	};
> > +	std::map<int, MappedBufferInfo> mappedBuffers;
> > +
> > +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > +		const int fd = plane.fd.fd();
> > +		if (mappedBuffers.find(fd) == mappedBuffers.end()) {
> > +			const size_t length = lseek(fd, 0, SEEK_END);
> > +			mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length };
> > +		}
> > +
> > +		const size_t length = mappedBuffers[fd].dmabufLength;
> > +
> > +		if (plane.offset > length ||
> > +		    plane.offset + plane.length > length) {
> > +			std::cerr << "plane is out of buffer: buffer length="
> > +				  << length << ", plane offset=" << plane.offset
> > +				  << ", plane length=" << plane.length
> > +				  << std::endl;
> > +			return nullptr;
> > +		}
> > +		size_t &mapLength = mappedBuffers[fd].mapLength;
> > +		mapLength = std::max(mapLength,
> > +				     static_cast<size_t>(plane.offset + plane.length));
> > +	}
> > +
> > +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > +		const int fd = plane.fd.fd();
> > +		auto &info = mappedBuffers[fd];
> > +		if (!info.address) {
> > +			void *address = mmap(nullptr, info.mapLength, mmapFlags,
> > +					     MAP_SHARED, fd, 0);
> > +			if (address == MAP_FAILED) {
> > +				int error = -errno;
> > +				std::cerr << "Failed to mmap plane: "
> > +					  << strerror(-error) << std::endl;
> > +				return nullptr;
> > +			}
> > +
> > +			info.address = static_cast<uint8_t *>(address);
> > +			image->maps_.emplace_back(info.address, info.mapLength);
> > +		}
> > +
> > +		image->planes_.emplace_back(info.address + plane.offset, plane.length);
> > +	}
> > +
> 
> Why are you using two loops on buffer->planes() ? Is it for code clarity
> or something I did not get ?

Because we may have multiple planes using the same dmabuf fd. The first
look gathers the dmabuf fds along with their length, the second loop
then maps them. We need to compute the length to be mapped by looking at
all planes first, before doing any mapping.

> > +	return image;
> > +}
> > +
> > +Image::Image() = default;
> > +
> > +Image::~Image()
> > +{
> > +	for (Span<uint8_t> &map : maps_)
> > +		munmap(map.data(), map.size());
> > +}
> > +
> > +unsigned int Image::numPlanes() const
> > +{
> > +	return planes_.size();
> > +}
> > +
> > +Span<uint8_t> Image::data(unsigned int plane)
> > +{
> > +	return planes_[plane];
> > +}
> > +
> > +Span<const uint8_t> Image::data(unsigned int plane) const
> > +{
> > +	return planes_[plane];
> > +}
> > diff --git a/src/cam/image.h b/src/cam/image.h
> > new file mode 100644
> > index 000000000000..1ce5f84e5f9e
> > --- /dev/null
> > +++ b/src/cam/image.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * image.h - Multi-planar image with access to pixel data
> > + */
> > +#ifndef __CAM_IMAGE_H__
> > +#define __CAM_IMAGE_H__
> > +
> > +#include <memory>
> > +#include <stdint.h>
> > +#include <vector>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/flags.h>
> > +#include <libcamera/base/span.h>
> > +
> > +#include <libcamera/framebuffer.h>
> > +
> > +class Image
> > +{
> > +public:
> > +	enum class MapMode {
> > +		ReadOnly = 1 << 0,
> > +		WriteOnly = 1 << 1,
> > +		ReadWrite = ReadOnly | WriteOnly,
> > +	};
> > +
> > +	static std::unique_ptr<Image> fromFrameBuffer(const libcamera::FrameBuffer *buffer,
> > +						      MapMode mode);
> > +
> > +	~Image();
> > +
> > +	unsigned int numPlanes() const;
> > +
> > +	libcamera::Span<uint8_t> data(unsigned int plane);
> > +	libcamera::Span<const uint8_t> data(unsigned int plane) const;
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(Image)
> > +
> > +	Image();
> > +
> > +	std::vector<libcamera::Span<uint8_t>> maps_;
> > +	std::vector<libcamera::Span<uint8_t>> planes_;
> > +};
> > +
> > +namespace libcamera {
> > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(Image::MapMode)
> > +}
> > +
> > +#endif /* __CAM_IMAGE_H__ */
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index ea36aaa5c514..e8e2ae57d3f4 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -14,6 +14,7 @@ cam_sources = files([
> >      'event_loop.cpp',
> >      'file_sink.cpp',
> >      'frame_sink.cpp',
> > +    'image.cpp',
> >      'main.cpp',
> >      'options.cpp',
> >      'stream_options.cpp',
> > 

-- 
Regards,

Laurent Pinchart