[libcamera-devel] libcamera: Give MappedFrameBuffer its own implementation
diff mbox series

Message ID 20210806092529.572680-1-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel] libcamera: Give MappedFrameBuffer its own implementation
Related show

Commit Message

Kieran Bingham Aug. 6, 2021, 9:25 a.m. UTC 
The MappedFrameBuffer is a convenience feature which sits on top of the
FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.

This implementation is internal and currently sits in the same internal
files as the internal FrameBuffer, thus exposing those internals to users
of the MappedFramebuffer implementation.

Move the MappedFrameBuffer and MappedBuffer implementation to its own
implementation files, and fix the sources throughout to use that accordingly.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/framebuffer.h      |  36 ----
 .../libcamera/internal/mapped_framebuffer.h   |  52 ++++++
 include/libcamera/internal/meson.build        |   1 +
 src/android/camera_buffer.h                   |   2 +
 src/android/camera_device.h                   |   2 -
 src/android/camera_stream.h                   |   2 -
 src/android/jpeg/encoder_libjpeg.cpp          |   1 +
 src/android/jpeg/encoder_libjpeg.h            |   1 -
 src/android/jpeg/post_processor_jpeg.h        |   2 -
 src/android/jpeg/thumbnailer.cpp              |   2 +
 src/android/jpeg/thumbnailer.h                |   2 +-
 src/android/mm/generic_camera_buffer.cpp      |   2 +-
 src/android/post_processor.h                  |   2 -
 src/android/yuv/post_processor_yuv.cpp        |   1 +
 src/ipa/ipu3/ipu3.cpp                         |   2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           |   2 +-
 src/libcamera/framebuffer.cpp                 | 145 ---------------
 src/libcamera/mapped_framebuffer.cpp          | 171 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 test/mapped-buffer.cpp                        |   2 +-
 20 files changed, 236 insertions(+), 195 deletions(-)
 create mode 100644 include/libcamera/internal/mapped_framebuffer.h
 create mode 100644 src/libcamera/mapped_framebuffer.cpp

Comments

Hirokazu Honda Aug. 6, 2021, 10:29 a.m. UTC | #1 
Hi Kieran, thank you for the patch.

On Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> The MappedFrameBuffer is a convenience feature which sits on top of the
> FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.
>
> This implementation is internal and currently sits in the same internal
> files as the internal FrameBuffer, thus exposing those internals to users
> of the MappedFramebuffer implementation.
>
> Move the MappedFrameBuffer and MappedBuffer implementation to its own
> implementation files, and fix the sources throughout to use that accordingly.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/framebuffer.h      |  36 ----
>  .../libcamera/internal/mapped_framebuffer.h   |  52 ++++++
>  include/libcamera/internal/meson.build        |   1 +
>  src/android/camera_buffer.h                   |   2 +
>  src/android/camera_device.h                   |   2 -
>  src/android/camera_stream.h                   |   2 -
>  src/android/jpeg/encoder_libjpeg.cpp          |   1 +
>  src/android/jpeg/encoder_libjpeg.h            |   1 -
>  src/android/jpeg/post_processor_jpeg.h        |   2 -
>  src/android/jpeg/thumbnailer.cpp              |   2 +
>  src/android/jpeg/thumbnailer.h                |   2 +-
>  src/android/mm/generic_camera_buffer.cpp      |   2 +-
>  src/android/post_processor.h                  |   2 -
>  src/android/yuv/post_processor_yuv.cpp        |   1 +
>  src/ipa/ipu3/ipu3.cpp                         |   2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           |   2 +-
>  src/libcamera/framebuffer.cpp                 | 145 ---------------
>  src/libcamera/mapped_framebuffer.cpp          | 171 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  test/mapped-buffer.cpp                        |   2 +-
>  20 files changed, 236 insertions(+), 195 deletions(-)
>  create mode 100644 include/libcamera/internal/mapped_framebuffer.h
>  create mode 100644 src/libcamera/mapped_framebuffer.cpp
>

I wonder if MappedFrameBuffer is so useful that it should be put in
libcamera/base.
I found src/cam/frame_sink.cpp does mmap()/munmap, which is what
MappedFrameBuffer does.

> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index 8c187adf70c7..1352578a6cfb 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -7,46 +7,10 @@
>  #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
>  #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
>
> -#include <sys/mman.h>
> -#include <vector>
> -
> -#include <libcamera/base/class.h>
> -#include <libcamera/base/span.h>
> -
>  #include <libcamera/framebuffer.h>
>
>  namespace libcamera {
>
> -class MappedBuffer
> -{
> -public:
> -       using Plane = Span<uint8_t>;
> -
> -       ~MappedBuffer();
> -
> -       MappedBuffer(MappedBuffer &&other);
> -       MappedBuffer &operator=(MappedBuffer &&other);
> -
> -       bool isValid() const { return error_ == 0; }
> -       int error() const { return error_; }
> -       const std::vector<Plane> &maps() const { return maps_; }
> -
> -protected:
> -       MappedBuffer();
> -
> -       int error_;
> -       std::vector<Plane> maps_;
> -
> -private:
> -       LIBCAMERA_DISABLE_COPY(MappedBuffer)
> -};
> -
> -class MappedFrameBuffer : public MappedBuffer
> -{
> -public:
> -       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> -};
> -
>  class FrameBuffer::Private : public Extensible::Private
>  {
>         LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> new file mode 100644
> index 000000000000..41e587364260
> --- /dev/null
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mapped_framebuffer.h - Frame buffer memory mapping support
> + */
> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> +
> +#include <sys/mman.h>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/span.h>
> +
> +#include <libcamera/framebuffer.h>
> +
> +namespace libcamera {
> +
> +class MappedBuffer
> +{
> +public:
> +       using Plane = Span<uint8_t>;
> +
> +       ~MappedBuffer();
> +
> +       MappedBuffer(MappedBuffer &&other);
> +       MappedBuffer &operator=(MappedBuffer &&other);
> +
> +       bool isValid() const { return error_ == 0; }
> +       int error() const { return error_; }
> +       const std::vector<Plane> &maps() const { return maps_; }
> +
> +protected:
> +       MappedBuffer();
> +
> +       int error_;
> +       std::vector<Plane> maps_;
> +
> +private:
> +       LIBCAMERA_DISABLE_COPY(MappedBuffer)
> +};
> +
> +class MappedFrameBuffer : public MappedBuffer
> +{
> +public:
> +       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index dac1a2d36fa8..665fd6de4ed3 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
>      'ipa_module.h',
>      'ipa_proxy.h',
>      'ipc_unixsocket.h',
> +    'mapped_framebuffer.h',
>      'media_device.h',
>      'media_object.h',
>      'pipeline_handler.h',
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 21373fa25bf8..e67cfa2b363e 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -7,6 +7,8 @@
>  #ifndef __ANDROID_CAMERA_BUFFER_H__
>  #define __ANDROID_CAMERA_BUFFER_H__
>
> +#include <sys/mman.h>
> +
>  #include <hardware/camera3.h>
>
>  #include <libcamera/base/class.h>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 089a6204605e..dd9aebba7553 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -24,8 +24,6 @@
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
> -#include "libcamera/internal/framebuffer.h"
> -
>  #include "camera_capabilities.h"
>  #include "camera_metadata.h"
>  #include "camera_stream.h"
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 629d9e00e08d..2dab6c3a37d1 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -19,8 +19,6 @@
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>
> -#include "libcamera/internal/framebuffer.h"
> -
>  class CameraDevice;
>  class CameraMetadata;
>  class PostProcessor;
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index e6358ca9466f..372018d2207f 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -23,6 +23,7 @@
>  #include <libcamera/pixel_format.h>
>
>  #include "libcamera/internal/formats.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  using namespace libcamera;
>
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index 14bf89223982..61fbd1a69278 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -10,7 +10,6 @@
>  #include "encoder.h"
>
>  #include "libcamera/internal/formats.h"
> -#include "libcamera/internal/framebuffer.h"
>
>  #include <jpeglib.h>
>
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 5c399be9eb6a..6fd3102229ab 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -13,8 +13,6 @@
>
>  #include <libcamera/geometry.h>
>
> -#include "libcamera/internal/framebuffer.h"
> -
>  class CameraDevice;
>
>  class PostProcessorJpeg : public PostProcessor
> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> index 5cb00744a688..535e2cece914 100644
> --- a/src/android/jpeg/thumbnailer.cpp
> +++ b/src/android/jpeg/thumbnailer.cpp
> @@ -11,6 +11,8 @@
>
>  #include <libcamera/formats.h>
>
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
>  using namespace libcamera;
>
>  LOG_DEFINE_CATEGORY(Thumbnailer)
> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> index 68cbf74329a9..4d086c4943b0 100644
> --- a/src/android/jpeg/thumbnailer.h
> +++ b/src/android/jpeg/thumbnailer.h
> @@ -7,10 +7,10 @@
>  #ifndef __ANDROID_JPEG_THUMBNAILER_H__
>  #define __ANDROID_JPEG_THUMBNAILER_H__
>
> +#include <libcamera/framebuffer.h>
>  #include <libcamera/geometry.h>
>
>  #include "libcamera/internal/formats.h"
> -#include "libcamera/internal/framebuffer.h"
>
>  class Thumbnailer
>  {
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 2a4b77ea5236..0c8adfd999b9 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -11,7 +11,7 @@
>
>  #include <libcamera/base/log.h>
>
> -#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  using namespace libcamera;
>
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index 689f85d9d3b8..ab2b2c606fd0 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -10,8 +10,6 @@
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/stream.h>
>
> -#include "libcamera/internal/framebuffer.h"
> -
>  #include "camera_buffer.h"
>
>  class CameraMetadata;
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index 772e805b32cc..509d4244d614 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -16,6 +16,7 @@
>  #include <libcamera/pixel_format.h>
>
>  #include "libcamera/internal/formats.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  using namespace libcamera;
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36e50f..2647bf2f3b96 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -20,7 +20,7 @@
>  #include <libcamera/ipa/ipu3_ipa_interface.h>
>  #include <libcamera/request.h>
>
> -#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 48e11c699f29..76f67dd4567a 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -28,7 +28,7 @@
>  #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>  #include <libcamera/request.h>
>
> -#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  #include "agc_algorithm.hpp"
>  #include "agc_status.h"
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index a59e93fbdf91..3d98affb20f9 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const
>   * indicate that the metadata is invalid.
>   */
>
> -/**
> - * \class MappedBuffer
> - * \brief Provide an interface to support managing memory mapped buffers
> - *
> - * The MappedBuffer interface provides access to a set of MappedPlanes which
> - * are available for access by the CPU.
> - *
> - * This class is not meant to be constructed directly, but instead derived
> - * classes should be used to implement the correct mapping of a source buffer.
> - *
> - * This allows treating CPU accessible memory through a generic interface
> - * regardless of whether it originates from a libcamera FrameBuffer or other
> - * source.
> - */
> -
> -/**
> - * \typedef MappedBuffer::Plane
> - * \brief A mapped region of memory accessible to the CPU
> - *
> - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
> - * region.
> - */
> -
> -/**
> - * \brief Construct an empty MappedBuffer
> - */
> -MappedBuffer::MappedBuffer()
> -       : error_(0)
> -{
> -}
> -
> -/**
> - * \brief Move constructor, construct the MappedBuffer with the contents of \a
> - * other using move semantics
> - * \param[in] other The other MappedBuffer
> - *
> - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> - * MappedBuffer and invalidates the \a other.
> - *
> - * No mappings are unmapped or destroyed in this process.
> - */
> -MappedBuffer::MappedBuffer(MappedBuffer &&other)
> -{
> -       *this = std::move(other);
> -}
> -
> -/**
> - * \brief Move assignment operator, replace the mappings with those of \a other
> -* \param[in] other The other MappedBuffer
> - *
> - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> - * MappedBuffer and invalidates the \a other.
> - *
> - * No mappings are unmapped or destroyed in this process.
> - */
> -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> -{
> -       error_ = other.error_;
> -       maps_ = std::move(other.maps_);
> -       other.error_ = -ENOENT;
> -
> -       return *this;
> -}
> -
> -MappedBuffer::~MappedBuffer()
> -{
> -       for (Plane &map : maps_)
> -               munmap(map.data(), map.size());
> -}
> -
> -/**
> - * \fn MappedBuffer::isValid()
> - * \brief Check if the MappedBuffer instance is valid
> - * \return True if the MappedBuffer has valid mappings, false otherwise
> - */
> -
> -/**
> - * \fn MappedBuffer::error()
> - * \brief Retrieve the map error status
> - *
> - * This function retrieves the error status from the MappedBuffer.
> - * The error status is a negative number as defined by errno.h. If
> - * no error occurred, this function returns 0.
> - *
> - * \return The map error code
> - */
> -
> -/**
> - * \fn MappedBuffer::maps()
> - * \brief Retrieve the mapped planes
> - *
> - * This function retrieves the successfully mapped planes stored as a vector
> - * of Span<uint8_t> to provide access to the mapped memory.
> - *
> - * \return A vector of the mapped planes
> - */
> -
> -/**
> - * \var MappedBuffer::error_
> - * \brief Stores the error value if present
> - *
> - * MappedBuffer derived classes shall set this to a negative value as defined
> - * by errno.h if an error occured during the mapping process.
> - */
> -
> -/**
> - * \var MappedBuffer::maps_
> - * \brief Stores the internal mapped planes
> - *
> - * MappedBuffer derived classes shall store the mappings they create in this
> - * vector which is parsed during destruct to unmap any memory mappings which
> - * completed successfully.
> - */
> -
> -/**
> - * \class MappedFrameBuffer
> - * \brief Map a FrameBuffer using the MappedBuffer interface
> - */
> -
> -/**
> - * \brief Map all planes of a FrameBuffer
> - * \param[in] buffer FrameBuffer to be mapped
> - * \param[in] flags Protection flags to apply to map
> - *
> - * Construct an object to map a frame buffer for CPU access.
> - * The flags are passed directly to mmap and should be either PROT_READ,
> - * PROT_WRITE, or a bitwise-or combination of both.
> - */
> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> -{
> -       maps_.reserve(buffer->planes().size());
> -
> -       for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -               void *address = mmap(nullptr, plane.length, flags,
> -                                    MAP_SHARED, plane.fd.fd(), 0);
> -               if (address == MAP_FAILED) {
> -                       error_ = -errno;
> -                       LOG(Buffer, Error) << "Failed to mmap plane";
> -                       break;
> -               }
> -
> -               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> -       }
> -}
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> new file mode 100644
> index 000000000000..0e30fc542154
> --- /dev/null
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -0,0 +1,171 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * mapped_framebuffer.cpp - Mapped Framebuffer support
> + */
> +
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
> +#include <errno.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file libcamera/internal/mapped_framebuffer.h
> + * \brief Frame buffer memory mapping support
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Buffer)
> +
> +/**
> + * \class MappedBuffer
> + * \brief Provide an interface to support managing memory mapped buffers
> + *
> + * The MappedBuffer interface provides access to a set of MappedPlanes which
> + * are available for access by the CPU.
> + *
> + * This class is not meant to be constructed directly, but instead derived
> + * classes should be used to implement the correct mapping of a source buffer.
> + *
> + * This allows treating CPU accessible memory through a generic interface
> + * regardless of whether it originates from a libcamera FrameBuffer or other
> + * source.
> + */
> +
> +/**
> + * \typedef MappedBuffer::Plane
> + * \brief A mapped region of memory accessible to the CPU
> + *
> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
> + * region.
> + */
> +
> +/**
> + * \brief Construct an empty MappedBuffer
> + */
> +MappedBuffer::MappedBuffer()
> +       : error_(0)
> +{
> +}
> +
> +/**
> + * \brief Move constructor, construct the MappedBuffer with the contents of \a
> + * other using move semantics
> + * \param[in] other The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> + * MappedBuffer and invalidates the \a other.
> + *
> + * No mappings are unmapped or destroyed in this process.
> + */
> +MappedBuffer::MappedBuffer(MappedBuffer &&other)
> +{
> +       *this = std::move(other);
> +}
> +
> +/**
> + * \brief Move assignment operator, replace the mappings with those of \a other
> +* \param[in] other The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> + * MappedBuffer and invalidates the \a other.
> + *
> + * No mappings are unmapped or destroyed in this process.
> + */
> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> +{
> +       error_ = other.error_;
> +       maps_ = std::move(other.maps_);
> +       other.error_ = -ENOENT;
> +
> +       return *this;
> +}
> +
> +MappedBuffer::~MappedBuffer()
> +{
> +       for (Plane &map : maps_)
> +               munmap(map.data(), map.size());
> +}
> +
> +/**
> + * \fn MappedBuffer::isValid()
> + * \brief Check if the MappedBuffer instance is valid
> + * \return True if the MappedBuffer has valid mappings, false otherwise
> + */
> +
> +/**
> + * \fn MappedBuffer::error()
> + * \brief Retrieve the map error status
> + *
> + * This function retrieves the error status from the MappedBuffer.
> + * The error status is a negative number as defined by errno.h. If
> + * no error occurred, this function returns 0.
> + *
> + * \return The map error code
> + */
> +
> +/**
> + * \fn MappedBuffer::maps()
> + * \brief Retrieve the mapped planes
> + *
> + * This function retrieves the successfully mapped planes stored as a vector
> + * of Span<uint8_t> to provide access to the mapped memory.
> + *
> + * \return A vector of the mapped planes
> + */
> +
> +/**
> + * \var MappedBuffer::error_
> + * \brief Stores the error value if present
> + *
> + * MappedBuffer derived classes shall set this to a negative value as defined
> + * by errno.h if an error occured during the mapping process.
> + */
> +
> +/**
> + * \var MappedBuffer::maps_
> + * \brief Stores the internal mapped planes
> + *
> + * MappedBuffer derived classes shall store the mappings they create in this
> + * vector which is parsed during destruct to unmap any memory mappings which
> + * completed successfully.
> + */
> +
> +/**
> + * \class MappedFrameBuffer
> + * \brief Map a FrameBuffer using the MappedBuffer interface
> + */
> +
> +/**
> + * \brief Map all planes of a FrameBuffer
> + * \param[in] buffer FrameBuffer to be mapped
> + * \param[in] flags Protection flags to apply to map
> + *
> + * Construct an object to map a frame buffer for CPU access.
> + * The flags are passed directly to mmap and should be either PROT_READ,
> + * PROT_WRITE, or a bitwise-or combination of both.
> + */
> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> +{
> +       maps_.reserve(buffer->planes().size());
> +
> +       for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +               void *address = mmap(nullptr, plane.length, flags,
> +                                    MAP_SHARED, plane.fd.fd(), 0);
> +               if (address == MAP_FAILED) {
> +                       error_ = -errno;
> +                       LOG(Buffer, Error) << "Failed to mmap plane";
> +                       break;
> +               }
> +
> +               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +       }
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 4f08580157f9..e9230b983aeb 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -28,6 +28,7 @@ libcamera_sources = files([
>      'ipc_pipe.cpp',
>      'ipc_pipe_unixsocket.cpp',
>      'ipc_unixsocket.cpp',
> +    'mapped_framebuffer.cpp',
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> index c9479194cb68..a3d1511b74ce 100644
> --- a/test/mapped-buffer.cpp
> +++ b/test/mapped-buffer.cpp
> @@ -9,7 +9,7 @@
>
>  #include <libcamera/framebuffer_allocator.h>
>
> -#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>
>  #include "camera_test.h"
>  #include "test.h"
> --
> 2.30.2
>
Laurent Pinchart Aug. 6, 2021, 10:37 a.m. UTC | #2 
Hi Hiro,

On Fri, Aug 06, 2021 at 07:29:57PM +0900, Hirokazu Honda wrote:
> On Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham wrote:
> >
> > The MappedFrameBuffer is a convenience feature which sits on top of the
> > FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.
> >
> > This implementation is internal and currently sits in the same internal
> > files as the internal FrameBuffer, thus exposing those internals to users
> > of the MappedFramebuffer implementation.
> >
> > Move the MappedFrameBuffer and MappedBuffer implementation to its own
> > implementation files, and fix the sources throughout to use that accordingly.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/internal/framebuffer.h      |  36 ----
> >  .../libcamera/internal/mapped_framebuffer.h   |  52 ++++++
> >  include/libcamera/internal/meson.build        |   1 +
> >  src/android/camera_buffer.h                   |   2 +
> >  src/android/camera_device.h                   |   2 -
> >  src/android/camera_stream.h                   |   2 -
> >  src/android/jpeg/encoder_libjpeg.cpp          |   1 +
> >  src/android/jpeg/encoder_libjpeg.h            |   1 -
> >  src/android/jpeg/post_processor_jpeg.h        |   2 -
> >  src/android/jpeg/thumbnailer.cpp              |   2 +
> >  src/android/jpeg/thumbnailer.h                |   2 +-
> >  src/android/mm/generic_camera_buffer.cpp      |   2 +-
> >  src/android/post_processor.h                  |   2 -
> >  src/android/yuv/post_processor_yuv.cpp        |   1 +
> >  src/ipa/ipu3/ipu3.cpp                         |   2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           |   2 +-
> >  src/libcamera/framebuffer.cpp                 | 145 ---------------
> >  src/libcamera/mapped_framebuffer.cpp          | 171 ++++++++++++++++++
> >  src/libcamera/meson.build                     |   1 +
> >  test/mapped-buffer.cpp                        |   2 +-
> >  20 files changed, 236 insertions(+), 195 deletions(-)
> >  create mode 100644 include/libcamera/internal/mapped_framebuffer.h
> >  create mode 100644 src/libcamera/mapped_framebuffer.cpp
> 
> I wonder if MappedFrameBuffer is so useful that it should be put in
> libcamera/base.
> I found src/cam/frame_sink.cpp does mmap()/munmap, which is what
> MappedFrameBuffer does.

The issue is that MappedFrameBuffer depends on FrameBuffer, which is
part of libcamera, not libcamera base. This is something we have
discussed before, and we have decided to postpone the discussion :-) One
option would be to make MappedFrameBuffer part of the libcamera public
API, but we will then need to polish the API a bit.

> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > index 8c187adf70c7..1352578a6cfb 100644
> > --- a/include/libcamera/internal/framebuffer.h
> > +++ b/include/libcamera/internal/framebuffer.h
> > @@ -7,46 +7,10 @@
> >  #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
> >  #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
> >
> > -#include <sys/mman.h>
> > -#include <vector>
> > -
> > -#include <libcamera/base/class.h>
> > -#include <libcamera/base/span.h>
> > -
> >  #include <libcamera/framebuffer.h>
> >
> >  namespace libcamera {
> >
> > -class MappedBuffer
> > -{
> > -public:
> > -       using Plane = Span<uint8_t>;
> > -
> > -       ~MappedBuffer();
> > -
> > -       MappedBuffer(MappedBuffer &&other);
> > -       MappedBuffer &operator=(MappedBuffer &&other);
> > -
> > -       bool isValid() const { return error_ == 0; }
> > -       int error() const { return error_; }
> > -       const std::vector<Plane> &maps() const { return maps_; }
> > -
> > -protected:
> > -       MappedBuffer();
> > -
> > -       int error_;
> > -       std::vector<Plane> maps_;
> > -
> > -private:
> > -       LIBCAMERA_DISABLE_COPY(MappedBuffer)
> > -};
> > -
> > -class MappedFrameBuffer : public MappedBuffer
> > -{
> > -public:
> > -       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> > -};
> > -
> >  class FrameBuffer::Private : public Extensible::Private
> >  {
> >         LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> > new file mode 100644
> > index 000000000000..41e587364260
> > --- /dev/null
> > +++ b/include/libcamera/internal/mapped_framebuffer.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * mapped_framebuffer.h - Frame buffer memory mapping support
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> > +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> > +
> > +#include <sys/mman.h>

This isn't needed anymore, but you need stdint.h instead.

> > +#include <vector>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/span.h>
> > +
> > +#include <libcamera/framebuffer.h>
> > +
> > +namespace libcamera {
> > +
> > +class MappedBuffer
> > +{
> > +public:
> > +       using Plane = Span<uint8_t>;
> > +
> > +       ~MappedBuffer();
> > +
> > +       MappedBuffer(MappedBuffer &&other);
> > +       MappedBuffer &operator=(MappedBuffer &&other);
> > +
> > +       bool isValid() const { return error_ == 0; }
> > +       int error() const { return error_; }
> > +       const std::vector<Plane> &maps() const { return maps_; }
> > +
> > +protected:
> > +       MappedBuffer();
> > +
> > +       int error_;
> > +       std::vector<Plane> maps_;
> > +
> > +private:
> > +       LIBCAMERA_DISABLE_COPY(MappedBuffer)
> > +};
> > +
> > +class MappedFrameBuffer : public MappedBuffer
> > +{
> > +public:
> > +       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index dac1a2d36fa8..665fd6de4ed3 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
> >      'ipa_module.h',
> >      'ipa_proxy.h',
> >      'ipc_unixsocket.h',
> > +    'mapped_framebuffer.h',
> >      'media_device.h',
> >      'media_object.h',
> >      'pipeline_handler.h',
> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > index 21373fa25bf8..e67cfa2b363e 100644
> > --- a/src/android/camera_buffer.h
> > +++ b/src/android/camera_buffer.h
> > @@ -7,6 +7,8 @@
> >  #ifndef __ANDROID_CAMERA_BUFFER_H__
> >  #define __ANDROID_CAMERA_BUFFER_H__
> >
> > +#include <sys/mman.h>
> > +
> >  #include <hardware/camera3.h>
> >
> >  #include <libcamera/base/class.h>
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 089a6204605e..dd9aebba7553 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -24,8 +24,6 @@
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > -
> >  #include "camera_capabilities.h"
> >  #include "camera_metadata.h"
> >  #include "camera_stream.h"
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 629d9e00e08d..2dab6c3a37d1 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -19,8 +19,6 @@
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/pixel_format.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > -
> >  class CameraDevice;
> >  class CameraMetadata;
> >  class PostProcessor;
> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> > index e6358ca9466f..372018d2207f 100644
> > --- a/src/android/jpeg/encoder_libjpeg.cpp
> > +++ b/src/android/jpeg/encoder_libjpeg.cpp
> > @@ -23,6 +23,7 @@
> >  #include <libcamera/pixel_format.h>
> >
> >  #include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  using namespace libcamera;
> >
> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> > index 14bf89223982..61fbd1a69278 100644
> > --- a/src/android/jpeg/encoder_libjpeg.h
> > +++ b/src/android/jpeg/encoder_libjpeg.h
> > @@ -10,7 +10,6 @@
> >  #include "encoder.h"
> >
> >  #include "libcamera/internal/formats.h"
> > -#include "libcamera/internal/framebuffer.h"
> >
> >  #include <jpeglib.h>
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 5c399be9eb6a..6fd3102229ab 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -13,8 +13,6 @@
> >
> >  #include <libcamera/geometry.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > -
> >  class CameraDevice;
> >
> >  class PostProcessorJpeg : public PostProcessor
> > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
> > index 5cb00744a688..535e2cece914 100644
> > --- a/src/android/jpeg/thumbnailer.cpp
> > +++ b/src/android/jpeg/thumbnailer.cpp
> > @@ -11,6 +11,8 @@
> >
> >  #include <libcamera/formats.h>
> >
> > +#include "libcamera/internal/mapped_framebuffer.h"
> > +
> >  using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(Thumbnailer)
> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > index 68cbf74329a9..4d086c4943b0 100644
> > --- a/src/android/jpeg/thumbnailer.h
> > +++ b/src/android/jpeg/thumbnailer.h
> > @@ -7,10 +7,10 @@
> >  #ifndef __ANDROID_JPEG_THUMBNAILER_H__
> >  #define __ANDROID_JPEG_THUMBNAILER_H__
> >
> > +#include <libcamera/framebuffer.h>
> >  #include <libcamera/geometry.h>
> >
> >  #include "libcamera/internal/formats.h"
> > -#include "libcamera/internal/framebuffer.h"
> >
> >  class Thumbnailer
> >  {
> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > index 2a4b77ea5236..0c8adfd999b9 100644
> > --- a/src/android/mm/generic_camera_buffer.cpp
> > +++ b/src/android/mm/generic_camera_buffer.cpp
> > @@ -11,7 +11,7 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  using namespace libcamera;
> >
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index 689f85d9d3b8..ab2b2c606fd0 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -10,8 +10,6 @@
> >  #include <libcamera/framebuffer.h>
> >  #include <libcamera/stream.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > -
> >  #include "camera_buffer.h"
> >
> >  class CameraMetadata;
> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> > index 772e805b32cc..509d4244d614 100644
> > --- a/src/android/yuv/post_processor_yuv.cpp
> > +++ b/src/android/yuv/post_processor_yuv.cpp
> > @@ -16,6 +16,7 @@
> >  #include <libcamera/pixel_format.h>
> >
> >  #include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  using namespace libcamera;
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 71698d36e50f..2647bf2f3b96 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -20,7 +20,7 @@
> >  #include <libcamera/ipa/ipu3_ipa_interface.h>
> >  #include <libcamera/request.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  #include "ipu3_agc.h"
> >  #include "ipu3_awb.h"
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 48e11c699f29..76f67dd4567a 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -28,7 +28,7 @@
> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> >  #include <libcamera/request.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  #include "agc_algorithm.hpp"
> >  #include "agc_status.h"
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index a59e93fbdf91..3d98affb20f9 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const
> >   * indicate that the metadata is invalid.
> >   */
> >
> > -/**
> > - * \class MappedBuffer
> > - * \brief Provide an interface to support managing memory mapped buffers
> > - *
> > - * The MappedBuffer interface provides access to a set of MappedPlanes which
> > - * are available for access by the CPU.
> > - *
> > - * This class is not meant to be constructed directly, but instead derived
> > - * classes should be used to implement the correct mapping of a source buffer.
> > - *
> > - * This allows treating CPU accessible memory through a generic interface
> > - * regardless of whether it originates from a libcamera FrameBuffer or other
> > - * source.
> > - */
> > -
> > -/**
> > - * \typedef MappedBuffer::Plane
> > - * \brief A mapped region of memory accessible to the CPU
> > - *
> > - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
> > - * region.
> > - */
> > -
> > -/**
> > - * \brief Construct an empty MappedBuffer
> > - */
> > -MappedBuffer::MappedBuffer()
> > -       : error_(0)
> > -{
> > -}
> > -
> > -/**
> > - * \brief Move constructor, construct the MappedBuffer with the contents of \a
> > - * other using move semantics
> > - * \param[in] other The other MappedBuffer
> > - *
> > - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> > - * MappedBuffer and invalidates the \a other.
> > - *
> > - * No mappings are unmapped or destroyed in this process.
> > - */
> > -MappedBuffer::MappedBuffer(MappedBuffer &&other)
> > -{
> > -       *this = std::move(other);
> > -}
> > -
> > -/**
> > - * \brief Move assignment operator, replace the mappings with those of \a other
> > -* \param[in] other The other MappedBuffer
> > - *
> > - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> > - * MappedBuffer and invalidates the \a other.
> > - *
> > - * No mappings are unmapped or destroyed in this process.
> > - */
> > -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> > -{
> > -       error_ = other.error_;
> > -       maps_ = std::move(other.maps_);
> > -       other.error_ = -ENOENT;
> > -
> > -       return *this;
> > -}
> > -
> > -MappedBuffer::~MappedBuffer()
> > -{
> > -       for (Plane &map : maps_)
> > -               munmap(map.data(), map.size());
> > -}
> > -
> > -/**
> > - * \fn MappedBuffer::isValid()
> > - * \brief Check if the MappedBuffer instance is valid
> > - * \return True if the MappedBuffer has valid mappings, false otherwise
> > - */
> > -
> > -/**
> > - * \fn MappedBuffer::error()
> > - * \brief Retrieve the map error status
> > - *
> > - * This function retrieves the error status from the MappedBuffer.
> > - * The error status is a negative number as defined by errno.h. If
> > - * no error occurred, this function returns 0.
> > - *
> > - * \return The map error code
> > - */
> > -
> > -/**
> > - * \fn MappedBuffer::maps()
> > - * \brief Retrieve the mapped planes
> > - *
> > - * This function retrieves the successfully mapped planes stored as a vector
> > - * of Span<uint8_t> to provide access to the mapped memory.
> > - *
> > - * \return A vector of the mapped planes
> > - */
> > -
> > -/**
> > - * \var MappedBuffer::error_
> > - * \brief Stores the error value if present
> > - *
> > - * MappedBuffer derived classes shall set this to a negative value as defined
> > - * by errno.h if an error occured during the mapping process.
> > - */
> > -
> > -/**
> > - * \var MappedBuffer::maps_
> > - * \brief Stores the internal mapped planes
> > - *
> > - * MappedBuffer derived classes shall store the mappings they create in this
> > - * vector which is parsed during destruct to unmap any memory mappings which
> > - * completed successfully.
> > - */
> > -
> > -/**
> > - * \class MappedFrameBuffer
> > - * \brief Map a FrameBuffer using the MappedBuffer interface
> > - */
> > -
> > -/**
> > - * \brief Map all planes of a FrameBuffer
> > - * \param[in] buffer FrameBuffer to be mapped
> > - * \param[in] flags Protection flags to apply to map
> > - *
> > - * Construct an object to map a frame buffer for CPU access.
> > - * The flags are passed directly to mmap and should be either PROT_READ,
> > - * PROT_WRITE, or a bitwise-or combination of both.
> > - */
> > -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> > -{
> > -       maps_.reserve(buffer->planes().size());
> > -
> > -       for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > -               void *address = mmap(nullptr, plane.length, flags,
> > -                                    MAP_SHARED, plane.fd.fd(), 0);
> > -               if (address == MAP_FAILED) {
> > -                       error_ = -errno;
> > -                       LOG(Buffer, Error) << "Failed to mmap plane";
> > -                       break;
> > -               }
> > -
> > -               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> > -       }
> > -}
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> > new file mode 100644
> > index 000000000000..0e30fc542154
> > --- /dev/null
> > +++ b/src/libcamera/mapped_framebuffer.cpp
> > @@ -0,0 +1,171 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * mapped_framebuffer.cpp - Mapped Framebuffer support
> > + */
> > +
> > +#include "libcamera/internal/mapped_framebuffer.h"
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file libcamera/internal/mapped_framebuffer.h
> > + * \brief Frame buffer memory mapping support
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(Buffer)
> > +
> > +/**
> > + * \class MappedBuffer
> > + * \brief Provide an interface to support managing memory mapped buffers
> > + *
> > + * The MappedBuffer interface provides access to a set of MappedPlanes which
> > + * are available for access by the CPU.
> > + *
> > + * This class is not meant to be constructed directly, but instead derived
> > + * classes should be used to implement the correct mapping of a source buffer.
> > + *
> > + * This allows treating CPU accessible memory through a generic interface
> > + * regardless of whether it originates from a libcamera FrameBuffer or other
> > + * source.
> > + */
> > +
> > +/**
> > + * \typedef MappedBuffer::Plane
> > + * \brief A mapped region of memory accessible to the CPU
> > + *
> > + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
> > + * region.
> > + */
> > +
> > +/**
> > + * \brief Construct an empty MappedBuffer
> > + */
> > +MappedBuffer::MappedBuffer()
> > +       : error_(0)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Move constructor, construct the MappedBuffer with the contents of \a
> > + * other using move semantics
> > + * \param[in] other The other MappedBuffer
> > + *
> > + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> > + * MappedBuffer and invalidates the \a other.
> > + *
> > + * No mappings are unmapped or destroyed in this process.
> > + */
> > +MappedBuffer::MappedBuffer(MappedBuffer &&other)
> > +{
> > +       *this = std::move(other);
> > +}
> > +
> > +/**
> > + * \brief Move assignment operator, replace the mappings with those of \a other
> > +* \param[in] other The other MappedBuffer
> > + *
> > + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
> > + * MappedBuffer and invalidates the \a other.
> > + *
> > + * No mappings are unmapped or destroyed in this process.
> > + */
> > +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> > +{
> > +       error_ = other.error_;
> > +       maps_ = std::move(other.maps_);
> > +       other.error_ = -ENOENT;
> > +
> > +       return *this;
> > +}
> > +
> > +MappedBuffer::~MappedBuffer()
> > +{
> > +       for (Plane &map : maps_)
> > +               munmap(map.data(), map.size());
> > +}
> > +
> > +/**
> > + * \fn MappedBuffer::isValid()
> > + * \brief Check if the MappedBuffer instance is valid
> > + * \return True if the MappedBuffer has valid mappings, false otherwise
> > + */
> > +
> > +/**
> > + * \fn MappedBuffer::error()
> > + * \brief Retrieve the map error status
> > + *
> > + * This function retrieves the error status from the MappedBuffer.
> > + * The error status is a negative number as defined by errno.h. If
> > + * no error occurred, this function returns 0.
> > + *
> > + * \return The map error code
> > + */
> > +
> > +/**
> > + * \fn MappedBuffer::maps()
> > + * \brief Retrieve the mapped planes
> > + *
> > + * This function retrieves the successfully mapped planes stored as a vector
> > + * of Span<uint8_t> to provide access to the mapped memory.
> > + *
> > + * \return A vector of the mapped planes
> > + */
> > +
> > +/**
> > + * \var MappedBuffer::error_
> > + * \brief Stores the error value if present
> > + *
> > + * MappedBuffer derived classes shall set this to a negative value as defined
> > + * by errno.h if an error occured during the mapping process.
> > + */
> > +
> > +/**
> > + * \var MappedBuffer::maps_
> > + * \brief Stores the internal mapped planes
> > + *
> > + * MappedBuffer derived classes shall store the mappings they create in this
> > + * vector which is parsed during destruct to unmap any memory mappings which
> > + * completed successfully.
> > + */
> > +
> > +/**
> > + * \class MappedFrameBuffer
> > + * \brief Map a FrameBuffer using the MappedBuffer interface
> > + */
> > +
> > +/**
> > + * \brief Map all planes of a FrameBuffer
> > + * \param[in] buffer FrameBuffer to be mapped
> > + * \param[in] flags Protection flags to apply to map
> > + *
> > + * Construct an object to map a frame buffer for CPU access.
> > + * The flags are passed directly to mmap and should be either PROT_READ,
> > + * PROT_WRITE, or a bitwise-or combination of both.
> > + */
> > +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> > +{
> > +       maps_.reserve(buffer->planes().size());
> > +
> > +       for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > +               void *address = mmap(nullptr, plane.length, flags,
> > +                                    MAP_SHARED, plane.fd.fd(), 0);
> > +               if (address == MAP_FAILED) {
> > +                       error_ = -errno;
> > +                       LOG(Buffer, Error) << "Failed to mmap plane";
> > +                       break;
> > +               }
> > +
> > +               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> > +       }
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 4f08580157f9..e9230b983aeb 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -28,6 +28,7 @@ libcamera_sources = files([
> >      'ipc_pipe.cpp',
> >      'ipc_pipe_unixsocket.cpp',
> >      'ipc_unixsocket.cpp',
> > +    'mapped_framebuffer.cpp',
> >      'media_device.cpp',
> >      'media_object.cpp',
> >      'pipeline_handler.cpp',
> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> > index c9479194cb68..a3d1511b74ce 100644
> > --- a/test/mapped-buffer.cpp
> > +++ b/test/mapped-buffer.cpp
> > @@ -9,7 +9,7 @@
> >
> >  #include <libcamera/framebuffer_allocator.h>
> >
> > -#include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >
> >  #include "camera_test.h"
> >  #include "test.h"
Kieran Bingham Aug. 6, 2021, 10:44 a.m. UTC | #3 
Hi Hiro,

On 06/08/2021 11:29, Hirokazu Honda wrote:
> Hi Kieran, thank you for the patch.
> 
> On Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>>
>> The MappedFrameBuffer is a convenience feature which sits on top of the
>> FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.
>>
>> This implementation is internal and currently sits in the same internal
>> files as the internal FrameBuffer, thus exposing those internals to users
>> of the MappedFramebuffer implementation.
>>
>> Move the MappedFrameBuffer and MappedBuffer implementation to its own
>> implementation files, and fix the sources throughout to use that accordingly.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  include/libcamera/internal/framebuffer.h      |  36 ----
>>  .../libcamera/internal/mapped_framebuffer.h   |  52 ++++++
>>  include/libcamera/internal/meson.build        |   1 +
>>  src/android/camera_buffer.h                   |   2 +
>>  src/android/camera_device.h                   |   2 -
>>  src/android/camera_stream.h                   |   2 -
>>  src/android/jpeg/encoder_libjpeg.cpp          |   1 +
>>  src/android/jpeg/encoder_libjpeg.h            |   1 -
>>  src/android/jpeg/post_processor_jpeg.h        |   2 -
>>  src/android/jpeg/thumbnailer.cpp              |   2 +
>>  src/android/jpeg/thumbnailer.h                |   2 +-
>>  src/android/mm/generic_camera_buffer.cpp      |   2 +-
>>  src/android/post_processor.h                  |   2 -
>>  src/android/yuv/post_processor_yuv.cpp        |   1 +
>>  src/ipa/ipu3/ipu3.cpp                         |   2 +-
>>  src/ipa/raspberrypi/raspberrypi.cpp           |   2 +-
>>  src/libcamera/framebuffer.cpp                 | 145 ---------------
>>  src/libcamera/mapped_framebuffer.cpp          | 171 ++++++++++++++++++
>>  src/libcamera/meson.build                     |   1 +
>>  test/mapped-buffer.cpp                        |   2 +-
>>  20 files changed, 236 insertions(+), 195 deletions(-)
>>  create mode 100644 include/libcamera/internal/mapped_framebuffer.h
>>  create mode 100644 src/libcamera/mapped_framebuffer.cpp
>>
> 
> I wonder if MappedFrameBuffer is so useful that it should be put in
> libcamera/base.

That's ... almost the partial intention of this.
However - base sits 'underneath' libcamera, and this class requires
FrameBuffer which means it has to go 'on top' of libcamera.

We already extracted a copy of this class to the Intel IPA, so indeed
there is scope for this being used elsewhere - but it needs a somewhat
'new' home which isn't determined yet.

So this is part of that clean up eitherway.

> I found src/cam/frame_sink.cpp does mmap()/munmap, which is what
> MappedFrameBuffer does.

Yes, I wanted that requirement to be abstracted - but it was argued that
MappedFrameBuffer should not form part of the libcamera public API.

But as you've seen - there's a lot of detail to mapping FrameBuffers -
and I don't want every application to have to duplicate all of that - so
theres a potential need for this to live 'somewhere'.

We already envisioned a 'helper' library on top of libcamera - so it's
possible it will move to something like that.


>> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
>> index 8c187adf70c7..1352578a6cfb 100644
>> --- a/include/libcamera/internal/framebuffer.h
>> +++ b/include/libcamera/internal/framebuffer.h
>> @@ -7,46 +7,10 @@
>>  #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
>>  #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
>>
>> -#include <sys/mman.h>
>> -#include <vector>
>> -
>> -#include <libcamera/base/class.h>
>> -#include <libcamera/base/span.h>
>> -
>>  #include <libcamera/framebuffer.h>
>>
>>  namespace libcamera {
>>
>> -class MappedBuffer
>> -{
>> -public:
>> -       using Plane = Span<uint8_t>;
>> -
>> -       ~MappedBuffer();
>> -
>> -       MappedBuffer(MappedBuffer &&other);
>> -       MappedBuffer &operator=(MappedBuffer &&other);
>> -
>> -       bool isValid() const { return error_ == 0; }
>> -       int error() const { return error_; }
>> -       const std::vector<Plane> &maps() const { return maps_; }
>> -
>> -protected:
>> -       MappedBuffer();
>> -
>> -       int error_;
>> -       std::vector<Plane> maps_;
>> -
>> -private:
>> -       LIBCAMERA_DISABLE_COPY(MappedBuffer)
>> -};
>> -
>> -class MappedFrameBuffer : public MappedBuffer
>> -{
>> -public:
>> -       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>> -};
>> -
>>  class FrameBuffer::Private : public Extensible::Private
>>  {
>>         LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
>> new file mode 100644
>> index 000000000000..41e587364260
>> --- /dev/null
>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * mapped_framebuffer.h - Frame buffer memory mapping support
>> + */
>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>> +
>> +#include <sys/mman.h>
>> +#include <vector>
>> +
>> +#include <libcamera/base/class.h>
>> +#include <libcamera/base/span.h>
>> +
>> +#include <libcamera/framebuffer.h>
>> +
>> +namespace libcamera {
>> +
>> +class MappedBuffer
>> +{
>> +public:
>> +       using Plane = Span<uint8_t>;
>> +
>> +       ~MappedBuffer();
>> +
>> +       MappedBuffer(MappedBuffer &&other);
>> +       MappedBuffer &operator=(MappedBuffer &&other);
>> +
>> +       bool isValid() const { return error_ == 0; }
>> +       int error() const { return error_; }
>> +       const std::vector<Plane> &maps() const { return maps_; }
>> +
>> +protected:
>> +       MappedBuffer();
>> +
>> +       int error_;
>> +       std::vector<Plane> maps_;
>> +
>> +private:
>> +       LIBCAMERA_DISABLE_COPY(MappedBuffer)
>> +};
>> +
>> +class MappedFrameBuffer : public MappedBuffer
>> +{
>> +public:
>> +       MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index dac1a2d36fa8..665fd6de4ed3 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
>>      'ipa_module.h',
>>      'ipa_proxy.h',
>>      'ipc_unixsocket.h',
>> +    'mapped_framebuffer.h',
>>      'media_device.h',
>>      'media_object.h',
>>      'pipeline_handler.h',
>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
>> index 21373fa25bf8..e67cfa2b363e 100644
>> --- a/src/android/camera_buffer.h
>> +++ b/src/android/camera_buffer.h
>> @@ -7,6 +7,8 @@
>>  #ifndef __ANDROID_CAMERA_BUFFER_H__
>>  #define __ANDROID_CAMERA_BUFFER_H__
>>
>> +#include <sys/mman.h>
>> +
>>  #include <hardware/camera3.h>
>>
>>  #include <libcamera/base/class.h>
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 089a6204605e..dd9aebba7553 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -24,8 +24,6 @@
>>  #include <libcamera/request.h>
>>  #include <libcamera/stream.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> -
>>  #include "camera_capabilities.h"
>>  #include "camera_metadata.h"
>>  #include "camera_stream.h"
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 629d9e00e08d..2dab6c3a37d1 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -19,8 +19,6 @@
>>  #include <libcamera/geometry.h>
>>  #include <libcamera/pixel_format.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> -
>>  class CameraDevice;
>>  class CameraMetadata;
>>  class PostProcessor;
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
>> index e6358ca9466f..372018d2207f 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -23,6 +23,7 @@
>>  #include <libcamera/pixel_format.h>
>>
>>  #include "libcamera/internal/formats.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  using namespace libcamera;
>>
>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
>> index 14bf89223982..61fbd1a69278 100644
>> --- a/src/android/jpeg/encoder_libjpeg.h
>> +++ b/src/android/jpeg/encoder_libjpeg.h
>> @@ -10,7 +10,6 @@
>>  #include "encoder.h"
>>
>>  #include "libcamera/internal/formats.h"
>> -#include "libcamera/internal/framebuffer.h"
>>
>>  #include <jpeglib.h>
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 5c399be9eb6a..6fd3102229ab 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -13,8 +13,6 @@
>>
>>  #include <libcamera/geometry.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> -
>>  class CameraDevice;
>>
>>  class PostProcessorJpeg : public PostProcessor
>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>> index 5cb00744a688..535e2cece914 100644
>> --- a/src/android/jpeg/thumbnailer.cpp
>> +++ b/src/android/jpeg/thumbnailer.cpp
>> @@ -11,6 +11,8 @@
>>
>>  #include <libcamera/formats.h>
>>
>> +#include "libcamera/internal/mapped_framebuffer.h"
>> +
>>  using namespace libcamera;
>>
>>  LOG_DEFINE_CATEGORY(Thumbnailer)
>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
>> index 68cbf74329a9..4d086c4943b0 100644
>> --- a/src/android/jpeg/thumbnailer.h
>> +++ b/src/android/jpeg/thumbnailer.h
>> @@ -7,10 +7,10 @@
>>  #ifndef __ANDROID_JPEG_THUMBNAILER_H__
>>  #define __ANDROID_JPEG_THUMBNAILER_H__
>>
>> +#include <libcamera/framebuffer.h>
>>  #include <libcamera/geometry.h>
>>
>>  #include "libcamera/internal/formats.h"
>> -#include "libcamera/internal/framebuffer.h"
>>
>>  class Thumbnailer
>>  {
>> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
>> index 2a4b77ea5236..0c8adfd999b9 100644
>> --- a/src/android/mm/generic_camera_buffer.cpp
>> +++ b/src/android/mm/generic_camera_buffer.cpp
>> @@ -11,7 +11,7 @@
>>
>>  #include <libcamera/base/log.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  using namespace libcamera;
>>
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 689f85d9d3b8..ab2b2c606fd0 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -10,8 +10,6 @@
>>  #include <libcamera/framebuffer.h>
>>  #include <libcamera/stream.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> -
>>  #include "camera_buffer.h"
>>
>>  class CameraMetadata;
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index 772e805b32cc..509d4244d614 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -16,6 +16,7 @@
>>  #include <libcamera/pixel_format.h>
>>
>>  #include "libcamera/internal/formats.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  using namespace libcamera;
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 71698d36e50f..2647bf2f3b96 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -20,7 +20,7 @@
>>  #include <libcamera/ipa/ipu3_ipa_interface.h>
>>  #include <libcamera/request.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  #include "ipu3_agc.h"
>>  #include "ipu3_awb.h"
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 48e11c699f29..76f67dd4567a 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -28,7 +28,7 @@
>>  #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>>  #include <libcamera/request.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  #include "agc_algorithm.hpp"
>>  #include "agc_status.h"
>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
>> index a59e93fbdf91..3d98affb20f9 100644
>> --- a/src/libcamera/framebuffer.cpp
>> +++ b/src/libcamera/framebuffer.cpp
>> @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const
>>   * indicate that the metadata is invalid.
>>   */
>>
>> -/**
>> - * \class MappedBuffer
>> - * \brief Provide an interface to support managing memory mapped buffers
>> - *
>> - * The MappedBuffer interface provides access to a set of MappedPlanes which
>> - * are available for access by the CPU.
>> - *
>> - * This class is not meant to be constructed directly, but instead derived
>> - * classes should be used to implement the correct mapping of a source buffer.
>> - *
>> - * This allows treating CPU accessible memory through a generic interface
>> - * regardless of whether it originates from a libcamera FrameBuffer or other
>> - * source.
>> - */
>> -
>> -/**
>> - * \typedef MappedBuffer::Plane
>> - * \brief A mapped region of memory accessible to the CPU
>> - *
>> - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
>> - * region.
>> - */
>> -
>> -/**
>> - * \brief Construct an empty MappedBuffer
>> - */
>> -MappedBuffer::MappedBuffer()
>> -       : error_(0)
>> -{
>> -}
>> -
>> -/**
>> - * \brief Move constructor, construct the MappedBuffer with the contents of \a
>> - * other using move semantics
>> - * \param[in] other The other MappedBuffer
>> - *
>> - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
>> - * MappedBuffer and invalidates the \a other.
>> - *
>> - * No mappings are unmapped or destroyed in this process.
>> - */
>> -MappedBuffer::MappedBuffer(MappedBuffer &&other)
>> -{
>> -       *this = std::move(other);
>> -}
>> -
>> -/**
>> - * \brief Move assignment operator, replace the mappings with those of \a other
>> -* \param[in] other The other MappedBuffer
>> - *
>> - * Moving a MappedBuffer moves the mappings contained in the \a other to the new
>> - * MappedBuffer and invalidates the \a other.
>> - *
>> - * No mappings are unmapped or destroyed in this process.
>> - */
>> -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>> -{
>> -       error_ = other.error_;
>> -       maps_ = std::move(other.maps_);
>> -       other.error_ = -ENOENT;
>> -
>> -       return *this;
>> -}
>> -
>> -MappedBuffer::~MappedBuffer()
>> -{
>> -       for (Plane &map : maps_)
>> -               munmap(map.data(), map.size());
>> -}
>> -
>> -/**
>> - * \fn MappedBuffer::isValid()
>> - * \brief Check if the MappedBuffer instance is valid
>> - * \return True if the MappedBuffer has valid mappings, false otherwise
>> - */
>> -
>> -/**
>> - * \fn MappedBuffer::error()
>> - * \brief Retrieve the map error status
>> - *
>> - * This function retrieves the error status from the MappedBuffer.
>> - * The error status is a negative number as defined by errno.h. If
>> - * no error occurred, this function returns 0.
>> - *
>> - * \return The map error code
>> - */
>> -
>> -/**
>> - * \fn MappedBuffer::maps()
>> - * \brief Retrieve the mapped planes
>> - *
>> - * This function retrieves the successfully mapped planes stored as a vector
>> - * of Span<uint8_t> to provide access to the mapped memory.
>> - *
>> - * \return A vector of the mapped planes
>> - */
>> -
>> -/**
>> - * \var MappedBuffer::error_
>> - * \brief Stores the error value if present
>> - *
>> - * MappedBuffer derived classes shall set this to a negative value as defined
>> - * by errno.h if an error occured during the mapping process.
>> - */
>> -
>> -/**
>> - * \var MappedBuffer::maps_
>> - * \brief Stores the internal mapped planes
>> - *
>> - * MappedBuffer derived classes shall store the mappings they create in this
>> - * vector which is parsed during destruct to unmap any memory mappings which
>> - * completed successfully.
>> - */
>> -
>> -/**
>> - * \class MappedFrameBuffer
>> - * \brief Map a FrameBuffer using the MappedBuffer interface
>> - */
>> -
>> -/**
>> - * \brief Map all planes of a FrameBuffer
>> - * \param[in] buffer FrameBuffer to be mapped
>> - * \param[in] flags Protection flags to apply to map
>> - *
>> - * Construct an object to map a frame buffer for CPU access.
>> - * The flags are passed directly to mmap and should be either PROT_READ,
>> - * PROT_WRITE, or a bitwise-or combination of both.
>> - */
>> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>> -{
>> -       maps_.reserve(buffer->planes().size());
>> -
>> -       for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> -               void *address = mmap(nullptr, plane.length, flags,
>> -                                    MAP_SHARED, plane.fd.fd(), 0);
>> -               if (address == MAP_FAILED) {
>> -                       error_ = -errno;
>> -                       LOG(Buffer, Error) << "Failed to mmap plane";
>> -                       break;
>> -               }
>> -
>> -               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
>> -       }
>> -}
>> -
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
>> new file mode 100644
>> index 000000000000..0e30fc542154
>> --- /dev/null
>> +++ b/src/libcamera/mapped_framebuffer.cpp
>> @@ -0,0 +1,171 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google Inc.
>> + *
>> + * mapped_framebuffer.cpp - Mapped Framebuffer support
>> + */
>> +
>> +#include "libcamera/internal/mapped_framebuffer.h"
>> +
>> +#include <errno.h>
>> +#include <string.h>
>> +#include <sys/mman.h>
>> +#include <unistd.h>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +/**
>> + * \file libcamera/internal/mapped_framebuffer.h
>> + * \brief Frame buffer memory mapping support
>> + */
>> +
>> +namespace libcamera {
>> +
>> +LOG_DECLARE_CATEGORY(Buffer)
>> +
>> +/**
>> + * \class MappedBuffer
>> + * \brief Provide an interface to support managing memory mapped buffers
>> + *
>> + * The MappedBuffer interface provides access to a set of MappedPlanes which
>> + * are available for access by the CPU.
>> + *
>> + * This class is not meant to be constructed directly, but instead derived
>> + * classes should be used to implement the correct mapping of a source buffer.
>> + *
>> + * This allows treating CPU accessible memory through a generic interface
>> + * regardless of whether it originates from a libcamera FrameBuffer or other
>> + * source.
>> + */
>> +
>> +/**
>> + * \typedef MappedBuffer::Plane
>> + * \brief A mapped region of memory accessible to the CPU
>> + *
>> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
>> + * region.
>> + */
>> +
>> +/**
>> + * \brief Construct an empty MappedBuffer
>> + */
>> +MappedBuffer::MappedBuffer()
>> +       : error_(0)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Move constructor, construct the MappedBuffer with the contents of \a
>> + * other using move semantics
>> + * \param[in] other The other MappedBuffer
>> + *
>> + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
>> + * MappedBuffer and invalidates the \a other.
>> + *
>> + * No mappings are unmapped or destroyed in this process.
>> + */
>> +MappedBuffer::MappedBuffer(MappedBuffer &&other)
>> +{
>> +       *this = std::move(other);
>> +}
>> +
>> +/**
>> + * \brief Move assignment operator, replace the mappings with those of \a other
>> +* \param[in] other The other MappedBuffer
>> + *
>> + * Moving a MappedBuffer moves the mappings contained in the \a other to the new
>> + * MappedBuffer and invalidates the \a other.
>> + *
>> + * No mappings are unmapped or destroyed in this process.
>> + */
>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>> +{
>> +       error_ = other.error_;
>> +       maps_ = std::move(other.maps_);
>> +       other.error_ = -ENOENT;
>> +
>> +       return *this;
>> +}
>> +
>> +MappedBuffer::~MappedBuffer()
>> +{
>> +       for (Plane &map : maps_)
>> +               munmap(map.data(), map.size());
>> +}
>> +
>> +/**
>> + * \fn MappedBuffer::isValid()
>> + * \brief Check if the MappedBuffer instance is valid
>> + * \return True if the MappedBuffer has valid mappings, false otherwise
>> + */
>> +
>> +/**
>> + * \fn MappedBuffer::error()
>> + * \brief Retrieve the map error status
>> + *
>> + * This function retrieves the error status from the MappedBuffer.
>> + * The error status is a negative number as defined by errno.h. If
>> + * no error occurred, this function returns 0.
>> + *
>> + * \return The map error code
>> + */
>> +
>> +/**
>> + * \fn MappedBuffer::maps()
>> + * \brief Retrieve the mapped planes
>> + *
>> + * This function retrieves the successfully mapped planes stored as a vector
>> + * of Span<uint8_t> to provide access to the mapped memory.
>> + *
>> + * \return A vector of the mapped planes
>> + */
>> +
>> +/**
>> + * \var MappedBuffer::error_
>> + * \brief Stores the error value if present
>> + *
>> + * MappedBuffer derived classes shall set this to a negative value as defined
>> + * by errno.h if an error occured during the mapping process.
>> + */
>> +
>> +/**
>> + * \var MappedBuffer::maps_
>> + * \brief Stores the internal mapped planes
>> + *
>> + * MappedBuffer derived classes shall store the mappings they create in this
>> + * vector which is parsed during destruct to unmap any memory mappings which
>> + * completed successfully.
>> + */
>> +
>> +/**
>> + * \class MappedFrameBuffer
>> + * \brief Map a FrameBuffer using the MappedBuffer interface
>> + */
>> +
>> +/**
>> + * \brief Map all planes of a FrameBuffer
>> + * \param[in] buffer FrameBuffer to be mapped
>> + * \param[in] flags Protection flags to apply to map
>> + *
>> + * Construct an object to map a frame buffer for CPU access.
>> + * The flags are passed directly to mmap and should be either PROT_READ,
>> + * PROT_WRITE, or a bitwise-or combination of both.
>> + */
>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>> +{
>> +       maps_.reserve(buffer->planes().size());
>> +
>> +       for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> +               void *address = mmap(nullptr, plane.length, flags,
>> +                                    MAP_SHARED, plane.fd.fd(), 0);
>> +               if (address == MAP_FAILED) {
>> +                       error_ = -errno;
>> +                       LOG(Buffer, Error) << "Failed to mmap plane";
>> +                       break;
>> +               }
>> +
>> +               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
>> +       }
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 4f08580157f9..e9230b983aeb 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -28,6 +28,7 @@ libcamera_sources = files([
>>      'ipc_pipe.cpp',
>>      'ipc_pipe_unixsocket.cpp',
>>      'ipc_unixsocket.cpp',
>> +    'mapped_framebuffer.cpp',
>>      'media_device.cpp',
>>      'media_object.cpp',
>>      'pipeline_handler.cpp',
>> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
>> index c9479194cb68..a3d1511b74ce 100644
>> --- a/test/mapped-buffer.cpp
>> +++ b/test/mapped-buffer.cpp
>> @@ -9,7 +9,7 @@
>>
>>  #include <libcamera/framebuffer_allocator.h>
>>
>> -#include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>>
>>  #include "camera_test.h"
>>  #include "test.h"
>> --
>> 2.30.2
>>
Kieran Bingham Aug. 6, 2021, 10:56 a.m. UTC | #4 
Hi Laurent,

On 06/08/2021 11:37, Laurent Pinchart wrote:
<snip>

>>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>>> @@ -0,0 +1,52 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Google Inc.
>>> + *
>>> + * mapped_framebuffer.h - Frame buffer memory mapping support
>>> + */
>>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>> +
>>> +#include <sys/mman.h>
> 
> This isn't needed anymore, but you need stdint.h instead.

Users of MappedFrameBuffer need to specify the mapping flags.

We can abstract that with your new Flags class now if you prefer ?


<snip>


>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>>> index dac1a2d36fa8..665fd6de4ed3 100644
>>> --- a/include/libcamera/internal/meson.build
>>> +++ b/include/libcamera/internal/meson.build
>>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
>>>      'ipa_module.h',
>>>      'ipa_proxy.h',
>>>      'ipc_unixsocket.h',
>>> +    'mapped_framebuffer.h',
>>>      'media_device.h',
>>>      'media_object.h',
>>>      'pipeline_handler.h',
>>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
>>> index 21373fa25bf8..e67cfa2b363e 100644
>>> --- a/src/android/camera_buffer.h
>>> +++ b/src/android/camera_buffer.h
>>> @@ -7,6 +7,8 @@
>>>  #ifndef __ANDROID_CAMERA_BUFFER_H__
>>>  #define __ANDROID_CAMERA_BUFFER_H__
>>>
>>> +#include <sys/mman.h>
>>> +

That's also why it is added here ...


>>>  #include <hardware/camera3.h>
>>>
>>>  #include <libcamera/base/class.h>

<snip>
Laurent Pinchart Aug. 6, 2021, 11:06 a.m. UTC | #5 
Hi Kieran,

On Fri, Aug 06, 2021 at 11:56:40AM +0100, Kieran Bingham wrote:
> On 06/08/2021 11:37, Laurent Pinchart wrote:
> <snip>
> 
> >>> +++ b/include/libcamera/internal/mapped_framebuffer.h
> >>> @@ -0,0 +1,52 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2021, Google Inc.
> >>> + *
> >>> + * mapped_framebuffer.h - Frame buffer memory mapping support
> >>> + */
> >>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> >>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
> >>> +
> >>> +#include <sys/mman.h>
> > 
> > This isn't needed anymore, but you need stdint.h instead.
> 
> Users of MappedFrameBuffer need to specify the mapping flags.

It's not required by mapped_framebuffer.h, but it's indeed not nice to
for all users to include it.

> We can abstract that with your new Flags class now if you prefer ?

If we want to make this class public, then I'd prefer not depending on
sys/mman.h indeed. We can add a custom scoped enum (and use the newly
merged Flags<> class :-)).

> >>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> >>> index dac1a2d36fa8..665fd6de4ed3 100644
> >>> --- a/include/libcamera/internal/meson.build
> >>> +++ b/include/libcamera/internal/meson.build
> >>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
> >>>      'ipa_module.h',
> >>>      'ipa_proxy.h',
> >>>      'ipc_unixsocket.h',
> >>> +    'mapped_framebuffer.h',
> >>>      'media_device.h',
> >>>      'media_object.h',
> >>>      'pipeline_handler.h',
> >>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> >>> index 21373fa25bf8..e67cfa2b363e 100644
> >>> --- a/src/android/camera_buffer.h
> >>> +++ b/src/android/camera_buffer.h
> >>> @@ -7,6 +7,8 @@
> >>>  #ifndef __ANDROID_CAMERA_BUFFER_H__
> >>>  #define __ANDROID_CAMERA_BUFFER_H__
> >>>
> >>> +#include <sys/mman.h>
> >>> +
> 
> That's also why it is added here ...
> 
> >>>  #include <hardware/camera3.h>
> >>>
> >>>  #include <libcamera/base/class.h>
Kieran Bingham Aug. 6, 2021, 1:15 p.m. UTC | #6 
Hi Laurent,

On 06/08/2021 12:06, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Aug 06, 2021 at 11:56:40AM +0100, Kieran Bingham wrote:
>> On 06/08/2021 11:37, Laurent Pinchart wrote:
>> <snip>
>>
>>>>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>>>>> @@ -0,0 +1,52 @@
>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2021, Google Inc.
>>>>> + *
>>>>> + * mapped_framebuffer.h - Frame buffer memory mapping support
>>>>> + */
>>>>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>>>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>>>> +
>>>>> +#include <sys/mman.h>
>>>
>>> This isn't needed anymore, but you need stdint.h instead.
>>
>> Users of MappedFrameBuffer need to specify the mapping flags.
> 
> It's not required by mapped_framebuffer.h, but it's indeed not nice to
> for all users to include it.
> 
>> We can abstract that with your new Flags class now if you prefer ?
> 
> If we want to make this class public, then I'd prefer not depending on
> sys/mman.h indeed. We can add a custom scoped enum (and use the newly
> merged Flags<> class :-)).

I've done all this on top, which I think makes more sense.
 (including picking up the stdint.h).

See the recently posted
 "libcamera: MappedFrameBuffer:: Use typed Flags<MapModes>"

--
Kieran


>>>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>>>>> index dac1a2d36fa8..665fd6de4ed3 100644
>>>>> --- a/include/libcamera/internal/meson.build
>>>>> +++ b/include/libcamera/internal/meson.build
>>>>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([
>>>>>      'ipa_module.h',
>>>>>      'ipa_proxy.h',
>>>>>      'ipc_unixsocket.h',
>>>>> +    'mapped_framebuffer.h',
>>>>>      'media_device.h',
>>>>>      'media_object.h',
>>>>>      'pipeline_handler.h',
>>>>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
>>>>> index 21373fa25bf8..e67cfa2b363e 100644
>>>>> --- a/src/android/camera_buffer.h
>>>>> +++ b/src/android/camera_buffer.h
>>>>> @@ -7,6 +7,8 @@
>>>>>  #ifndef __ANDROID_CAMERA_BUFFER_H__
>>>>>  #define __ANDROID_CAMERA_BUFFER_H__
>>>>>
>>>>> +#include <sys/mman.h>
>>>>> +
>>
>> That's also why it is added here ...
>>
>>>>>  #include <hardware/camera3.h>
>>>>>
>>>>>  #include <libcamera/base/class.h>
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
index 8c187adf70c7..1352578a6cfb 100644
--- a/include/libcamera/internal/framebuffer.h
+++ b/include/libcamera/internal/framebuffer.h
@@ -7,46 +7,10 @@ 
 #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
 #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__
 
-#include <sys/mman.h>
-#include <vector>
-
-#include <libcamera/base/class.h>
-#include <libcamera/base/span.h>
-
 #include <libcamera/framebuffer.h>
 
 namespace libcamera {
 
-class MappedBuffer
-{
-public:
-	using Plane = Span<uint8_t>;
-
-	~MappedBuffer();
-
-	MappedBuffer(MappedBuffer &&other);
-	MappedBuffer &operator=(MappedBuffer &&other);
-
-	bool isValid() const { return error_ == 0; }
-	int error() const { return error_; }
-	const std::vector<Plane> &maps() const { return maps_; }
-
-protected:
-	MappedBuffer();
-
-	int error_;
-	std::vector<Plane> maps_;
-
-private:
-	LIBCAMERA_DISABLE_COPY(MappedBuffer)
-};
-
-class MappedFrameBuffer : public MappedBuffer
-{
-public:
-	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
-};
-
 class FrameBuffer::Private : public Extensible::Private
 {
 	LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
new file mode 100644
index 000000000000..41e587364260
--- /dev/null
+++ b/include/libcamera/internal/mapped_framebuffer.h
@@ -0,0 +1,52 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * mapped_framebuffer.h - Frame buffer memory mapping support
+ */
+#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
+#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
+
+#include <sys/mman.h>
+#include <vector>
+
+#include <libcamera/base/class.h>
+#include <libcamera/base/span.h>
+
+#include <libcamera/framebuffer.h>
+
+namespace libcamera {
+
+class MappedBuffer
+{
+public:
+	using Plane = Span<uint8_t>;
+
+	~MappedBuffer();
+
+	MappedBuffer(MappedBuffer &&other);
+	MappedBuffer &operator=(MappedBuffer &&other);
+
+	bool isValid() const { return error_ == 0; }
+	int error() const { return error_; }
+	const std::vector<Plane> &maps() const { return maps_; }
+
+protected:
+	MappedBuffer();
+
+	int error_;
+	std::vector<Plane> maps_;
+
+private:
+	LIBCAMERA_DISABLE_COPY(MappedBuffer)
+};
+
+class MappedFrameBuffer : public MappedBuffer
+{
+public:
+	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
+};
+
+} /* namespace libcamera */
+
+#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
index dac1a2d36fa8..665fd6de4ed3 100644
--- a/include/libcamera/internal/meson.build
+++ b/include/libcamera/internal/meson.build
@@ -28,6 +28,7 @@  libcamera_internal_headers = files([
     'ipa_module.h',
     'ipa_proxy.h',
     'ipc_unixsocket.h',
+    'mapped_framebuffer.h',
     'media_device.h',
     'media_object.h',
     'pipeline_handler.h',
diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
index 21373fa25bf8..e67cfa2b363e 100644
--- a/src/android/camera_buffer.h
+++ b/src/android/camera_buffer.h
@@ -7,6 +7,8 @@ 
 #ifndef __ANDROID_CAMERA_BUFFER_H__
 #define __ANDROID_CAMERA_BUFFER_H__
 
+#include <sys/mman.h>
+
 #include <hardware/camera3.h>
 
 #include <libcamera/base/class.h>
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 089a6204605e..dd9aebba7553 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -24,8 +24,6 @@ 
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
-#include "libcamera/internal/framebuffer.h"
-
 #include "camera_capabilities.h"
 #include "camera_metadata.h"
 #include "camera_stream.h"
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 629d9e00e08d..2dab6c3a37d1 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -19,8 +19,6 @@ 
 #include <libcamera/geometry.h>
 #include <libcamera/pixel_format.h>
 
-#include "libcamera/internal/framebuffer.h"
-
 class CameraDevice;
 class CameraMetadata;
 class PostProcessor;
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index e6358ca9466f..372018d2207f 100644
--- a/src/android/jpeg/encoder_libjpeg.cpp
+++ b/src/android/jpeg/encoder_libjpeg.cpp
@@ -23,6 +23,7 @@ 
 #include <libcamera/pixel_format.h>
 
 #include "libcamera/internal/formats.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 using namespace libcamera;
 
diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
index 14bf89223982..61fbd1a69278 100644
--- a/src/android/jpeg/encoder_libjpeg.h
+++ b/src/android/jpeg/encoder_libjpeg.h
@@ -10,7 +10,6 @@ 
 #include "encoder.h"
 
 #include "libcamera/internal/formats.h"
-#include "libcamera/internal/framebuffer.h"
 
 #include <jpeglib.h>
 
diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
index 5c399be9eb6a..6fd3102229ab 100644
--- a/src/android/jpeg/post_processor_jpeg.h
+++ b/src/android/jpeg/post_processor_jpeg.h
@@ -13,8 +13,6 @@ 
 
 #include <libcamera/geometry.h>
 
-#include "libcamera/internal/framebuffer.h"
-
 class CameraDevice;
 
 class PostProcessorJpeg : public PostProcessor
diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
index 5cb00744a688..535e2cece914 100644
--- a/src/android/jpeg/thumbnailer.cpp
+++ b/src/android/jpeg/thumbnailer.cpp
@@ -11,6 +11,8 @@ 
 
 #include <libcamera/formats.h>
 
+#include "libcamera/internal/mapped_framebuffer.h"
+
 using namespace libcamera;
 
 LOG_DEFINE_CATEGORY(Thumbnailer)
diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
index 68cbf74329a9..4d086c4943b0 100644
--- a/src/android/jpeg/thumbnailer.h
+++ b/src/android/jpeg/thumbnailer.h
@@ -7,10 +7,10 @@ 
 #ifndef __ANDROID_JPEG_THUMBNAILER_H__
 #define __ANDROID_JPEG_THUMBNAILER_H__
 
+#include <libcamera/framebuffer.h>
 #include <libcamera/geometry.h>
 
 #include "libcamera/internal/formats.h"
-#include "libcamera/internal/framebuffer.h"
 
 class Thumbnailer
 {
diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
index 2a4b77ea5236..0c8adfd999b9 100644
--- a/src/android/mm/generic_camera_buffer.cpp
+++ b/src/android/mm/generic_camera_buffer.cpp
@@ -11,7 +11,7 @@ 
 
 #include <libcamera/base/log.h>
 
-#include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 using namespace libcamera;
 
diff --git a/src/android/post_processor.h b/src/android/post_processor.h
index 689f85d9d3b8..ab2b2c606fd0 100644
--- a/src/android/post_processor.h
+++ b/src/android/post_processor.h
@@ -10,8 +10,6 @@ 
 #include <libcamera/framebuffer.h>
 #include <libcamera/stream.h>
 
-#include "libcamera/internal/framebuffer.h"
-
 #include "camera_buffer.h"
 
 class CameraMetadata;
diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
index 772e805b32cc..509d4244d614 100644
--- a/src/android/yuv/post_processor_yuv.cpp
+++ b/src/android/yuv/post_processor_yuv.cpp
@@ -16,6 +16,7 @@ 
 #include <libcamera/pixel_format.h>
 
 #include "libcamera/internal/formats.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 using namespace libcamera;
 
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 71698d36e50f..2647bf2f3b96 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -20,7 +20,7 @@ 
 #include <libcamera/ipa/ipu3_ipa_interface.h>
 #include <libcamera/request.h>
 
-#include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 #include "ipu3_agc.h"
 #include "ipu3_awb.h"
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 48e11c699f29..76f67dd4567a 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -28,7 +28,7 @@ 
 #include <libcamera/ipa/raspberrypi_ipa_interface.h>
 #include <libcamera/request.h>
 
-#include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 #include "agc_algorithm.hpp"
 #include "agc_status.h"
diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
index a59e93fbdf91..3d98affb20f9 100644
--- a/src/libcamera/framebuffer.cpp
+++ b/src/libcamera/framebuffer.cpp
@@ -251,149 +251,4 @@  Request *FrameBuffer::request() const
  * indicate that the metadata is invalid.
  */
 
-/**
- * \class MappedBuffer
- * \brief Provide an interface to support managing memory mapped buffers
- *
- * The MappedBuffer interface provides access to a set of MappedPlanes which
- * are available for access by the CPU.
- *
- * This class is not meant to be constructed directly, but instead derived
- * classes should be used to implement the correct mapping of a source buffer.
- *
- * This allows treating CPU accessible memory through a generic interface
- * regardless of whether it originates from a libcamera FrameBuffer or other
- * source.
- */
-
-/**
- * \typedef MappedBuffer::Plane
- * \brief A mapped region of memory accessible to the CPU
- *
- * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
- * region.
- */
-
-/**
- * \brief Construct an empty MappedBuffer
- */
-MappedBuffer::MappedBuffer()
-	: error_(0)
-{
-}
-
-/**
- * \brief Move constructor, construct the MappedBuffer with the contents of \a
- * other using move semantics
- * \param[in] other The other MappedBuffer
- *
- * Moving a MappedBuffer moves the mappings contained in the \a other to the new
- * MappedBuffer and invalidates the \a other.
- *
- * No mappings are unmapped or destroyed in this process.
- */
-MappedBuffer::MappedBuffer(MappedBuffer &&other)
-{
-	*this = std::move(other);
-}
-
-/**
- * \brief Move assignment operator, replace the mappings with those of \a other
-* \param[in] other The other MappedBuffer
- *
- * Moving a MappedBuffer moves the mappings contained in the \a other to the new
- * MappedBuffer and invalidates the \a other.
- *
- * No mappings are unmapped or destroyed in this process.
- */
-MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
-{
-	error_ = other.error_;
-	maps_ = std::move(other.maps_);
-	other.error_ = -ENOENT;
-
-	return *this;
-}
-
-MappedBuffer::~MappedBuffer()
-{
-	for (Plane &map : maps_)
-		munmap(map.data(), map.size());
-}
-
-/**
- * \fn MappedBuffer::isValid()
- * \brief Check if the MappedBuffer instance is valid
- * \return True if the MappedBuffer has valid mappings, false otherwise
- */
-
-/**
- * \fn MappedBuffer::error()
- * \brief Retrieve the map error status
- *
- * This function retrieves the error status from the MappedBuffer.
- * The error status is a negative number as defined by errno.h. If
- * no error occurred, this function returns 0.
- *
- * \return The map error code
- */
-
-/**
- * \fn MappedBuffer::maps()
- * \brief Retrieve the mapped planes
- *
- * This function retrieves the successfully mapped planes stored as a vector
- * of Span<uint8_t> to provide access to the mapped memory.
- *
- * \return A vector of the mapped planes
- */
-
-/**
- * \var MappedBuffer::error_
- * \brief Stores the error value if present
- *
- * MappedBuffer derived classes shall set this to a negative value as defined
- * by errno.h if an error occured during the mapping process.
- */
-
-/**
- * \var MappedBuffer::maps_
- * \brief Stores the internal mapped planes
- *
- * MappedBuffer derived classes shall store the mappings they create in this
- * vector which is parsed during destruct to unmap any memory mappings which
- * completed successfully.
- */
-
-/**
- * \class MappedFrameBuffer
- * \brief Map a FrameBuffer using the MappedBuffer interface
- */
-
-/**
- * \brief Map all planes of a FrameBuffer
- * \param[in] buffer FrameBuffer to be mapped
- * \param[in] flags Protection flags to apply to map
- *
- * Construct an object to map a frame buffer for CPU access.
- * The flags are passed directly to mmap and should be either PROT_READ,
- * PROT_WRITE, or a bitwise-or combination of both.
- */
-MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
-{
-	maps_.reserve(buffer->planes().size());
-
-	for (const FrameBuffer::Plane &plane : buffer->planes()) {
-		void *address = mmap(nullptr, plane.length, flags,
-				     MAP_SHARED, plane.fd.fd(), 0);
-		if (address == MAP_FAILED) {
-			error_ = -errno;
-			LOG(Buffer, Error) << "Failed to mmap plane";
-			break;
-		}
-
-		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
-	}
-}
-
 } /* namespace libcamera */
diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
new file mode 100644
index 000000000000..0e30fc542154
--- /dev/null
+++ b/src/libcamera/mapped_framebuffer.cpp
@@ -0,0 +1,171 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google Inc.
+ *
+ * mapped_framebuffer.cpp - Mapped Framebuffer support
+ */
+
+#include "libcamera/internal/mapped_framebuffer.h"
+
+#include <errno.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include <libcamera/base/log.h>
+
+/**
+ * \file libcamera/internal/mapped_framebuffer.h
+ * \brief Frame buffer memory mapping support
+ */
+
+namespace libcamera {
+
+LOG_DECLARE_CATEGORY(Buffer)
+
+/**
+ * \class MappedBuffer
+ * \brief Provide an interface to support managing memory mapped buffers
+ *
+ * The MappedBuffer interface provides access to a set of MappedPlanes which
+ * are available for access by the CPU.
+ *
+ * This class is not meant to be constructed directly, but instead derived
+ * classes should be used to implement the correct mapping of a source buffer.
+ *
+ * This allows treating CPU accessible memory through a generic interface
+ * regardless of whether it originates from a libcamera FrameBuffer or other
+ * source.
+ */
+
+/**
+ * \typedef MappedBuffer::Plane
+ * \brief A mapped region of memory accessible to the CPU
+ *
+ * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
+ * region.
+ */
+
+/**
+ * \brief Construct an empty MappedBuffer
+ */
+MappedBuffer::MappedBuffer()
+	: error_(0)
+{
+}
+
+/**
+ * \brief Move constructor, construct the MappedBuffer with the contents of \a
+ * other using move semantics
+ * \param[in] other The other MappedBuffer
+ *
+ * Moving a MappedBuffer moves the mappings contained in the \a other to the new
+ * MappedBuffer and invalidates the \a other.
+ *
+ * No mappings are unmapped or destroyed in this process.
+ */
+MappedBuffer::MappedBuffer(MappedBuffer &&other)
+{
+	*this = std::move(other);
+}
+
+/**
+ * \brief Move assignment operator, replace the mappings with those of \a other
+* \param[in] other The other MappedBuffer
+ *
+ * Moving a MappedBuffer moves the mappings contained in the \a other to the new
+ * MappedBuffer and invalidates the \a other.
+ *
+ * No mappings are unmapped or destroyed in this process.
+ */
+MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
+{
+	error_ = other.error_;
+	maps_ = std::move(other.maps_);
+	other.error_ = -ENOENT;
+
+	return *this;
+}
+
+MappedBuffer::~MappedBuffer()
+{
+	for (Plane &map : maps_)
+		munmap(map.data(), map.size());
+}
+
+/**
+ * \fn MappedBuffer::isValid()
+ * \brief Check if the MappedBuffer instance is valid
+ * \return True if the MappedBuffer has valid mappings, false otherwise
+ */
+
+/**
+ * \fn MappedBuffer::error()
+ * \brief Retrieve the map error status
+ *
+ * This function retrieves the error status from the MappedBuffer.
+ * The error status is a negative number as defined by errno.h. If
+ * no error occurred, this function returns 0.
+ *
+ * \return The map error code
+ */
+
+/**
+ * \fn MappedBuffer::maps()
+ * \brief Retrieve the mapped planes
+ *
+ * This function retrieves the successfully mapped planes stored as a vector
+ * of Span<uint8_t> to provide access to the mapped memory.
+ *
+ * \return A vector of the mapped planes
+ */
+
+/**
+ * \var MappedBuffer::error_
+ * \brief Stores the error value if present
+ *
+ * MappedBuffer derived classes shall set this to a negative value as defined
+ * by errno.h if an error occured during the mapping process.
+ */
+
+/**
+ * \var MappedBuffer::maps_
+ * \brief Stores the internal mapped planes
+ *
+ * MappedBuffer derived classes shall store the mappings they create in this
+ * vector which is parsed during destruct to unmap any memory mappings which
+ * completed successfully.
+ */
+
+/**
+ * \class MappedFrameBuffer
+ * \brief Map a FrameBuffer using the MappedBuffer interface
+ */
+
+/**
+ * \brief Map all planes of a FrameBuffer
+ * \param[in] buffer FrameBuffer to be mapped
+ * \param[in] flags Protection flags to apply to map
+ *
+ * Construct an object to map a frame buffer for CPU access.
+ * The flags are passed directly to mmap and should be either PROT_READ,
+ * PROT_WRITE, or a bitwise-or combination of both.
+ */
+MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
+{
+	maps_.reserve(buffer->planes().size());
+
+	for (const FrameBuffer::Plane &plane : buffer->planes()) {
+		void *address = mmap(nullptr, plane.length, flags,
+				     MAP_SHARED, plane.fd.fd(), 0);
+		if (address == MAP_FAILED) {
+			error_ = -errno;
+			LOG(Buffer, Error) << "Failed to mmap plane";
+			break;
+		}
+
+		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
+	}
+}
+
+} /* namespace libcamera */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index 4f08580157f9..e9230b983aeb 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -28,6 +28,7 @@  libcamera_sources = files([
     'ipc_pipe.cpp',
     'ipc_pipe_unixsocket.cpp',
     'ipc_unixsocket.cpp',
+    'mapped_framebuffer.cpp',
     'media_device.cpp',
     'media_object.cpp',
     'pipeline_handler.cpp',
diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
index c9479194cb68..a3d1511b74ce 100644
--- a/test/mapped-buffer.cpp
+++ b/test/mapped-buffer.cpp
@@ -9,7 +9,7 @@ 
 
 #include <libcamera/framebuffer_allocator.h>
 
-#include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/mapped_framebuffer.h"
 
 #include "camera_test.h"
 #include "test.h"