[libcamera-devel] [PATCH v2] libcamera: MappedFrameBuffer: Use typed Flags<MapModes>
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 9 14:47:42 CEST 2021
Hi Laurent,
On 06/08/2021 16:19, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Fri, Aug 06, 2021 at 02:53:15PM +0100, Kieran Bingham wrote:
>> Remove the need for callers to reference PROT_READ/PROT_WRITE directly
>> from <sys/mman.h> by instead exposing the Read/Write mapping options as
>> flags from the MappedFrameBuffer class itself.
>>
>> While here, introduce the <stdint.h> header which is required for the
>> uint8_t as part of the Plane.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> ---
>> v2
>> - Use fully scoped enum class
>> - Swap MapMode and MapModes
>> - s/mmap_flags/mmapFlags/
>> - Remove and fix documentation regarding the modes
>> - add LIBCAMERA_FLAGS_ENABLE_OPERATORS()
>>
>> .../libcamera/internal/mapped_framebuffer.h | 16 +++++++--
>> src/android/jpeg/encoder_libjpeg.cpp | 2 +-
>> src/android/jpeg/thumbnailer.cpp | 2 +-
>> src/android/yuv/post_processor_yuv.cpp | 2 +-
>> src/ipa/ipu3/ipu3.cpp | 2 +-
>> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-
>> src/libcamera/mapped_framebuffer.cpp | 34 ++++++++++++++++---
>> test/mapped-buffer.cpp | 6 ++--
>> 8 files changed, 52 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
>> index 41e587364260..e8234ebf76ff 100644
>> --- a/include/libcamera/internal/mapped_framebuffer.h
>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>> @@ -7,10 +7,11 @@
>> #ifndef __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>> #define __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__
>>
>> -#include <sys/mman.h>
>
> I trust that you've looked through the code base to ensure no other
> unnecessary inclusion of sys/mman.h is left.
No ;-)
But now I have at least removed them all and added back only the ones
that break compilation:
Which leaves:
modified: src/android/camera_device.cpp
modified: src/android/jpeg/encoder_libjpeg.cpp
modified: src/ipa/ipu3/ipu3.cpp
modified: src/libcamera/framebuffer.cpp
modified: src/libcamera/ipa_module.cpp
modified: src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
modified: src/libcamera/v4l2_videodevice.cpp
modified: src/v4l2/v4l2_camera_proxy.h
modified: src/v4l2/v4l2_compat.cpp
modified: src/v4l2/v4l2_compat_manager.h
that can potentially have mman.h removed, but those are not all
necessarily caused by this series, nor no longer required if they simply
pass compilation due to other components including the requirements for
them ...
Especially given that I can remove all those references to mman.h before
my MappedFrameBuffer patch - and have a successful build just the same.
It might be better to sweep through things like that with a tool like
include-what-you-use as a more general task, or inclusion in checkstyle.py.
>> +#include <stdint.h>
>> #include <vector>
>>
>> #include <libcamera/base/class.h>
>> +#include <libcamera/base/flags.h>
>> #include <libcamera/base/span.h>
>>
>> #include <libcamera/framebuffer.h>
>> @@ -44,9 +45,20 @@ private:
>> class MappedFrameBuffer : public MappedBuffer
>> {
>> public:
>> - MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>> + enum class MapMode {
>> + Read = 0 << 1,
>> + Write = 1 << 1,
>> +
>
> I'd drop this blank line.
Dropped.
>
>> + ReadWrite = Read | Write,
>> + };
>> +
>> + using MapModes = Flags<MapMode>;
>> +
>> + MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags);
>
> I'll be annoying again I'm afraid, but "MapModes flags" bothers me :-/
> Looking at the two example from File, we could have
>
> enum class MapModeFlag {
> ...
> };
>
> using MapMode = Flags<MapModeFlag>;
>
> MappedFrameBuffer(const FrameBuffer *buffer, MapMode mode);
>
> or
> enum class MapFlag {
> ...
> };
>
> using MapFlags = Flags<MapFlag>;
>
> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>
> Other options are possible (and it may make sense to standardize naming
> schemes once and for all), but naming the type "modes" and the argument
> "flags" lacks consistency.
Yes, I think I introduced 'Modes' because Read/Write/ReadWrite are more
modes to me than flags.
But as they are still MapFlags, I'm going to pick your second option.
I sort of dislike the enum being singular - because that's where the
flag(*s*) are stored.
>
> Sorry for being nitpicking, it's the first usage of Flags<> outside of
> the File class, so it's affected by the 0, 1, N generalization theorem.
> I'm also thinking that MappedBuffer may graduate as a public API, hence
> the particular attention.
>
>> };
>>
>> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapMode)
>> +
>> } /* namespace libcamera */
>>
>> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */
>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
>> index 372018d2207f..34472e8da6a2 100644
>> --- a/src/android/jpeg/encoder_libjpeg.cpp
>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
>> @@ -182,7 +182,7 @@ void EncoderLibJpeg::compressNV(Span<const uint8_t> frame)
>> int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest,
>> Span<const uint8_t> exifData, unsigned int quality)
>> {
>> - MappedFrameBuffer frame(&source, PROT_READ);
>> + MappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);
>> if (!frame.isValid()) {
>> LOG(JPEG, Error) << "Failed to map FrameBuffer : "
>> << strerror(frame.error());
>> diff --git a/src/android/jpeg/thumbnailer.cpp b/src/android/jpeg/thumbnailer.cpp
>> index 535e2cece914..66b4e696b107 100644
>> --- a/src/android/jpeg/thumbnailer.cpp
>> +++ b/src/android/jpeg/thumbnailer.cpp
>> @@ -41,7 +41,7 @@ void Thumbnailer::createThumbnail(const FrameBuffer &source,
>> const Size &targetSize,
>> std::vector<unsigned char> *destination)
>> {
>> - MappedFrameBuffer frame(&source, PROT_READ);
>> + MappedFrameBuffer frame(&source, MappedFrameBuffer::MapMode::Read);
>> if (!frame.isValid()) {
>> LOG(Thumbnailer, Error)
>> << "Failed to map FrameBuffer : "
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index 509d4244d614..6d156c20646c 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -57,7 +57,7 @@ int PostProcessorYuv::process(const FrameBuffer &source,
>> if (!isValidBuffers(source, *destination))
>> return -EINVAL;
>>
>> - const MappedFrameBuffer sourceMapped(&source, PROT_READ);
>> + const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapMode::Read);
>> if (!sourceMapped.isValid()) {
>> LOG(YUV, Error) << "Failed to mmap camera frame buffer";
>> return -EINVAL;
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2647bf2f3b96..c8fc26525707 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -211,7 +211,7 @@ void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>> for (const IPABuffer &buffer : buffers) {
>> const FrameBuffer fb(buffer.planes);
>> buffers_.emplace(buffer.id,
>> - MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
>> + MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));
>> }
>> }
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 76f67dd4567a..54e22d91084b 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -411,7 +411,8 @@ void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>> {
>> for (const IPABuffer &buffer : buffers) {
>> const FrameBuffer fb(buffer.planes);
>> - buffers_.emplace(buffer.id, MappedFrameBuffer(&fb, PROT_READ | PROT_WRITE));
>> + buffers_.emplace(buffer.id,
>> + MappedFrameBuffer(&fb, MappedFrameBuffer::MapMode::ReadWrite));
>> }
>> }
>>
>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
>> index 0e30fc542154..207c80b5aba2 100644
>> --- a/src/libcamera/mapped_framebuffer.cpp
>> +++ b/src/libcamera/mapped_framebuffer.cpp
>> @@ -142,21 +142,45 @@ MappedBuffer::~MappedBuffer()
>> * \brief Map a FrameBuffer using the MappedBuffer interface
>> */
>>
>> +/**
>> + * \enum MappedFrameBuffer::MapMode
>> + * \brief Specify the mapping mode for the FrameBuffer
>> + * \var MappedFrameBuffer::Read
>> + * \brief Create a Read-only mapping
>
> Maybe s/Read/read/ ? Same for write.
>
>> + * \var MappedFrameBuffer::Write
>> + * \brief Create a Write-only mapping
>> + * \var MappedFrameBuffer::ReadWrite
>> + * \brief Create a mapping with both read and write capabilities
>
> Capabilities sound a bit weird to me in this context, I would have
> written "Create a mapping that can be both read and written".
All updated/taken.
>
>> + */
>> +
>> +/**
>> + * \typedef MappedFrameBuffer::MapModes
>> + * \brief A bitwise combination of MappedFrameBuffer::MapMode values
>> + */
>> +
>> /**
>> * \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.
>> + * Construct an object to map a frame buffer for CPU access. The mapping can be
>> + * made as Read only, Write only or support Read and Write operations by setting
>> + * the MapMode flags accordingly.
>> */
>> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapModes flags)
>> {
>> maps_.reserve(buffer->planes().size());
>>
>> + int mmapFlags = 0;
>> +
>> + if (flags & MapMode::Read)
>> + mmapFlags |= PROT_READ;
>> +
>> + if (flags & MapMode::Write)
>> + mmapFlags |= PROT_WRITE;
>> +
>> for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> - void *address = mmap(nullptr, plane.length, flags,
>> + void *address = mmap(nullptr, plane.length, mmapFlags,
>> MAP_SHARED, plane.fd.fd(), 0);
>> if (address == MAP_FAILED) {
>> error_ = -errno;
>> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
>> index a3d1511b74ce..63d20e7bc9c4 100644
>> --- a/test/mapped-buffer.cpp
>> +++ b/test/mapped-buffer.cpp
>> @@ -71,7 +71,7 @@ protected:
>> const std::unique_ptr<FrameBuffer> &buffer = allocator_->buffers(stream_).front();
>> std::vector<MappedBuffer> maps;
>>
>> - MappedFrameBuffer map(buffer.get(), PROT_READ);
>> + MappedFrameBuffer map(buffer.get(), MappedFrameBuffer::MapMode::Read);
>> if (!map.isValid()) {
>> cout << "Failed to successfully map buffer" << endl;
>> return TestFail;
>> @@ -90,13 +90,13 @@ protected:
>> }
>>
>> /* Test for multiple successful maps on the same buffer. */
>> - MappedFrameBuffer write_map(buffer.get(), PROT_WRITE);
>> + MappedFrameBuffer write_map(buffer.get(), MappedFrameBuffer::MapMode::Write);
>> if (!write_map.isValid()) {
>> cout << "Failed to map write buffer" << endl;
>> return TestFail;
>> }
>>
>> - MappedFrameBuffer rw_map(buffer.get(), PROT_READ | PROT_WRITE);
>> + MappedFrameBuffer rw_map(buffer.get(), MappedFrameBuffer::MapMode::ReadWrite);
>> if (!rw_map.isValid()) {
>> cout << "Failed to map RW buffer" << endl;
>> return TestFail;
>
More information about the libcamera-devel
mailing list