| Message ID | 20210805174955.1579287-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 >
Hi Hiro, On 05/08/2021 18:49, Hirokazu Honda 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. If we can handle the logic in MappedBuffer to get the correct addresses that would certainly be beneficial indeed, so this sounds like good development. > > Signed-off-by: Hirokazu Honda <hiroh@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; Can we have a buffer with zero planes? Is this something that should generate an Error or Warning message? (Or if it really implies there's a bug, a Fatal?) > maps_.reserve(buffer->planes().size()); > > + /* > + * Below assumes planes are in the same buffer and planes are > + * consecutive. > + * \todo remove this assumption. To do that, will we need to store the PixelFormat information as part of the FrameBuffer? Or at least some additional data there so that we know it's layout? And - now I've seen Laurent posted a patch to expose a Format for that purpose... > + */ > + 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; > } > } > >
Hello, On Fri, Aug 06, 2021 at 02:18:10PM +0100, Kieran Bingham wrote: > On 05/08/2021 18:49, Hirokazu Honda 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. > > If we can handle the logic in MappedBuffer to get the correct addresses > that would certainly be beneficial indeed, so this sounds like good > development. > > > Signed-off-by: Hirokazu Honda <hiroh@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()); The names are a bit confusing, I would expect maps_ to contained the mapped memory. How about doing that, and renaming chunks_ to planes_ ? > > } > > > > /** > > @@ -381,18 +382,36 @@ MappedBuffer::~MappedBuffer() > > */ > > MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags) > > { > > + if (buffer->planes().empty()) > > + return; > > Can we have a buffer with zero planes? Is this something that should > generate an Error or Warning message? > > (Or if it really implies there's a bug, a Fatal?) It's not supposed to happen, so I think an ASSERT() would be enough. > > maps_.reserve(buffer->planes().size()); > > > > + /* > > + * Below assumes planes are in the same buffer and planes are > > + * consecutive. > > + * \todo remove this assumption. > > To do that, will we need to store the PixelFormat information as part of > the FrameBuffer? Or at least some additional data there so that we know > it's layout? > > And - now I've seen Laurent posted a patch to expose a Format for that > purpose... I'd really like to avoid coupling FrameBuffer and FrameFormat. What we need if an offset field in FrameBuffer::Plane. With that, MappedFrameBuffer can check if the fds in different planes are identical, and if so, map the fd once only and use the offsets to compute addresses. I think that would be enough. Until we get that, could we verify that the assertion holds with an ASSERT() here ? An ASSERT(buffer->planes().size() == 1) should suffice. > > + */ > > + 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; > > } > > } > >
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; } }
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@chromium.org> --- include/libcamera/internal/framebuffer.h | 1 + src/libcamera/framebuffer.cpp | 41 +++++++++++++++++------- 2 files changed, 31 insertions(+), 11 deletions(-)