[{"id":18589,"web_url":"https://patchwork.libcamera.org/comment/18589/","msgid":"","date":"2021-08-06T10:29:57","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran, thank you for the patch.\n\nOn Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham\n wrote:\n>\n> The MappedFrameBuffer is a convenience feature which sits on top of the\n> FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.\n>\n> This implementation is internal and currently sits in the same internal\n> files as the internal FrameBuffer, thus exposing those internals to users\n> of the MappedFramebuffer implementation.\n>\n> Move the MappedFrameBuffer and MappedBuffer implementation to its own\n> implementation files, and fix the sources throughout to use that accordingly.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/framebuffer.h | 36 ----\n> .../libcamera/internal/mapped_framebuffer.h | 52 ++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/android/camera_buffer.h | 2 +\n> src/android/camera_device.h | 2 -\n> src/android/camera_stream.h | 2 -\n> src/android/jpeg/encoder_libjpeg.cpp | 1 +\n> src/android/jpeg/encoder_libjpeg.h | 1 -\n> src/android/jpeg/post_processor_jpeg.h | 2 -\n> src/android/jpeg/thumbnailer.cpp | 2 +\n> src/android/jpeg/thumbnailer.h | 2 +-\n> src/android/mm/generic_camera_buffer.cpp | 2 +-\n> src/android/post_processor.h | 2 -\n> src/android/yuv/post_processor_yuv.cpp | 1 +\n> src/ipa/ipu3/ipu3.cpp | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> src/libcamera/framebuffer.cpp | 145 ---------------\n> src/libcamera/mapped_framebuffer.cpp | 171 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> test/mapped-buffer.cpp | 2 +-\n> 20 files changed, 236 insertions(+), 195 deletions(-)\n> create mode 100644 include/libcamera/internal/mapped_framebuffer.h\n> create mode 100644 src/libcamera/mapped_framebuffer.cpp\n>\n\nI wonder if MappedFrameBuffer is so useful that it should be put in\nlibcamera/base.\nI found src/cam/frame_sink.cpp does mmap()/munmap, which is what\nMappedFrameBuffer does.\n\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index 8c187adf70c7..1352578a6cfb 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -7,46 +7,10 @@\n> #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n> #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n>\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -\n> #include \n>\n> namespace libcamera {\n>\n> -class MappedBuffer\n> -{\n> -public:\n> - using Plane = Span;\n> -\n> - ~MappedBuffer();\n> -\n> - MappedBuffer(MappedBuffer &&other);\n> - MappedBuffer &operator=(MappedBuffer &&other);\n> -\n> - bool isValid() const { return error_ == 0; }\n> - int error() const { return error_; }\n> - const std::vector &maps() const { return maps_; }\n> -\n> -protected:\n> - MappedBuffer();\n> -\n> - int error_;\n> - std::vector maps_;\n> -\n> -private:\n> - LIBCAMERA_DISABLE_COPY(MappedBuffer)\n> -};\n> -\n> -class MappedFrameBuffer : public MappedBuffer\n> -{\n> -public:\n> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> -};\n> -\n> class FrameBuffer::Private : public Extensible::Private\n> {\n> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> new file mode 100644\n> index 000000000000..41e587364260\n> --- /dev/null\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * mapped_framebuffer.h - Frame buffer memory mapping support\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class MappedBuffer\n> +{\n> +public:\n> + using Plane = Span;\n> +\n> + ~MappedBuffer();\n> +\n> + MappedBuffer(MappedBuffer &&other);\n> + MappedBuffer &operator=(MappedBuffer &&other);\n> +\n> + bool isValid() const { return error_ == 0; }\n> + int error() const { return error_; }\n> + const std::vector &maps() const { return maps_; }\n> +\n> +protected:\n> + MappedBuffer();\n> +\n> + int error_;\n> + std::vector maps_;\n> +\n> +private:\n> + LIBCAMERA_DISABLE_COPY(MappedBuffer)\n> +};\n> +\n> +class MappedFrameBuffer : public MappedBuffer\n> +{\n> +public:\n> + MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index dac1a2d36fa8..665fd6de4ed3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n> 'ipa_module.h',\n> 'ipa_proxy.h',\n> 'ipc_unixsocket.h',\n> + 'mapped_framebuffer.h',\n> 'media_device.h',\n> 'media_object.h',\n> 'pipeline_handler.h',\n> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> index 21373fa25bf8..e67cfa2b363e 100644\n> --- a/src/android/camera_buffer.h\n> +++ b/src/android/camera_buffer.h\n> @@ -7,6 +7,8 @@\n> #ifndef __ANDROID_CAMERA_BUFFER_H__\n> #define __ANDROID_CAMERA_BUFFER_H__\n>\n> +#include \n> +\n> #include \n>\n> #include \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 089a6204605e..dd9aebba7553 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -24,8 +24,6 @@\n> #include \n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> #include \"camera_capabilities.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 629d9e00e08d..2dab6c3a37d1 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -19,8 +19,6 @@\n> #include \n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> class CameraDevice;\n> class CameraMetadata;\n> class PostProcessor;\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index e6358ca9466f..372018d2207f 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -23,6 +23,7 @@\n> #include \n>\n> #include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> using namespace libcamera;\n>\n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index 14bf89223982..61fbd1a69278 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -10,7 +10,6 @@\n> #include \"encoder.h\"\n>\n> #include \"libcamera/internal/formats.h\"\n> -#include \"libcamera/internal/framebuffer.h\"\n>\n> #include \n>\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index 5c399be9eb6a..6fd3102229ab 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -13,8 +13,6 @@\n>\n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> class CameraDevice;\n>\n> class PostProcessorJpeg : public PostProcessor\n> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> index 5cb00744a688..535e2cece914 100644\n> --- a/src/android/jpeg/thumbnailer.cpp\n> +++ b/src/android/jpeg/thumbnailer.cpp\n> @@ -11,6 +11,8 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> using namespace libcamera;\n>\n> LOG_DEFINE_CATEGORY(Thumbnailer)\n> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> index 68cbf74329a9..4d086c4943b0 100644\n> --- a/src/android/jpeg/thumbnailer.h\n> +++ b/src/android/jpeg/thumbnailer.h\n> @@ -7,10 +7,10 @@\n> #ifndef __ANDROID_JPEG_THUMBNAILER_H__\n> #define __ANDROID_JPEG_THUMBNAILER_H__\n>\n> +#include \n> #include \n>\n> #include \"libcamera/internal/formats.h\"\n> -#include \"libcamera/internal/framebuffer.h\"\n>\n> class Thumbnailer\n> {\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 2a4b77ea5236..0c8adfd999b9 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -11,7 +11,7 @@\n>\n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> using namespace libcamera;\n>\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index 689f85d9d3b8..ab2b2c606fd0 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -10,8 +10,6 @@\n> #include \n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> -\n> #include \"camera_buffer.h\"\n>\n> class CameraMetadata;\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 772e805b32cc..509d4244d614 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -16,6 +16,7 @@\n> #include \n>\n> #include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> using namespace libcamera;\n>\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36e50f..2647bf2f3b96 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -20,7 +20,7 @@\n> #include \n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 48e11c699f29..76f67dd4567a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -28,7 +28,7 @@\n> #include \n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> #include \"agc_algorithm.hpp\"\n> #include \"agc_status.h\"\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index a59e93fbdf91..3d98affb20f9 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const\n> * indicate that the metadata is invalid.\n> */\n>\n> -/**\n> - * \\class MappedBuffer\n> - * \\brief Provide an interface to support managing memory mapped buffers\n> - *\n> - * The MappedBuffer interface provides access to a set of MappedPlanes which\n> - * are available for access by the CPU.\n> - *\n> - * This class is not meant to be constructed directly, but instead derived\n> - * classes should be used to implement the correct mapping of a source buffer.\n> - *\n> - * This allows treating CPU accessible memory through a generic interface\n> - * regardless of whether it originates from a libcamera FrameBuffer or other\n> - * source.\n> - */\n> -\n> -/**\n> - * \\typedef MappedBuffer::Plane\n> - * \\brief A mapped region of memory accessible to the CPU\n> - *\n> - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> - * region.\n> - */\n> -\n> -/**\n> - * \\brief Construct an empty MappedBuffer\n> - */\n> -MappedBuffer::MappedBuffer()\n> - : error_(0)\n> -{\n> -}\n> -\n> -/**\n> - * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n> - * other using move semantics\n> - * \\param[in] other The other MappedBuffer\n> - *\n> - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> - * MappedBuffer and invalidates the \\a other.\n> - *\n> - * No mappings are unmapped or destroyed in this process.\n> - */\n> -MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> -{\n> - *this = std::move(other);\n> -}\n> -\n> -/**\n> - * \\brief Move assignment operator, replace the mappings with those of \\a other\n> -* \\param[in] other The other MappedBuffer\n> - *\n> - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> - * MappedBuffer and invalidates the \\a other.\n> - *\n> - * No mappings are unmapped or destroyed in this process.\n> - */\n> -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> -{\n> - error_ = other.error_;\n> - maps_ = std::move(other.maps_);\n> - other.error_ = -ENOENT;\n> -\n> - return *this;\n> -}\n> -\n> -MappedBuffer::~MappedBuffer()\n> -{\n> - for (Plane &map : maps_)\n> - munmap(map.data(), map.size());\n> -}\n> -\n> -/**\n> - * \\fn MappedBuffer::isValid()\n> - * \\brief Check if the MappedBuffer instance is valid\n> - * \\return True if the MappedBuffer has valid mappings, false otherwise\n> - */\n> -\n> -/**\n> - * \\fn MappedBuffer::error()\n> - * \\brief Retrieve the map error status\n> - *\n> - * This function retrieves the error status from the MappedBuffer.\n> - * The error status is a negative number as defined by errno.h. If\n> - * no error occurred, this function returns 0.\n> - *\n> - * \\return The map error code\n> - */\n> -\n> -/**\n> - * \\fn MappedBuffer::maps()\n> - * \\brief Retrieve the mapped planes\n> - *\n> - * This function retrieves the successfully mapped planes stored as a vector\n> - * of Span to provide access to the mapped memory.\n> - *\n> - * \\return A vector of the mapped planes\n> - */\n> -\n> -/**\n> - * \\var MappedBuffer::error_\n> - * \\brief Stores the error value if present\n> - *\n> - * MappedBuffer derived classes shall set this to a negative value as defined\n> - * by errno.h if an error occured during the mapping process.\n> - */\n> -\n> -/**\n> - * \\var MappedBuffer::maps_\n> - * \\brief Stores the internal mapped planes\n> - *\n> - * MappedBuffer derived classes shall store the mappings they create in this\n> - * vector which is parsed during destruct to unmap any memory mappings which\n> - * completed successfully.\n> - */\n> -\n> -/**\n> - * \\class MappedFrameBuffer\n> - * \\brief Map a FrameBuffer using the MappedBuffer interface\n> - */\n> -\n> -/**\n> - * \\brief Map all planes of a FrameBuffer\n> - * \\param[in] buffer FrameBuffer to be mapped\n> - * \\param[in] flags Protection flags to apply to map\n> - *\n> - * Construct an object to map a frame buffer for CPU access.\n> - * The flags are passed directly to mmap and should be either PROT_READ,\n> - * PROT_WRITE, or a bitwise-or combination of both.\n> - */\n> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> -{\n> - maps_.reserve(buffer->planes().size());\n> -\n> - for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> - void *address = mmap(nullptr, plane.length, flags,\n> - MAP_SHARED, plane.fd.fd(), 0);\n> - if (address == MAP_FAILED) {\n> - error_ = -errno;\n> - LOG(Buffer, Error) << \"Failed to mmap plane\";\n> - break;\n> - }\n> -\n> - maps_.emplace_back(static_cast(address), plane.length);\n> - }\n> -}\n> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> new file mode 100644\n> index 000000000000..0e30fc542154\n> --- /dev/null\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -0,0 +1,171 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * mapped_framebuffer.cpp - Mapped Framebuffer support\n> + */\n> +\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file libcamera/internal/mapped_framebuffer.h\n> + * \\brief Frame buffer memory mapping support\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Buffer)\n> +\n> +/**\n> + * \\class MappedBuffer\n> + * \\brief Provide an interface to support managing memory mapped buffers\n> + *\n> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n> + * are available for access by the CPU.\n> + *\n> + * This class is not meant to be constructed directly, but instead derived\n> + * classes should be used to implement the correct mapping of a source buffer.\n> + *\n> + * This allows treating CPU accessible memory through a generic interface\n> + * regardless of whether it originates from a libcamera FrameBuffer or other\n> + * source.\n> + */\n> +\n> +/**\n> + * \\typedef MappedBuffer::Plane\n> + * \\brief A mapped region of memory accessible to the CPU\n> + *\n> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> + * region.\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty MappedBuffer\n> + */\n> +MappedBuffer::MappedBuffer()\n> + : error_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n> + * other using move semantics\n> + * \\param[in] other The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> + * MappedBuffer and invalidates the \\a other.\n> + *\n> + * No mappings are unmapped or destroyed in this process.\n> + */\n> +MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> +{\n> + *this = std::move(other);\n> +}\n> +\n> +/**\n> + * \\brief Move assignment operator, replace the mappings with those of \\a other\n> +* \\param[in] other The other MappedBuffer\n> + *\n> + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> + * MappedBuffer and invalidates the \\a other.\n> + *\n> + * No mappings are unmapped or destroyed in this process.\n> + */\n> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> +{\n> + error_ = other.error_;\n> + maps_ = std::move(other.maps_);\n> + other.error_ = -ENOENT;\n> +\n> + return *this;\n> +}\n> +\n> +MappedBuffer::~MappedBuffer()\n> +{\n> + for (Plane &map : maps_)\n> + munmap(map.data(), map.size());\n> +}\n> +\n> +/**\n> + * \\fn MappedBuffer::isValid()\n> + * \\brief Check if the MappedBuffer instance is valid\n> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::error()\n> + * \\brief Retrieve the map error status\n> + *\n> + * This function retrieves the error status from the MappedBuffer.\n> + * The error status is a negative number as defined by errno.h. If\n> + * no error occurred, this function returns 0.\n> + *\n> + * \\return The map error code\n> + */\n> +\n> +/**\n> + * \\fn MappedBuffer::maps()\n> + * \\brief Retrieve the mapped planes\n> + *\n> + * This function retrieves the successfully mapped planes stored as a vector\n> + * of Span to provide access to the mapped memory.\n> + *\n> + * \\return A vector of the mapped planes\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::error_\n> + * \\brief Stores the error value if present\n> + *\n> + * MappedBuffer derived classes shall set this to a negative value as defined\n> + * by errno.h if an error occured during the mapping process.\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::maps_\n> + * \\brief Stores the internal mapped planes\n> + *\n> + * MappedBuffer derived classes shall store the mappings they create in this\n> + * vector which is parsed during destruct to unmap any memory mappings which\n> + * completed successfully.\n> + */\n> +\n> +/**\n> + * \\class MappedFrameBuffer\n> + * \\brief Map a FrameBuffer using the MappedBuffer interface\n> + */\n> +\n> +/**\n> + * \\brief Map all planes of a FrameBuffer\n> + * \\param[in] buffer FrameBuffer to be mapped\n> + * \\param[in] flags Protection flags to apply to map\n> + *\n> + * Construct an object to map a frame buffer for CPU access.\n> + * The flags are passed directly to mmap and should be either PROT_READ,\n> + * PROT_WRITE, or a bitwise-or combination of both.\n> + */\n> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> +{\n> + maps_.reserve(buffer->planes().size());\n> +\n> + for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> + void *address = mmap(nullptr, plane.length, flags,\n> + MAP_SHARED, plane.fd.fd(), 0);\n> + if (address == MAP_FAILED) {\n> + error_ = -errno;\n> + LOG(Buffer, Error) << \"Failed to mmap plane\";\n> + break;\n> + }\n> +\n> + maps_.emplace_back(static_cast(address), plane.length);\n> + }\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 4f08580157f9..e9230b983aeb 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -28,6 +28,7 @@ libcamera_sources = files([\n> 'ipc_pipe.cpp',\n> 'ipc_pipe_unixsocket.cpp',\n> 'ipc_unixsocket.cpp',\n> + 'mapped_framebuffer.cpp',\n> 'media_device.cpp',\n> 'media_object.cpp',\n> 'pipeline_handler.cpp',\n> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> index c9479194cb68..a3d1511b74ce 100644\n> --- a/test/mapped-buffer.cpp\n> +++ b/test/mapped-buffer.cpp\n> @@ -9,7 +9,7 @@\n>\n> #include \n>\n> -#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> #include \"camera_test.h\"\n> #include \"test.h\"\n> --\n> 2.30.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8D987BD87D\n\tfor ;\n\tFri, 6 Aug 2021 10:30:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 069F868811;\n\tFri, 6 Aug 2021 12:30:10 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2628A60266\n\tfor ;\n\tFri, 6 Aug 2021 12:30:08 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id zb12so9545414ejb.5\n\tfor ;\n\tFri, 06 Aug 2021 03:30:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YjqKCGp7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=E7DSJ6oreZszcrlyiwvof1QDMtthXi1p1581nbeIFAo=;\n\tb=YjqKCGp740+uXwowBIkHg1luG9bQ1P40fcOaw38hslw4JmuSO+FgxBLGadmbW9u2K0\n\t0D8oNksyHAqFbbF4A9v/LvzrRCBbIf+195quW9psCCn+qZcuf9Va04SpiLEVO0Yi0UKi\n\tjly6OMqXTksRoIRXASycJpvGApK1dFW3vDICg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=E7DSJ6oreZszcrlyiwvof1QDMtthXi1p1581nbeIFAo=;\n\tb=T8zAgdlI1X3/CrRhuFfNzLb/sm5Q8ENEND3cP3JisGfyw3dPTEfeMbSKOnDJyYztKw\n\tPBvLV7nw8sHbhAMudCylCUgQ07SeMAVyxykK3M5TafAL0x9PL5FuAdN7+UepbhZw86ui\n\teyrEpgVlF5vjEZihZHxpPCIj6jwjMczYHnHG0vV0NX5NGmneGaBoN5ekGwesaQQxpj/f\n\tM8i6y9JHJgEEVANOjX3R8jD/J41NrmhaQmoPKWH9clbsk+jtZ6Bqj4bA43SzyklghnVr\n\tRvhdqtPp6EbiztyQ7IzG9U0ehRpe9xWdSm0/eb7NkwLQSx3ivSAIe5K2zkJULpUBybEu\n\t6tNQ==","X-Gm-Message-State":"AOAM533fZFR1QXNf1LK0A/yeDYxxK40Byr26rjuftLRyy0vSz/nPW5qc\n\t5+NgtjIxR2d3ELvvuCpfH8ngx3rBzjepVRiTG1PQBw==","X-Google-Smtp-Source":"ABdhPJyUfcNbvO5f8jRm7P0e4rwJ8W7dyTZQ9bR7UoCBs9KiwCqqnZ8bNGwesQ2bCqB5bymFEXfGCj+A75zaZL1or7c=","X-Received":"by 2002:a17:906:b052:: with SMTP id\n\tbj18mr9143682ejb.55.1628245807443; \n\tFri, 06 Aug 2021 03:30:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Fri, 6 Aug 2021 19:29:57 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18592,"web_url":"https://patchwork.libcamera.org/comment/18592/","msgid":"","date":"2021-08-06T10:37:22","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 06, 2021 at 07:29:57PM +0900, Hirokazu Honda wrote:\n> On Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham wrote:\n> >\n> > The MappedFrameBuffer is a convenience feature which sits on top of the\n> > FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.\n> >\n> > This implementation is internal and currently sits in the same internal\n> > files as the internal FrameBuffer, thus exposing those internals to users\n> > of the MappedFramebuffer implementation.\n> >\n> > Move the MappedFrameBuffer and MappedBuffer implementation to its own\n> > implementation files, and fix the sources throughout to use that accordingly.\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/internal/framebuffer.h | 36 ----\n> > .../libcamera/internal/mapped_framebuffer.h | 52 ++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/android/camera_buffer.h | 2 +\n> > src/android/camera_device.h | 2 -\n> > src/android/camera_stream.h | 2 -\n> > src/android/jpeg/encoder_libjpeg.cpp | 1 +\n> > src/android/jpeg/encoder_libjpeg.h | 1 -\n> > src/android/jpeg/post_processor_jpeg.h | 2 -\n> > src/android/jpeg/thumbnailer.cpp | 2 +\n> > src/android/jpeg/thumbnailer.h | 2 +-\n> > src/android/mm/generic_camera_buffer.cpp | 2 +-\n> > src/android/post_processor.h | 2 -\n> > src/android/yuv/post_processor_yuv.cpp | 1 +\n> > src/ipa/ipu3/ipu3.cpp | 2 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/libcamera/framebuffer.cpp | 145 ---------------\n> > src/libcamera/mapped_framebuffer.cpp | 171 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > test/mapped-buffer.cpp | 2 +-\n> > 20 files changed, 236 insertions(+), 195 deletions(-)\n> > create mode 100644 include/libcamera/internal/mapped_framebuffer.h\n> > create mode 100644 src/libcamera/mapped_framebuffer.cpp\n> \n> I wonder if MappedFrameBuffer is so useful that it should be put in\n> libcamera/base.\n> I found src/cam/frame_sink.cpp does mmap()/munmap, which is what\n> MappedFrameBuffer does.\n\nThe issue is that MappedFrameBuffer depends on FrameBuffer, which is\npart of libcamera, not libcamera base. This is something we have\ndiscussed before, and we have decided to postpone the discussion :-) One\noption would be to make MappedFrameBuffer part of the libcamera public\nAPI, but we will then need to polish the API a bit.\n\n> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > index 8c187adf70c7..1352578a6cfb 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -7,46 +7,10 @@\n> > #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n> > #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n> >\n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > -\n> > #include \n> >\n> > namespace libcamera {\n> >\n> > -class MappedBuffer\n> > -{\n> > -public:\n> > - using Plane = Span;\n> > -\n> > - ~MappedBuffer();\n> > -\n> > - MappedBuffer(MappedBuffer &&other);\n> > - MappedBuffer &operator=(MappedBuffer &&other);\n> > -\n> > - bool isValid() const { return error_ == 0; }\n> > - int error() const { return error_; }\n> > - const std::vector &maps() const { return maps_; }\n> > -\n> > -protected:\n> > - MappedBuffer();\n> > -\n> > - int error_;\n> > - std::vector maps_;\n> > -\n> > -private:\n> > - LIBCAMERA_DISABLE_COPY(MappedBuffer)\n> > -};\n> > -\n> > -class MappedFrameBuffer : public MappedBuffer\n> > -{\n> > -public:\n> > - MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> > -};\n> > -\n> > class FrameBuffer::Private : public Extensible::Private\n> > {\n> > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> > new file mode 100644\n> > index 000000000000..41e587364260\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/mapped_framebuffer.h\n> > @@ -0,0 +1,52 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * mapped_framebuffer.h - Frame buffer memory mapping support\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> > +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> > +\n> > +#include \n\nThis isn't needed anymore, but you need stdint.h instead.\n\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class MappedBuffer\n> > +{\n> > +public:\n> > + using Plane = Span;\n> > +\n> > + ~MappedBuffer();\n> > +\n> > + MappedBuffer(MappedBuffer &&other);\n> > + MappedBuffer &operator=(MappedBuffer &&other);\n> > +\n> > + bool isValid() const { return error_ == 0; }\n> > + int error() const { return error_; }\n> > + const std::vector &maps() const { return maps_; }\n> > +\n> > +protected:\n> > + MappedBuffer();\n> > +\n> > + int error_;\n> > + std::vector maps_;\n> > +\n> > +private:\n> > + LIBCAMERA_DISABLE_COPY(MappedBuffer)\n> > +};\n> > +\n> > +class MappedFrameBuffer : public MappedBuffer\n> > +{\n> > +public:\n> > + MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index dac1a2d36fa8..665fd6de4ed3 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n> > 'ipa_module.h',\n> > 'ipa_proxy.h',\n> > 'ipc_unixsocket.h',\n> > + 'mapped_framebuffer.h',\n> > 'media_device.h',\n> > 'media_object.h',\n> > 'pipeline_handler.h',\n> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > index 21373fa25bf8..e67cfa2b363e 100644\n> > --- a/src/android/camera_buffer.h\n> > +++ b/src/android/camera_buffer.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __ANDROID_CAMERA_BUFFER_H__\n> > #define __ANDROID_CAMERA_BUFFER_H__\n> >\n> > +#include \n> > +\n> > #include \n> >\n> > #include \n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 089a6204605e..dd9aebba7553 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -24,8 +24,6 @@\n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > -\n> > #include \"camera_capabilities.h\"\n> > #include \"camera_metadata.h\"\n> > #include \"camera_stream.h\"\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index 629d9e00e08d..2dab6c3a37d1 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -19,8 +19,6 @@\n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > -\n> > class CameraDevice;\n> > class CameraMetadata;\n> > class PostProcessor;\n> > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > index e6358ca9466f..372018d2207f 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > @@ -23,6 +23,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > using namespace libcamera;\n> >\n> > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > index 14bf89223982..61fbd1a69278 100644\n> > --- a/src/android/jpeg/encoder_libjpeg.h\n> > +++ b/src/android/jpeg/encoder_libjpeg.h\n> > @@ -10,7 +10,6 @@\n> > #include \"encoder.h\"\n> >\n> > #include \"libcamera/internal/formats.h\"\n> > -#include \"libcamera/internal/framebuffer.h\"\n> >\n> > #include \n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> > index 5c399be9eb6a..6fd3102229ab 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -13,8 +13,6 @@\n> >\n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > -\n> > class CameraDevice;\n> >\n> > class PostProcessorJpeg : public PostProcessor\n> > diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n> > index 5cb00744a688..535e2cece914 100644\n> > --- a/src/android/jpeg/thumbnailer.cpp\n> > +++ b/src/android/jpeg/thumbnailer.cpp\n> > @@ -11,6 +11,8 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > using namespace libcamera;\n> >\n> > LOG_DEFINE_CATEGORY(Thumbnailer)\n> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > index 68cbf74329a9..4d086c4943b0 100644\n> > --- a/src/android/jpeg/thumbnailer.h\n> > +++ b/src/android/jpeg/thumbnailer.h\n> > @@ -7,10 +7,10 @@\n> > #ifndef __ANDROID_JPEG_THUMBNAILER_H__\n> > #define __ANDROID_JPEG_THUMBNAILER_H__\n> >\n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/formats.h\"\n> > -#include \"libcamera/internal/framebuffer.h\"\n> >\n> > class Thumbnailer\n> > {\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index 2a4b77ea5236..0c8adfd999b9 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -11,7 +11,7 @@\n> >\n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > using namespace libcamera;\n> >\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index 689f85d9d3b8..ab2b2c606fd0 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -10,8 +10,6 @@\n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > -\n> > #include \"camera_buffer.h\"\n> >\n> > class CameraMetadata;\n> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> > index 772e805b32cc..509d4244d614 100644\n> > --- a/src/android/yuv/post_processor_yuv.cpp\n> > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > using namespace libcamera;\n> >\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 71698d36e50f..2647bf2f3b96 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -20,7 +20,7 @@\n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > #include \"ipu3_agc.h\"\n> > #include \"ipu3_awb.h\"\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 48e11c699f29..76f67dd4567a 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -28,7 +28,7 @@\n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > #include \"agc_algorithm.hpp\"\n> > #include \"agc_status.h\"\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index a59e93fbdf91..3d98affb20f9 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const\n> > * indicate that the metadata is invalid.\n> > */\n> >\n> > -/**\n> > - * \\class MappedBuffer\n> > - * \\brief Provide an interface to support managing memory mapped buffers\n> > - *\n> > - * The MappedBuffer interface provides access to a set of MappedPlanes which\n> > - * are available for access by the CPU.\n> > - *\n> > - * This class is not meant to be constructed directly, but instead derived\n> > - * classes should be used to implement the correct mapping of a source buffer.\n> > - *\n> > - * This allows treating CPU accessible memory through a generic interface\n> > - * regardless of whether it originates from a libcamera FrameBuffer or other\n> > - * source.\n> > - */\n> > -\n> > -/**\n> > - * \\typedef MappedBuffer::Plane\n> > - * \\brief A mapped region of memory accessible to the CPU\n> > - *\n> > - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> > - * region.\n> > - */\n> > -\n> > -/**\n> > - * \\brief Construct an empty MappedBuffer\n> > - */\n> > -MappedBuffer::MappedBuffer()\n> > - : error_(0)\n> > -{\n> > -}\n> > -\n> > -/**\n> > - * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n> > - * other using move semantics\n> > - * \\param[in] other The other MappedBuffer\n> > - *\n> > - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> > - * MappedBuffer and invalidates the \\a other.\n> > - *\n> > - * No mappings are unmapped or destroyed in this process.\n> > - */\n> > -MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> > -{\n> > - *this = std::move(other);\n> > -}\n> > -\n> > -/**\n> > - * \\brief Move assignment operator, replace the mappings with those of \\a other\n> > -* \\param[in] other The other MappedBuffer\n> > - *\n> > - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> > - * MappedBuffer and invalidates the \\a other.\n> > - *\n> > - * No mappings are unmapped or destroyed in this process.\n> > - */\n> > -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> > -{\n> > - error_ = other.error_;\n> > - maps_ = std::move(other.maps_);\n> > - other.error_ = -ENOENT;\n> > -\n> > - return *this;\n> > -}\n> > -\n> > -MappedBuffer::~MappedBuffer()\n> > -{\n> > - for (Plane &map : maps_)\n> > - munmap(map.data(), map.size());\n> > -}\n> > -\n> > -/**\n> > - * \\fn MappedBuffer::isValid()\n> > - * \\brief Check if the MappedBuffer instance is valid\n> > - * \\return True if the MappedBuffer has valid mappings, false otherwise\n> > - */\n> > -\n> > -/**\n> > - * \\fn MappedBuffer::error()\n> > - * \\brief Retrieve the map error status\n> > - *\n> > - * This function retrieves the error status from the MappedBuffer.\n> > - * The error status is a negative number as defined by errno.h. If\n> > - * no error occurred, this function returns 0.\n> > - *\n> > - * \\return The map error code\n> > - */\n> > -\n> > -/**\n> > - * \\fn MappedBuffer::maps()\n> > - * \\brief Retrieve the mapped planes\n> > - *\n> > - * This function retrieves the successfully mapped planes stored as a vector\n> > - * of Span to provide access to the mapped memory.\n> > - *\n> > - * \\return A vector of the mapped planes\n> > - */\n> > -\n> > -/**\n> > - * \\var MappedBuffer::error_\n> > - * \\brief Stores the error value if present\n> > - *\n> > - * MappedBuffer derived classes shall set this to a negative value as defined\n> > - * by errno.h if an error occured during the mapping process.\n> > - */\n> > -\n> > -/**\n> > - * \\var MappedBuffer::maps_\n> > - * \\brief Stores the internal mapped planes\n> > - *\n> > - * MappedBuffer derived classes shall store the mappings they create in this\n> > - * vector which is parsed during destruct to unmap any memory mappings which\n> > - * completed successfully.\n> > - */\n> > -\n> > -/**\n> > - * \\class MappedFrameBuffer\n> > - * \\brief Map a FrameBuffer using the MappedBuffer interface\n> > - */\n> > -\n> > -/**\n> > - * \\brief Map all planes of a FrameBuffer\n> > - * \\param[in] buffer FrameBuffer to be mapped\n> > - * \\param[in] flags Protection flags to apply to map\n> > - *\n> > - * Construct an object to map a frame buffer for CPU access.\n> > - * The flags are passed directly to mmap and should be either PROT_READ,\n> > - * PROT_WRITE, or a bitwise-or combination of both.\n> > - */\n> > -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> > -{\n> > - maps_.reserve(buffer->planes().size());\n> > -\n> > - for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > - void *address = mmap(nullptr, plane.length, flags,\n> > - MAP_SHARED, plane.fd.fd(), 0);\n> > - if (address == MAP_FAILED) {\n> > - error_ = -errno;\n> > - LOG(Buffer, Error) << \"Failed to mmap plane\";\n> > - break;\n> > - }\n> > -\n> > - maps_.emplace_back(static_cast(address), plane.length);\n> > - }\n> > -}\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> > new file mode 100644\n> > index 000000000000..0e30fc542154\n> > --- /dev/null\n> > +++ b/src/libcamera/mapped_framebuffer.cpp\n> > @@ -0,0 +1,171 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * mapped_framebuffer.cpp - Mapped Framebuffer support\n> > + */\n> > +\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file libcamera/internal/mapped_framebuffer.h\n> > + * \\brief Frame buffer memory mapping support\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Buffer)\n> > +\n> > +/**\n> > + * \\class MappedBuffer\n> > + * \\brief Provide an interface to support managing memory mapped buffers\n> > + *\n> > + * The MappedBuffer interface provides access to a set of MappedPlanes which\n> > + * are available for access by the CPU.\n> > + *\n> > + * This class is not meant to be constructed directly, but instead derived\n> > + * classes should be used to implement the correct mapping of a source buffer.\n> > + *\n> > + * This allows treating CPU accessible memory through a generic interface\n> > + * regardless of whether it originates from a libcamera FrameBuffer or other\n> > + * source.\n> > + */\n> > +\n> > +/**\n> > + * \\typedef MappedBuffer::Plane\n> > + * \\brief A mapped region of memory accessible to the CPU\n> > + *\n> > + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n> > + * region.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an empty MappedBuffer\n> > + */\n> > +MappedBuffer::MappedBuffer()\n> > + : error_(0)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n> > + * other using move semantics\n> > + * \\param[in] other The other MappedBuffer\n> > + *\n> > + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> > + * MappedBuffer and invalidates the \\a other.\n> > + *\n> > + * No mappings are unmapped or destroyed in this process.\n> > + */\n> > +MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> > +{\n> > + *this = std::move(other);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Move assignment operator, replace the mappings with those of \\a other\n> > +* \\param[in] other The other MappedBuffer\n> > + *\n> > + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n> > + * MappedBuffer and invalidates the \\a other.\n> > + *\n> > + * No mappings are unmapped or destroyed in this process.\n> > + */\n> > +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> > +{\n> > + error_ = other.error_;\n> > + maps_ = std::move(other.maps_);\n> > + other.error_ = -ENOENT;\n> > +\n> > + return *this;\n> > +}\n> > +\n> > +MappedBuffer::~MappedBuffer()\n> > +{\n> > + for (Plane &map : maps_)\n> > + munmap(map.data(), map.size());\n> > +}\n> > +\n> > +/**\n> > + * \\fn MappedBuffer::isValid()\n> > + * \\brief Check if the MappedBuffer instance is valid\n> > + * \\return True if the MappedBuffer has valid mappings, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn MappedBuffer::error()\n> > + * \\brief Retrieve the map error status\n> > + *\n> > + * This function retrieves the error status from the MappedBuffer.\n> > + * The error status is a negative number as defined by errno.h. If\n> > + * no error occurred, this function returns 0.\n> > + *\n> > + * \\return The map error code\n> > + */\n> > +\n> > +/**\n> > + * \\fn MappedBuffer::maps()\n> > + * \\brief Retrieve the mapped planes\n> > + *\n> > + * This function retrieves the successfully mapped planes stored as a vector\n> > + * of Span to provide access to the mapped memory.\n> > + *\n> > + * \\return A vector of the mapped planes\n> > + */\n> > +\n> > +/**\n> > + * \\var MappedBuffer::error_\n> > + * \\brief Stores the error value if present\n> > + *\n> > + * MappedBuffer derived classes shall set this to a negative value as defined\n> > + * by errno.h if an error occured during the mapping process.\n> > + */\n> > +\n> > +/**\n> > + * \\var MappedBuffer::maps_\n> > + * \\brief Stores the internal mapped planes\n> > + *\n> > + * MappedBuffer derived classes shall store the mappings they create in this\n> > + * vector which is parsed during destruct to unmap any memory mappings which\n> > + * completed successfully.\n> > + */\n> > +\n> > +/**\n> > + * \\class MappedFrameBuffer\n> > + * \\brief Map a FrameBuffer using the MappedBuffer interface\n> > + */\n> > +\n> > +/**\n> > + * \\brief Map all planes of a FrameBuffer\n> > + * \\param[in] buffer FrameBuffer to be mapped\n> > + * \\param[in] flags Protection flags to apply to map\n> > + *\n> > + * Construct an object to map a frame buffer for CPU access.\n> > + * The flags are passed directly to mmap and should be either PROT_READ,\n> > + * PROT_WRITE, or a bitwise-or combination of both.\n> > + */\n> > +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n> > +{\n> > + maps_.reserve(buffer->planes().size());\n> > +\n> > + for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > + void *address = mmap(nullptr, plane.length, flags,\n> > + MAP_SHARED, plane.fd.fd(), 0);\n> > + if (address == MAP_FAILED) {\n> > + error_ = -errno;\n> > + LOG(Buffer, Error) << \"Failed to mmap plane\";\n> > + break;\n> > + }\n> > +\n> > + maps_.emplace_back(static_cast(address), plane.length);\n> > + }\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 4f08580157f9..e9230b983aeb 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -28,6 +28,7 @@ libcamera_sources = files([\n> > 'ipc_pipe.cpp',\n> > 'ipc_pipe_unixsocket.cpp',\n> > 'ipc_unixsocket.cpp',\n> > + 'mapped_framebuffer.cpp',\n> > 'media_device.cpp',\n> > 'media_object.cpp',\n> > 'pipeline_handler.cpp',\n> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> > index c9479194cb68..a3d1511b74ce 100644\n> > --- a/test/mapped-buffer.cpp\n> > +++ b/test/mapped-buffer.cpp\n> > @@ -9,7 +9,7 @@\n> >\n> > #include \n> >\n> > -#include \"libcamera/internal/framebuffer.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > #include \"camera_test.h\"\n> > #include \"test.h\"","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24091C3235\n\tfor ;\n\tFri, 6 Aug 2021 10:37:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 841F968822;\n\tFri, 6 Aug 2021 12:37:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DF0860266\n\tfor ;\n\tFri, 6 Aug 2021 12:37:36 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7AD6A4FB;\n\tFri, 6 Aug 2021 12:37:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RUZx5WfS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628246255;\n\tbh=Sfjx7P/CUEKpfrvx+Z+P3NihDNI0uYfmVYZ/TNpWl/4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RUZx5WfS5rSirK+9PM6cBN38fpp8FuGm2MP17C4h9kvsBEptLwf7u08CAZhOcw3nV\n\tDVJ1A66y4ykPlrKzf1V8L1aV7Y5TzfXmgwGiIFRirPWE0kfitcscnNd6e/+ocsf2lV\n\tH3vOmOM1ID2d0QPNpNpd94jp6gS2CmfXbDGrTpbM=","Date":"Fri, 6 Aug 2021 13:37:22 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18594,"web_url":"https://patchwork.libcamera.org/comment/18594/","msgid":"<602cbd18-4f55-fe0f-6414-40d84a13db4c@ideasonboard.com>","date":"2021-08-06T10:44:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 06/08/2021 11:29, Hirokazu Honda wrote:\n> Hi Kieran, thank you for the patch.\n> \n> On Fri, Aug 6, 2021 at 6:25 PM Kieran Bingham\n> wrote:\n>>\n>> The MappedFrameBuffer is a convenience feature which sits on top of the\n>> FrameBuffer and facilitates mapping it to CPU accessible memory with mmap.\n>>\n>> This implementation is internal and currently sits in the same internal\n>> files as the internal FrameBuffer, thus exposing those internals to users\n>> of the MappedFramebuffer implementation.\n>>\n>> Move the MappedFrameBuffer and MappedBuffer implementation to its own\n>> implementation files, and fix the sources throughout to use that accordingly.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/internal/framebuffer.h | 36 ----\n>> .../libcamera/internal/mapped_framebuffer.h | 52 ++++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/android/camera_buffer.h | 2 +\n>> src/android/camera_device.h | 2 -\n>> src/android/camera_stream.h | 2 -\n>> src/android/jpeg/encoder_libjpeg.cpp | 1 +\n>> src/android/jpeg/encoder_libjpeg.h | 1 -\n>> src/android/jpeg/post_processor_jpeg.h | 2 -\n>> src/android/jpeg/thumbnailer.cpp | 2 +\n>> src/android/jpeg/thumbnailer.h | 2 +-\n>> src/android/mm/generic_camera_buffer.cpp | 2 +-\n>> src/android/post_processor.h | 2 -\n>> src/android/yuv/post_processor_yuv.cpp | 1 +\n>> src/ipa/ipu3/ipu3.cpp | 2 +-\n>> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n>> src/libcamera/framebuffer.cpp | 145 ---------------\n>> src/libcamera/mapped_framebuffer.cpp | 171 ++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> test/mapped-buffer.cpp | 2 +-\n>> 20 files changed, 236 insertions(+), 195 deletions(-)\n>> create mode 100644 include/libcamera/internal/mapped_framebuffer.h\n>> create mode 100644 src/libcamera/mapped_framebuffer.cpp\n>>\n> \n> I wonder if MappedFrameBuffer is so useful that it should be put in\n> libcamera/base.\n\nThat's ... almost the partial intention of this.\nHowever - base sits 'underneath' libcamera, and this class requires\nFrameBuffer which means it has to go 'on top' of libcamera.\n\nWe already extracted a copy of this class to the Intel IPA, so indeed\nthere is scope for this being used elsewhere - but it needs a somewhat\n'new' home which isn't determined yet.\n\nSo this is part of that clean up eitherway.\n\n> I found src/cam/frame_sink.cpp does mmap()/munmap, which is what\n> MappedFrameBuffer does.\n\nYes, I wanted that requirement to be abstracted - but it was argued that\nMappedFrameBuffer should not form part of the libcamera public API.\n\nBut as you've seen - there's a lot of detail to mapping FrameBuffers -\nand I don't want every application to have to duplicate all of that - so\ntheres a potential need for this to live 'somewhere'.\n\nWe already envisioned a 'helper' library on top of libcamera - so it's\npossible it will move to something like that.\n\n\n>> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n>> index 8c187adf70c7..1352578a6cfb 100644\n>> --- a/include/libcamera/internal/framebuffer.h\n>> +++ b/include/libcamera/internal/framebuffer.h\n>> @@ -7,46 +7,10 @@\n>> #ifndef __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n>> #define __LIBCAMERA_INTERNAL_FRAMEBUFFER_H__\n>>\n>> -#include \n>> -#include \n>> -\n>> -#include \n>> -#include \n>> -\n>> #include \n>>\n>> namespace libcamera {\n>>\n>> -class MappedBuffer\n>> -{\n>> -public:\n>> - using Plane = Span;\n>> -\n>> - ~MappedBuffer();\n>> -\n>> - MappedBuffer(MappedBuffer &&other);\n>> - MappedBuffer &operator=(MappedBuffer &&other);\n>> -\n>> - bool isValid() const { return error_ == 0; }\n>> - int error() const { return error_; }\n>> - const std::vector &maps() const { return maps_; }\n>> -\n>> -protected:\n>> - MappedBuffer();\n>> -\n>> - int error_;\n>> - std::vector maps_;\n>> -\n>> -private:\n>> - LIBCAMERA_DISABLE_COPY(MappedBuffer)\n>> -};\n>> -\n>> -class MappedFrameBuffer : public MappedBuffer\n>> -{\n>> -public:\n>> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>> -};\n>> -\n>> class FrameBuffer::Private : public Extensible::Private\n>> {\n>> LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n>> new file mode 100644\n>> index 000000000000..41e587364260\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>> @@ -0,0 +1,52 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * mapped_framebuffer.h - Frame buffer memory mapping support\n>> + */\n>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class MappedBuffer\n>> +{\n>> +public:\n>> + using Plane = Span;\n>> +\n>> + ~MappedBuffer();\n>> +\n>> + MappedBuffer(MappedBuffer &&other);\n>> + MappedBuffer &operator=(MappedBuffer &&other);\n>> +\n>> + bool isValid() const { return error_ == 0; }\n>> + int error() const { return error_; }\n>> + const std::vector &maps() const { return maps_; }\n>> +\n>> +protected:\n>> + MappedBuffer();\n>> +\n>> + int error_;\n>> + std::vector maps_;\n>> +\n>> +private:\n>> + LIBCAMERA_DISABLE_COPY(MappedBuffer)\n>> +};\n>> +\n>> +class MappedFrameBuffer : public MappedBuffer\n>> +{\n>> +public:\n>> + MappedFrameBuffer(const FrameBuffer *buffer, int flags);\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index dac1a2d36fa8..665fd6de4ed3 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n>> 'ipa_module.h',\n>> 'ipa_proxy.h',\n>> 'ipc_unixsocket.h',\n>> + 'mapped_framebuffer.h',\n>> 'media_device.h',\n>> 'media_object.h',\n>> 'pipeline_handler.h',\n>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n>> index 21373fa25bf8..e67cfa2b363e 100644\n>> --- a/src/android/camera_buffer.h\n>> +++ b/src/android/camera_buffer.h\n>> @@ -7,6 +7,8 @@\n>> #ifndef __ANDROID_CAMERA_BUFFER_H__\n>> #define __ANDROID_CAMERA_BUFFER_H__\n>>\n>> +#include \n>> +\n>> #include \n>>\n>> #include \n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 089a6204605e..dd9aebba7553 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -24,8 +24,6 @@\n>> #include \n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> -\n>> #include \"camera_capabilities.h\"\n>> #include \"camera_metadata.h\"\n>> #include \"camera_stream.h\"\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index 629d9e00e08d..2dab6c3a37d1 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -19,8 +19,6 @@\n>> #include \n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> -\n>> class CameraDevice;\n>> class CameraMetadata;\n>> class PostProcessor;\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n>> index e6358ca9466f..372018d2207f 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n>> @@ -23,6 +23,7 @@\n>> #include \n>>\n>> #include \"libcamera/internal/formats.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> using namespace libcamera;\n>>\n>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n>> index 14bf89223982..61fbd1a69278 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.h\n>> +++ b/src/android/jpeg/encoder_libjpeg.h\n>> @@ -10,7 +10,6 @@\n>> #include \"encoder.h\"\n>>\n>> #include \"libcamera/internal/formats.h\"\n>> -#include \"libcamera/internal/framebuffer.h\"\n>>\n>> #include \n>>\n>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n>> index 5c399be9eb6a..6fd3102229ab 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.h\n>> +++ b/src/android/jpeg/post_processor_jpeg.h\n>> @@ -13,8 +13,6 @@\n>>\n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> -\n>> class CameraDevice;\n>>\n>> class PostProcessorJpeg : public PostProcessor\n>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp\n>> index 5cb00744a688..535e2cece914 100644\n>> --- a/src/android/jpeg/thumbnailer.cpp\n>> +++ b/src/android/jpeg/thumbnailer.cpp\n>> @@ -11,6 +11,8 @@\n>>\n>> #include \n>>\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>> +\n>> using namespace libcamera;\n>>\n>> LOG_DEFINE_CATEGORY(Thumbnailer)\n>> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n>> index 68cbf74329a9..4d086c4943b0 100644\n>> --- a/src/android/jpeg/thumbnailer.h\n>> +++ b/src/android/jpeg/thumbnailer.h\n>> @@ -7,10 +7,10 @@\n>> #ifndef __ANDROID_JPEG_THUMBNAILER_H__\n>> #define __ANDROID_JPEG_THUMBNAILER_H__\n>>\n>> +#include \n>> #include \n>>\n>> #include \"libcamera/internal/formats.h\"\n>> -#include \"libcamera/internal/framebuffer.h\"\n>>\n>> class Thumbnailer\n>> {\n>> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n>> index 2a4b77ea5236..0c8adfd999b9 100644\n>> --- a/src/android/mm/generic_camera_buffer.cpp\n>> +++ b/src/android/mm/generic_camera_buffer.cpp\n>> @@ -11,7 +11,7 @@\n>>\n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> using namespace libcamera;\n>>\n>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>> index 689f85d9d3b8..ab2b2c606fd0 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -10,8 +10,6 @@\n>> #include \n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> -\n>> #include \"camera_buffer.h\"\n>>\n>> class CameraMetadata;\n>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n>> index 772e805b32cc..509d4244d614 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -16,6 +16,7 @@\n>> #include \n>>\n>> #include \"libcamera/internal/formats.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> using namespace libcamera;\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 71698d36e50f..2647bf2f3b96 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -20,7 +20,7 @@\n>> #include \n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> #include \"ipu3_agc.h\"\n>> #include \"ipu3_awb.h\"\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 48e11c699f29..76f67dd4567a 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -28,7 +28,7 @@\n>> #include \n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> #include \"agc_algorithm.hpp\"\n>> #include \"agc_status.h\"\n>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n>> index a59e93fbdf91..3d98affb20f9 100644\n>> --- a/src/libcamera/framebuffer.cpp\n>> +++ b/src/libcamera/framebuffer.cpp\n>> @@ -251,149 +251,4 @@ Request *FrameBuffer::request() const\n>> * indicate that the metadata is invalid.\n>> */\n>>\n>> -/**\n>> - * \\class MappedBuffer\n>> - * \\brief Provide an interface to support managing memory mapped buffers\n>> - *\n>> - * The MappedBuffer interface provides access to a set of MappedPlanes which\n>> - * are available for access by the CPU.\n>> - *\n>> - * This class is not meant to be constructed directly, but instead derived\n>> - * classes should be used to implement the correct mapping of a source buffer.\n>> - *\n>> - * This allows treating CPU accessible memory through a generic interface\n>> - * regardless of whether it originates from a libcamera FrameBuffer or other\n>> - * source.\n>> - */\n>> -\n>> -/**\n>> - * \\typedef MappedBuffer::Plane\n>> - * \\brief A mapped region of memory accessible to the CPU\n>> - *\n>> - * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n>> - * region.\n>> - */\n>> -\n>> -/**\n>> - * \\brief Construct an empty MappedBuffer\n>> - */\n>> -MappedBuffer::MappedBuffer()\n>> - : error_(0)\n>> -{\n>> -}\n>> -\n>> -/**\n>> - * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n>> - * other using move semantics\n>> - * \\param[in] other The other MappedBuffer\n>> - *\n>> - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n>> - * MappedBuffer and invalidates the \\a other.\n>> - *\n>> - * No mappings are unmapped or destroyed in this process.\n>> - */\n>> -MappedBuffer::MappedBuffer(MappedBuffer &&other)\n>> -{\n>> - *this = std::move(other);\n>> -}\n>> -\n>> -/**\n>> - * \\brief Move assignment operator, replace the mappings with those of \\a other\n>> -* \\param[in] other The other MappedBuffer\n>> - *\n>> - * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n>> - * MappedBuffer and invalidates the \\a other.\n>> - *\n>> - * No mappings are unmapped or destroyed in this process.\n>> - */\n>> -MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n>> -{\n>> - error_ = other.error_;\n>> - maps_ = std::move(other.maps_);\n>> - other.error_ = -ENOENT;\n>> -\n>> - return *this;\n>> -}\n>> -\n>> -MappedBuffer::~MappedBuffer()\n>> -{\n>> - for (Plane &map : maps_)\n>> - munmap(map.data(), map.size());\n>> -}\n>> -\n>> -/**\n>> - * \\fn MappedBuffer::isValid()\n>> - * \\brief Check if the MappedBuffer instance is valid\n>> - * \\return True if the MappedBuffer has valid mappings, false otherwise\n>> - */\n>> -\n>> -/**\n>> - * \\fn MappedBuffer::error()\n>> - * \\brief Retrieve the map error status\n>> - *\n>> - * This function retrieves the error status from the MappedBuffer.\n>> - * The error status is a negative number as defined by errno.h. If\n>> - * no error occurred, this function returns 0.\n>> - *\n>> - * \\return The map error code\n>> - */\n>> -\n>> -/**\n>> - * \\fn MappedBuffer::maps()\n>> - * \\brief Retrieve the mapped planes\n>> - *\n>> - * This function retrieves the successfully mapped planes stored as a vector\n>> - * of Span to provide access to the mapped memory.\n>> - *\n>> - * \\return A vector of the mapped planes\n>> - */\n>> -\n>> -/**\n>> - * \\var MappedBuffer::error_\n>> - * \\brief Stores the error value if present\n>> - *\n>> - * MappedBuffer derived classes shall set this to a negative value as defined\n>> - * by errno.h if an error occured during the mapping process.\n>> - */\n>> -\n>> -/**\n>> - * \\var MappedBuffer::maps_\n>> - * \\brief Stores the internal mapped planes\n>> - *\n>> - * MappedBuffer derived classes shall store the mappings they create in this\n>> - * vector which is parsed during destruct to unmap any memory mappings which\n>> - * completed successfully.\n>> - */\n>> -\n>> -/**\n>> - * \\class MappedFrameBuffer\n>> - * \\brief Map a FrameBuffer using the MappedBuffer interface\n>> - */\n>> -\n>> -/**\n>> - * \\brief Map all planes of a FrameBuffer\n>> - * \\param[in] buffer FrameBuffer to be mapped\n>> - * \\param[in] flags Protection flags to apply to map\n>> - *\n>> - * Construct an object to map a frame buffer for CPU access.\n>> - * The flags are passed directly to mmap and should be either PROT_READ,\n>> - * PROT_WRITE, or a bitwise-or combination of both.\n>> - */\n>> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n>> -{\n>> - maps_.reserve(buffer->planes().size());\n>> -\n>> - for (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> - void *address = mmap(nullptr, plane.length, flags,\n>> - MAP_SHARED, plane.fd.fd(), 0);\n>> - if (address == MAP_FAILED) {\n>> - error_ = -errno;\n>> - LOG(Buffer, Error) << \"Failed to mmap plane\";\n>> - break;\n>> - }\n>> -\n>> - maps_.emplace_back(static_cast(address), plane.length);\n>> - }\n>> -}\n>> -\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n>> new file mode 100644\n>> index 000000000000..0e30fc542154\n>> --- /dev/null\n>> +++ b/src/libcamera/mapped_framebuffer.cpp\n>> @@ -0,0 +1,171 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * mapped_framebuffer.cpp - Mapped Framebuffer support\n>> + */\n>> +\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +/**\n>> + * \\file libcamera/internal/mapped_framebuffer.h\n>> + * \\brief Frame buffer memory mapping support\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DECLARE_CATEGORY(Buffer)\n>> +\n>> +/**\n>> + * \\class MappedBuffer\n>> + * \\brief Provide an interface to support managing memory mapped buffers\n>> + *\n>> + * The MappedBuffer interface provides access to a set of MappedPlanes which\n>> + * are available for access by the CPU.\n>> + *\n>> + * This class is not meant to be constructed directly, but instead derived\n>> + * classes should be used to implement the correct mapping of a source buffer.\n>> + *\n>> + * This allows treating CPU accessible memory through a generic interface\n>> + * regardless of whether it originates from a libcamera FrameBuffer or other\n>> + * source.\n>> + */\n>> +\n>> +/**\n>> + * \\typedef MappedBuffer::Plane\n>> + * \\brief A mapped region of memory accessible to the CPU\n>> + *\n>> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory\n>> + * region.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct an empty MappedBuffer\n>> + */\n>> +MappedBuffer::MappedBuffer()\n>> + : error_(0)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Move constructor, construct the MappedBuffer with the contents of \\a\n>> + * other using move semantics\n>> + * \\param[in] other The other MappedBuffer\n>> + *\n>> + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n>> + * MappedBuffer and invalidates the \\a other.\n>> + *\n>> + * No mappings are unmapped or destroyed in this process.\n>> + */\n>> +MappedBuffer::MappedBuffer(MappedBuffer &&other)\n>> +{\n>> + *this = std::move(other);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Move assignment operator, replace the mappings with those of \\a other\n>> +* \\param[in] other The other MappedBuffer\n>> + *\n>> + * Moving a MappedBuffer moves the mappings contained in the \\a other to the new\n>> + * MappedBuffer and invalidates the \\a other.\n>> + *\n>> + * No mappings are unmapped or destroyed in this process.\n>> + */\n>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n>> +{\n>> + error_ = other.error_;\n>> + maps_ = std::move(other.maps_);\n>> + other.error_ = -ENOENT;\n>> +\n>> + return *this;\n>> +}\n>> +\n>> +MappedBuffer::~MappedBuffer()\n>> +{\n>> + for (Plane &map : maps_)\n>> + munmap(map.data(), map.size());\n>> +}\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::isValid()\n>> + * \\brief Check if the MappedBuffer instance is valid\n>> + * \\return True if the MappedBuffer has valid mappings, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::error()\n>> + * \\brief Retrieve the map error status\n>> + *\n>> + * This function retrieves the error status from the MappedBuffer.\n>> + * The error status is a negative number as defined by errno.h. If\n>> + * no error occurred, this function returns 0.\n>> + *\n>> + * \\return The map error code\n>> + */\n>> +\n>> +/**\n>> + * \\fn MappedBuffer::maps()\n>> + * \\brief Retrieve the mapped planes\n>> + *\n>> + * This function retrieves the successfully mapped planes stored as a vector\n>> + * of Span to provide access to the mapped memory.\n>> + *\n>> + * \\return A vector of the mapped planes\n>> + */\n>> +\n>> +/**\n>> + * \\var MappedBuffer::error_\n>> + * \\brief Stores the error value if present\n>> + *\n>> + * MappedBuffer derived classes shall set this to a negative value as defined\n>> + * by errno.h if an error occured during the mapping process.\n>> + */\n>> +\n>> +/**\n>> + * \\var MappedBuffer::maps_\n>> + * \\brief Stores the internal mapped planes\n>> + *\n>> + * MappedBuffer derived classes shall store the mappings they create in this\n>> + * vector which is parsed during destruct to unmap any memory mappings which\n>> + * completed successfully.\n>> + */\n>> +\n>> +/**\n>> + * \\class MappedFrameBuffer\n>> + * \\brief Map a FrameBuffer using the MappedBuffer interface\n>> + */\n>> +\n>> +/**\n>> + * \\brief Map all planes of a FrameBuffer\n>> + * \\param[in] buffer FrameBuffer to be mapped\n>> + * \\param[in] flags Protection flags to apply to map\n>> + *\n>> + * Construct an object to map a frame buffer for CPU access.\n>> + * The flags are passed directly to mmap and should be either PROT_READ,\n>> + * PROT_WRITE, or a bitwise-or combination of both.\n>> + */\n>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)\n>> +{\n>> + maps_.reserve(buffer->planes().size());\n>> +\n>> + for (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> + void *address = mmap(nullptr, plane.length, flags,\n>> + MAP_SHARED, plane.fd.fd(), 0);\n>> + if (address == MAP_FAILED) {\n>> + error_ = -errno;\n>> + LOG(Buffer, Error) << \"Failed to mmap plane\";\n>> + break;\n>> + }\n>> +\n>> + maps_.emplace_back(static_cast(address), plane.length);\n>> + }\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 4f08580157f9..e9230b983aeb 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -28,6 +28,7 @@ libcamera_sources = files([\n>> 'ipc_pipe.cpp',\n>> 'ipc_pipe_unixsocket.cpp',\n>> 'ipc_unixsocket.cpp',\n>> + 'mapped_framebuffer.cpp',\n>> 'media_device.cpp',\n>> 'media_object.cpp',\n>> 'pipeline_handler.cpp',\n>> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n>> index c9479194cb68..a3d1511b74ce 100644\n>> --- a/test/mapped-buffer.cpp\n>> +++ b/test/mapped-buffer.cpp\n>> @@ -9,7 +9,7 @@\n>>\n>> #include \n>>\n>> -#include \"libcamera/internal/framebuffer.h\"\n>> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>>\n>> #include \"camera_test.h\"\n>> #include \"test.h\"\n>> --\n>> 2.30.2\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AB12BC3235\n\tfor ;\n\tFri, 6 Aug 2021 10:44:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6164968815;\n\tFri, 6 Aug 2021 12:44:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A0EF60266\n\tfor ;\n\tFri, 6 Aug 2021 12:44:19 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 27AF84FB;\n\tFri, 6 Aug 2021 12:44:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AFqyfFBk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628246658;\n\tbh=FG5fWoNQUajXb/5H1rFBCu5dA5M0yXZ8YqgAOAagtNQ=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=AFqyfFBk0Ltf6XC84kxmiG4XHAjbVa46+XVfmSajvqrj47JTitGpL0ZfvzAHQKATr\n\tT0YrNGVLYq8ds7z+vL/5YT4uOJsT2UF4JGPfw2T7+CasFfm4Ta7fjMyp2UnnOKeEpk\n\tQZpU00DtG5WLEuOQB6kiPa8hJg3MHxFi3smmu3Ic=","From":"Kieran Bingham ","To":"Hirokazu Honda ","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t","Message-ID":"<602cbd18-4f55-fe0f-6414-40d84a13db4c@ideasonboard.com>","Date":"Fri, 6 Aug 2021 11:44:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18595,"web_url":"https://patchwork.libcamera.org/comment/18595/","msgid":"<6abacc7d-3157-edba-e4a3-b6ed27088677@ideasonboard.com>","date":"2021-08-06T10:56:40","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/08/2021 11:37, Laurent Pinchart wrote:\n\n\n>>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>>> @@ -0,0 +1,52 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * mapped_framebuffer.h - Frame buffer memory mapping support\n>>> + */\n>>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>>> +\n>>> +#include \n> \n> This isn't needed anymore, but you need stdint.h instead.\n\nUsers of MappedFrameBuffer need to specify the mapping flags.\n\nWe can abstract that with your new Flags class now if you prefer ?\n\n\n\n\n\n>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>>> index dac1a2d36fa8..665fd6de4ed3 100644\n>>> --- a/include/libcamera/internal/meson.build\n>>> +++ b/include/libcamera/internal/meson.build\n>>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n>>> 'ipa_module.h',\n>>> 'ipa_proxy.h',\n>>> 'ipc_unixsocket.h',\n>>> + 'mapped_framebuffer.h',\n>>> 'media_device.h',\n>>> 'media_object.h',\n>>> 'pipeline_handler.h',\n>>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n>>> index 21373fa25bf8..e67cfa2b363e 100644\n>>> --- a/src/android/camera_buffer.h\n>>> +++ b/src/android/camera_buffer.h\n>>> @@ -7,6 +7,8 @@\n>>> #ifndef __ANDROID_CAMERA_BUFFER_H__\n>>> #define __ANDROID_CAMERA_BUFFER_H__\n>>>\n>>> +#include \n>>> +\n\nThat's also why it is added here ...\n\n\n>>> #include \n>>>\n>>> #include \n\n","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6A65BBD87D\n\tfor ;\n\tFri, 6 Aug 2021 10:56:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3A5560266;\n\tFri, 6 Aug 2021 12:56:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18E0960266\n\tfor ;\n\tFri, 6 Aug 2021 12:56:44 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 316DC4FB;\n\tFri, 6 Aug 2021 12:56:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OxTNerMK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628247403;\n\tbh=FIZ3GFHqXM5T7ueOlRcNXxi8a6gYmtcj06uDO3Ez/yw=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=OxTNerMKDIH6G1oQa/Ot3EZcQML0XQT84YXWnCLVc/9MM6btmiJ/OqWUCmM22S56t\n\tkM8hxv6BjlzROB7zQIa3bU3PLUGurALxyFJrpfCBQgDmCvNqOR8I71e4Vgc3erHKFw\n\tUwy4ST2Vvmq7s6gMOgOSrRj2NyxYZhWXJglPxCq0=","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t\n\t","Message-ID":"<6abacc7d-3157-edba-e4a3-b6ed27088677@ideasonboard.com>","Date":"Fri, 6 Aug 2021 11:56:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18596,"web_url":"https://patchwork.libcamera.org/comment/18596/","msgid":"","date":"2021-08-06T11:06:16","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Aug 06, 2021 at 11:56:40AM +0100, Kieran Bingham wrote:\n> On 06/08/2021 11:37, Laurent Pinchart wrote:\n> \n> \n> >>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> >>> @@ -0,0 +1,52 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2021, Google Inc.\n> >>> + *\n> >>> + * mapped_framebuffer.h - Frame buffer memory mapping support\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> >>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n> >>> +\n> >>> +#include \n> > \n> > This isn't needed anymore, but you need stdint.h instead.\n> \n> Users of MappedFrameBuffer need to specify the mapping flags.\n\nIt's not required by mapped_framebuffer.h, but it's indeed not nice to\nfor all users to include it.\n\n> We can abstract that with your new Flags class now if you prefer ?\n\nIf we want to make this class public, then I'd prefer not depending on\nsys/mman.h indeed. We can add a custom scoped enum (and use the newly\nmerged Flags<> class :-)).\n\n> >>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> >>> index dac1a2d36fa8..665fd6de4ed3 100644\n> >>> --- a/include/libcamera/internal/meson.build\n> >>> +++ b/include/libcamera/internal/meson.build\n> >>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n> >>> 'ipa_module.h',\n> >>> 'ipa_proxy.h',\n> >>> 'ipc_unixsocket.h',\n> >>> + 'mapped_framebuffer.h',\n> >>> 'media_device.h',\n> >>> 'media_object.h',\n> >>> 'pipeline_handler.h',\n> >>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> >>> index 21373fa25bf8..e67cfa2b363e 100644\n> >>> --- a/src/android/camera_buffer.h\n> >>> +++ b/src/android/camera_buffer.h\n> >>> @@ -7,6 +7,8 @@\n> >>> #ifndef __ANDROID_CAMERA_BUFFER_H__\n> >>> #define __ANDROID_CAMERA_BUFFER_H__\n> >>>\n> >>> +#include \n> >>> +\n> \n> That's also why it is added here ...\n> \n> >>> #include \n> >>>\n> >>> #include ","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 81DE1C3235\n\tfor ;\n\tFri, 6 Aug 2021 11:06:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 036AB60267;\n\tFri, 6 Aug 2021 13:06:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B37A760266\n\tfor ;\n\tFri, 6 Aug 2021 13:06:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2AA624FB;\n\tFri, 6 Aug 2021 13:06:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GDXkzYRo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628247990;\n\tbh=fK6lVqXryOtOJ69oV+08gnhQXyU252EUISa9qqGvcBM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GDXkzYRo4eUezkNU3+P1bbZDA9U2MyCPQiPZJaoKsjJ27cWD3aBE0uQ9wwhN0yCBU\n\tqSUfIfgGEwlxWUCLjt9NYSVMCtZhhSBOaxZU9LiWhFCInpeBAsDXCbya4Twlx2D1Xx\n\tbbEjem8+nLCph0ybEBoNdFHjNe+fH3UuvjumM3SA=","Date":"Fri, 6 Aug 2021 14:06:16 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t<6abacc7d-3157-edba-e4a3-b6ed27088677@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6abacc7d-3157-edba-e4a3-b6ed27088677@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18597,"web_url":"https://patchwork.libcamera.org/comment/18597/","msgid":"<46d0eb7b-eee4-d247-7697-a6ba59173f31@ideasonboard.com>","date":"2021-08-06T13:15:34","subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/08/2021 12:06, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Fri, Aug 06, 2021 at 11:56:40AM +0100, Kieran Bingham wrote:\n>> On 06/08/2021 11:37, Laurent Pinchart wrote:\n>> \n>>\n>>>>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>>>>> @@ -0,0 +1,52 @@\n>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2021, Google Inc.\n>>>>> + *\n>>>>> + * mapped_framebuffer.h - Frame buffer memory mapping support\n>>>>> + */\n>>>>> +#ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>>>>> +#define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__\n>>>>> +\n>>>>> +#include \n>>>\n>>> This isn't needed anymore, but you need stdint.h instead.\n>>\n>> Users of MappedFrameBuffer need to specify the mapping flags.\n> \n> It's not required by mapped_framebuffer.h, but it's indeed not nice to\n> for all users to include it.\n> \n>> We can abstract that with your new Flags class now if you prefer ?\n> \n> If we want to make this class public, then I'd prefer not depending on\n> sys/mman.h indeed. We can add a custom scoped enum (and use the newly\n> merged Flags<> class :-)).\n\nI've done all this on top, which I think makes more sense.\n (including picking up the stdint.h).\n\nSee the recently posted\n \"libcamera: MappedFrameBuffer:: Use typed Flags\"\n\n--\nKieran\n\n\n>>>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>>>>> index dac1a2d36fa8..665fd6de4ed3 100644\n>>>>> --- a/include/libcamera/internal/meson.build\n>>>>> +++ b/include/libcamera/internal/meson.build\n>>>>> @@ -28,6 +28,7 @@ libcamera_internal_headers = files([\n>>>>> 'ipa_module.h',\n>>>>> 'ipa_proxy.h',\n>>>>> 'ipc_unixsocket.h',\n>>>>> + 'mapped_framebuffer.h',\n>>>>> 'media_device.h',\n>>>>> 'media_object.h',\n>>>>> 'pipeline_handler.h',\n>>>>> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n>>>>> index 21373fa25bf8..e67cfa2b363e 100644\n>>>>> --- a/src/android/camera_buffer.h\n>>>>> +++ b/src/android/camera_buffer.h\n>>>>> @@ -7,6 +7,8 @@\n>>>>> #ifndef __ANDROID_CAMERA_BUFFER_H__\n>>>>> #define __ANDROID_CAMERA_BUFFER_H__\n>>>>>\n>>>>> +#include \n>>>>> +\n>>\n>> That's also why it is added here ...\n>>\n>>>>> #include \n>>>>>\n>>>>> #include \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49AAFBD87D\n\tfor ;\n\tFri, 6 Aug 2021 13:15:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0786260267;\n\tFri, 6 Aug 2021 15:15:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79B3160266\n\tfor ;\n\tFri, 6 Aug 2021 15:15:38 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EDC64FB;\n\tFri, 6 Aug 2021 15:15:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"P1Gm36mK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628255738;\n\tbh=YuibADu+NWZQw7suSp0ti31Iw9eqCC7hz4iG0evnt8k=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=P1Gm36mKs9z23zKuNyO9AZVSp01KNmL33rsOZVrflfPX0WwAvZxaSFzuOXYq7lleT\n\tEyykiFhwkOlnwwzU8RzMfeR0402/yfsX0LOcnm53uKB4i0dCUHUOnp67Y1sjJ/eXP8\n\tBGeH8o2mzRcpVh9A/sMAJASYUpykUEN7zGDlw4Dg=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210806092529.572680-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t<6abacc7d-3157-edba-e4a3-b6ed27088677@ideasonboard.com>\n\t","Message-ID":"<46d0eb7b-eee4-d247-7697-a6ba59173f31@ideasonboard.com>","Date":"Fri, 6 Aug 2021 14:15:34 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Give MappedFrameBuffer its\n\town implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]