[libcamera-devel] [PATCH] libcamera: framebuffer: Return plane begin address by MappedBuffer::maps()

[Hirokazu Honda]

While I remove a manual address computation in Thumbnail
https://git.linuxtv.org/libcamera.git/tree/src/android/jpeg/thumbnailer.cpp#n64,
I noticed there might be a misunderstanding of
FrameBuffer::Plane::length meaning.
Not only to clarify the meaning but also discuss a better approach, I
filed https://bugs.libcamera.org/show_bug.cgi?id=72.

Regards,
-Hiro

On Fri, Aug 6, 2021 at 2:50 AM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> MappedBuffer::maps() returns std::vector<MappedBuffer::Plane>.
> Plane has the address, but the address points the beginning of the
> buffer containing the plane.
> This makes the Plane point the beginning of the plane. So
> MappedBuffer::maps()[i].data() returns the address of i-th plane.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/framebuffer.h |  1 +
>  src/libcamera/framebuffer.cpp            | 41 +++++++++++++++++-------
>  2 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index 8c187adf..2eb3b7c9 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -36,6 +36,7 @@ protected:
>
>         int error_;
>         std::vector<Plane> maps_;
> +       std::vector<Plane> chunks_;
>
>  private:
>         LIBCAMERA_DISABLE_COPY(MappedBuffer)
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index a59e93fb..8231c1df 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -310,6 +310,7 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>  {
>         error_ = other.error_;
>         maps_ = std::move(other.maps_);
> +       chunks_ = std::move(other.chunks_);
>         other.error_ = -ENOENT;
>
>         return *this;
> @@ -317,8 +318,8 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>
>  MappedBuffer::~MappedBuffer()
>  {
> -       for (Plane &map : maps_)
> -               munmap(map.data(), map.size());
> +       for (Plane &chunk : chunks_)
> +               munmap(chunk.data(), chunk.size());
>  }
>
>  /**
> @@ -381,18 +382,36 @@ MappedBuffer::~MappedBuffer()
>   */
>  MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>  {
> +       if (buffer->planes().empty())
> +               return;
>         maps_.reserve(buffer->planes().size());
>
> +       /*
> +        * Below assumes planes are in the same buffer and planes are
> +        * consecutive.
> +        * \todo remove this assumption.
> +        */
> +       const int fd = buffer->planes()[0].fd.fd();
> +       const off_t length = lseek(fd, 0, SEEK_END);
> +       if (length < 0) {
> +               error_ = -errno;
> +               LOG(Buffer, Error) << "Failed to lseek buffer";
> +               return;
> +       }
> +
> +       void *address = mmap(nullptr, length, flags, MAP_SHARED, fd, 0);
> +       if (address == MAP_FAILED) {
> +               error_ = -errno;
> +               LOG(Buffer, Error) << "Failed to mmap plane";
> +               return;
> +       }
> +       chunks_ = { MappedBuffer::Plane(static_cast<uint8_t *>(address),
> +                                       static_cast<size_t>(length)) };
> +       size_t offset = 0;
>         for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -               void *address = mmap(nullptr, plane.length, flags,
> -                                    MAP_SHARED, plane.fd.fd(), 0);
> -               if (address == MAP_FAILED) {
> -                       error_ = -errno;
> -                       LOG(Buffer, Error) << "Failed to mmap plane";
> -                       break;
> -               }
> -
> -               maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +               maps_.emplace_back(static_cast<uint8_t *>(address) + offset,
> +                                  plane.length);
> +               offset += plane.length;
>         }
>  }
>
> --
> 2.32.0.554.ge1b32706d8-goog
>