[libcamera-devel] [PATCH v4 21/22] libcamera: base: shared_fd: Rename fd() to get()
Hirokazu Honda
hiroh at chromium.org
Wed Dec 1 06:32:15 CET 2021
Hi Laurent,
On Tue, Nov 30, 2021 at 5:09 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Laurent
>
> On Tue, Nov 30, 2021 at 05:38:19AM +0200, Laurent Pinchart wrote:
> > For consistency with UniqueFD, rename the fd() function to get().
> > Renaming UniqueFD::get() to fd() would have been another option, but was
> > rejected to keep as close as possible to the std::shared_ptr<> and
> > std::unique_ptr<> APIs.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>
> I like this!
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Thanks
> j
>
> > ---
> > include/libcamera/base/shared_fd.h | 2 +-
> > include/libcamera/internal/v4l2_videodevice.h | 2 +-
> > src/cam/drm.cpp | 4 +-
> > src/cam/image.cpp | 4 +-
> > src/gstreamer/gstlibcameraallocator.cpp | 2 +-
> > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> > src/libcamera/base/shared_fd.cpp | 4 +-
> > src/libcamera/framebuffer.cpp | 2 +-
> > src/libcamera/ipc_pipe.cpp | 2 +-
> > src/libcamera/mapped_framebuffer.cpp | 4 +-
> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-
> > src/libcamera/v4l2_videodevice.cpp | 6 +--
> > src/v4l2/v4l2_camera.cpp | 2 +-
> > test/shared-fd.cpp | 39 ++++++++++---------
> > 14 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/libcamera/base/shared_fd.h b/include/libcamera/base/shared_fd.h
> > index 1dd51a7f046b..e53a8b88601e 100644
> > --- a/include/libcamera/base/shared_fd.h
> > +++ b/include/libcamera/base/shared_fd.h
> > @@ -27,7 +27,7 @@ public:
> > SharedFD &operator=(SharedFD &&other);
> >
> > bool isValid() const { return fd_ != nullptr; }
> > - int fd() const { return fd_ ? fd_->fd() : -1; }
> > + int get() const { return fd_ ? fd_->fd() : -1; }
> > UniqueFD dup() const;
> >
> > private:
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 5ba2b54643ac..9b2ec3afecff 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -140,7 +140,7 @@ private:
> > private:
> > struct Plane {
> > Plane(const FrameBuffer::Plane &plane)
> > - : fd(plane.fd.fd()), length(plane.length)
> > + : fd(plane.fd.get()), length(plane.length)
> > {
> > }
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index f25300913a7f..46e34eb546fb 100644
> > --- a/src/cam/drm.cpp
> > +++ b/src/cam/drm.cpp
> > @@ -608,12 +608,12 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
> >
> > unsigned int i = 0;
> > for (const libcamera::FrameBuffer::Plane &plane : planes) {
> > - int fd = plane.fd.fd();
> > + int fd = plane.fd.get();
> > uint32_t handle;
> >
> > auto iter = fb->planes_.find(fd);
> > if (iter == fb->planes_.end()) {
> > - ret = drmPrimeFDToHandle(fd_, plane.fd.fd(), &handle);
> > + ret = drmPrimeFDToHandle(fd_, plane.fd.get(), &handle);
> > if (ret < 0) {
> > ret = -errno;
> > std::cerr
> > diff --git a/src/cam/image.cpp b/src/cam/image.cpp
> > index 0180be78d396..fe2cc6da5a15 100644
> > --- a/src/cam/image.cpp
> > +++ b/src/cam/image.cpp
> > @@ -39,7 +39,7 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const FrameBuffer *buffer, MapMode
> > std::map<int, MappedBufferInfo> mappedBuffers;
> >
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - const int fd = plane.fd.fd();
> > + const int fd = plane.fd.get();
> > if (mappedBuffers.find(fd) == mappedBuffers.end()) {
> > const size_t length = lseek(fd, 0, SEEK_END);
> > mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length };
> > @@ -61,7 +61,7 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const FrameBuffer *buffer, MapMode
> > }
> >
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - const int fd = plane.fd.fd();
> > + const int fd = plane.fd.get();
> > auto &info = mappedBuffers[fd];
> > if (!info.address) {
> > void *address = mmap(nullptr, info.mapLength, mmapFlags,
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > index cb07d6e9e87f..c740b8fc82a8 100644
> > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -52,7 +52,7 @@ FrameWrap::FrameWrap(GstAllocator *allocator, FrameBuffer *buffer,
> > outstandingPlanes_(0)
> > {
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.fd(),
> > + GstMemory *mem = gst_fd_allocator_alloc(allocator, plane.fd.get(),
> > plane.offset + plane.length,
> > GST_FD_MEMORY_FLAG_DONT_CLOSE);
> > gst_memory_resize(mem, plane.offset, plane.length);
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index aaf629eeb3fc..0ed41385018a 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -377,7 +377,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> > if (lsTableHandle_.isValid()) {
> > lsTable_ = mmap(nullptr, ipa::RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> > - MAP_SHARED, lsTableHandle_.fd(), 0);
> > + MAP_SHARED, lsTableHandle_.get(), 0);
> >
> > if (lsTable_ == MAP_FAILED) {
> > LOG(IPARPI, Error) << "dmaHeap mmap failure for LS table.";
> > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> > index 56dc579258c7..0c8b93a47f85 100644
> > --- a/src/libcamera/base/shared_fd.cpp
> > +++ b/src/libcamera/base/shared_fd.cpp
> > @@ -208,7 +208,7 @@ SharedFD &SharedFD::operator=(SharedFD &&other)
> > */
> >
> > /**
> > - * \fn SharedFD::fd()
> > + * \fn SharedFD::get()
> > * \brief Retrieve the numerical file descriptor
> > * \return The numerical file descriptor, which may be -1 if the SharedFD
> > * instance is invalid
> > @@ -253,7 +253,7 @@ SharedFD &SharedFD::operator=(SharedFD &&other)
> > */
> > UniqueFD SharedFD::dup() const
> > {
> > - int dupFd = ::dup(fd());
> > + int dupFd = ::dup(get());
> > if (dupFd == -1) {
> > int ret = -errno;
> > LOG(SharedFD, Error)
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index f313566c5561..701212f3ae21 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -218,7 +218,7 @@ ino_t fileDescriptorInode(const SharedFD &fd)
> > return 0;
> >
> > struct stat st;
> > - int ret = fstat(fd.fd(), &st);
> > + int ret = fstat(fd.get(), &st);
> > if (ret < 0) {
> > ret = -errno;
> > LOG(Buffer, Fatal)
> > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp
> > index 3b47032de0a2..31a0ca09f9f0 100644
> > --- a/src/libcamera/ipc_pipe.cpp
> > +++ b/src/libcamera/ipc_pipe.cpp
> > @@ -113,7 +113,7 @@ IPCUnixSocket::Payload IPCMessage::payload() const
> > }
> >
> > for (const SharedFD &fd : fds_)
> > - payload.fds.push_back(fd.fd());
> > + payload.fds.push_back(fd.get());
> >
> > return payload;
> > }
> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> > index 464d35fef2d2..6860069b68ca 100644
> > --- a/src/libcamera/mapped_framebuffer.cpp
> > +++ b/src/libcamera/mapped_framebuffer.cpp
> > @@ -198,7 +198,7 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > std::map<int, MappedBufferInfo> mappedBuffers;
> >
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - const int fd = plane.fd.fd();
> > + const int fd = plane.fd.get();
> > if (mappedBuffers.find(fd) == mappedBuffers.end()) {
> > const size_t length = lseek(fd, 0, SEEK_END);
> > mappedBuffers[fd] = MappedBufferInfo{ nullptr, 0, length };
> > @@ -220,7 +220,7 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> > }
> >
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > - const int fd = plane.fd.fd();
> > + const int fd = plane.fd.get();
> > auto &info = mappedBuffers[fd];
> > if (!info.address) {
> > void *address = mmap(nullptr, info.mapLength, mmapFlags,
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index fdcd1eca11d6..fa2848533b29 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1470,7 +1470,7 @@ void RPiCameraData::setIspControls(const ControlList &controls)
> > Span<uint8_t> s = value.data();
> > bcm2835_isp_lens_shading *ls =
> > reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());
> > - ls->dmabuf = lsTable_.fd();
> > + ls->dmabuf = lsTable_.get();
> > }
> >
> > isp_[Isp::Input].dev()->setControls(&ctrls);
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 0d214d9edc7a..b4b89e2759b9 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -282,7 +282,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> > return false;
> >
> > for (unsigned int i = 0; i < planes.size(); i++)
> > - if (planes_[i].fd != planes[i].fd.fd() ||
> > + if (planes_[i].fd != planes[i].fd.get() ||
> > planes_[i].length != planes[i].length)
> > return false;
> > return true;
> > @@ -1517,9 +1517,9 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> > if (buf.memory == V4L2_MEMORY_DMABUF) {
> > if (multiPlanar) {
> > for (unsigned int p = 0; p < numV4l2Planes; ++p)
> > - v4l2Planes[p].m.fd = planes[p].fd.fd();
> > + v4l2Planes[p].m.fd = planes[p].fd.get();
> > } else {
> > - buf.m.fd = planes[0].fd.fd();
> > + buf.m.fd = planes[0].fd.get();
> > }
> > }
> >
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 464507505c79..e922b9e6aab2 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -195,7 +195,7 @@ int V4L2Camera::getBufferFd(unsigned int index)
> > if (buffers.size() <= index)
> > return -1;
> >
> > - return buffers[index]->planes()[0].fd.fd();
> > + return buffers[index]->planes()[0].fd.get();
> > }
> >
> > int V4L2Camera::streamOn()
> > diff --git a/test/shared-fd.cpp b/test/shared-fd.cpp
> > index 60e5d0aaa395..997d7be18c47 100644
> > --- a/test/shared-fd.cpp
> > +++ b/test/shared-fd.cpp
> > @@ -46,7 +46,7 @@ protected:
> > /* Test creating empty SharedFD. */
> > desc1_ = new SharedFD();
> >
> > - if (desc1_->fd() != -1) {
> > + if (desc1_->get() != -1) {
> > std::cout << "Failed fd numerical check (default constructor)"
> > << std::endl;
> > return TestFail;
> > @@ -60,19 +60,19 @@ protected:
> > * descriptor.
> > */
> > desc1_ = new SharedFD(fd_);
> > - if (desc1_->fd() == fd_) {
> > + if (desc1_->get() == fd_) {
> > std::cout << "Failed fd numerical check (lvalue ref constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (!isValidFd(fd_) || !isValidFd(desc1_->fd())) {
> > + if (!isValidFd(fd_) || !isValidFd(desc1_->get())) {
> > std::cout << "Failed fd validity after construction (lvalue ref constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - int fd = desc1_->fd();
> > + int fd = desc1_->get();
> >
> > delete desc1_;
> > desc1_ = nullptr;
> > @@ -91,19 +91,19 @@ protected:
> > int dupFdCopy = dupFd;
> >
> > desc1_ = new SharedFD(std::move(dupFd));
> > - if (desc1_->fd() != dupFdCopy) {
> > + if (desc1_->get() != dupFdCopy) {
> > std::cout << "Failed fd numerical check (rvalue ref constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (dupFd != -1 || !isValidFd(fd_) || !isValidFd(desc1_->fd())) {
> > + if (dupFd != -1 || !isValidFd(fd_) || !isValidFd(desc1_->get())) {
> > std::cout << "Failed fd validity after construction (rvalue ref constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - fd = desc1_->fd();
> > + fd = desc1_->get();
> >
> > delete desc1_;
> > desc1_ = nullptr;
> > @@ -118,13 +118,14 @@ protected:
> > desc1_ = new SharedFD(fd_);
> > desc2_ = new SharedFD(*desc1_);
> >
> > - if (desc1_->fd() == fd_ || desc2_->fd() == fd_ || desc1_->fd() != desc2_->fd()) {
> > + if (desc1_->get() == fd_ || desc2_->get() == fd_ ||
> > + desc1_->get() != desc2_->get()) {
> > std::cout << "Failed fd numerical check (copy constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (!isValidFd(desc1_->fd()) || !isValidFd(desc2_->fd())) {
> > + if (!isValidFd(desc1_->get()) || !isValidFd(desc2_->get())) {
> > std::cout << "Failed fd validity after construction (copy constructor)"
> > << std::endl;
> > return TestFail;
> > @@ -133,7 +134,7 @@ protected:
> > delete desc1_;
> > desc1_ = nullptr;
> >
> > - if (!isValidFd(desc2_->fd())) {
> > + if (!isValidFd(desc2_->get())) {
> > std::cout << "Failed fd validity after destruction (copy constructor)"
> > << std::endl;
> > return TestFail;
> > @@ -144,16 +145,16 @@ protected:
> >
> > /* Test creating SharedFD by taking over other SharedFD. */
> > desc1_ = new SharedFD(fd_);
> > - fd = desc1_->fd();
> > + fd = desc1_->get();
> > desc2_ = new SharedFD(std::move(*desc1_));
> >
> > - if (desc1_->fd() != -1 || desc2_->fd() != fd) {
> > + if (desc1_->get() != -1 || desc2_->get() != fd) {
> > std::cout << "Failed fd numerical check (move constructor)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (!isValidFd(desc2_->fd())) {
> > + if (!isValidFd(desc2_->get())) {
> > std::cout << "Failed fd validity after construction (move constructor)"
> > << std::endl;
> > return TestFail;
> > @@ -168,16 +169,16 @@ protected:
> > desc1_ = new SharedFD();
> > desc2_ = new SharedFD(fd_);
> >
> > - fd = desc2_->fd();
> > + fd = desc2_->get();
> > *desc1_ = *desc2_;
> >
> > - if (desc1_->fd() != fd || desc2_->fd() != fd) {
> > + if (desc1_->get() != fd || desc2_->get() != fd) {
> > std::cout << "Failed fd numerical check (copy assignment)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (!isValidFd(desc1_->fd()) || !isValidFd(desc2_->fd())) {
> > + if (!isValidFd(desc1_->get()) || !isValidFd(desc2_->get())) {
> > std::cout << "Failed fd validity after construction (copy assignment)"
> > << std::endl;
> > return TestFail;
> > @@ -192,16 +193,16 @@ protected:
> > desc1_ = new SharedFD();
> > desc2_ = new SharedFD(fd_);
> >
> > - fd = desc2_->fd();
> > + fd = desc2_->get();
> > *desc1_ = std::move(*desc2_);
> >
> > - if (desc1_->fd() != fd || desc2_->fd() != -1) {
> > + if (desc1_->get() != fd || desc2_->get() != -1) {
> > std::cout << "Failed fd numerical check (move assignment)"
> > << std::endl;
> > return TestFail;
> > }
> >
> > - if (!isValidFd(desc1_->fd())) {
> > + if (!isValidFd(desc1_->get())) {
> > std::cout << "Failed fd validity after construction (move assignment)"
> > << std::endl;
> > return TestFail;
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
More information about the libcamera-devel
mailing list