[libcamera-devel] [PATCH v2 19/27] cam: Add Image class
Hirokazu Honda
hiroh at chromium.org
Mon Sep 6 15:42:01 CEST 2021
Hi Laurent,
On Mon, Sep 6, 2021 at 8:55 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > > 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
More information about the libcamera-devel
mailing list