[libcamera-devel] [RFC PATCH 10/10] v4l2: V4L2Camera: Return int in getBufferFd()

[Hirokazu Honda]

Hi Laurent,

On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> > V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> > file descriptor is still owned by V4L2Camera. It should rather
> > return an integer to represent V4L2Camera doesn't have the
> > ownership of the file descriptor.
>
> The FileDescriptor class doesn't imply ownership, as it's similar to a
> shared_ptr<>. I'm not against this patch, but why do you think
> FileDescriptor is bad here ?
>
>
IMO, returning FileDescriptor implies that a caller shares the ownership of
it.
It enables a caller to keep it outlive a callee and even share its
ownership with others.
What do you think?

-Hiro

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/v4l2/v4l2_camera.cpp       | 6 +++---
> >  src/v4l2/v4l2_camera.h         | 2 +-
> >  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 97825c71..cccc6749 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
> >       bufferAllocator_->free(stream);
> >  }
> >
> > -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> > +int V4L2Camera::getBufferFd(unsigned int index)
> >  {
> >       Stream *stream = config_->at(0).stream();
> >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
> >               bufferAllocator_->buffers(stream);
> >
> >       if (buffers.size() <= index)
> > -             return FileDescriptor();
> > +             return -1;
> >
> > -     return buffers[index]->planes()[0].fd;
> > +     return buffers[index]->planes()[0].fd.fd();
> >  }
> >
> >  int V4L2Camera::streamOn()
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > index d2380462..8efe1642 100644
> > --- a/src/v4l2/v4l2_camera.h
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -53,7 +53,7 @@ public:
> >
> >       int allocBuffers(unsigned int count);
> >       void freeBuffers();
> > -     FileDescriptor getBufferFd(unsigned int index);
> > +     int getBufferFd(unsigned int index);
> >
> >       int streamOn();
> >       int streamOff();
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp
> b/src/v4l2/v4l2_camera_proxy.cpp
> > index f8bfe595..8f417421 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t
> length, int prot, int flags,
> >               return MAP_FAILED;
> >       }
> >
> > -     FileDescriptor fd = vcam_->getBufferFd(index);
> > -     if (!fd.isValid()) {
> > +     int fd = vcam_->getBufferFd(index);
> > +     if (fd < 0) {
> >               errno = EINVAL;
> >               return MAP_FAILED;
> >       }
> >
> >       void *map = V4L2CompatManager::instance()->fops().mmap(addr,
> length, prot,
> > -                                                            flags,
> fd.fd(), 0);
> > +                                                            flags, fd,
> 0);
> >       if (map == MAP_FAILED)
> >               return map;
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/c287b252/attachment.htm>