<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:<br>
> V4L2Camera::getBufferFd() returns FileDescriptor. However, the<br>
> file descriptor is still owned by V4L2Camera. It should rather<br>
> return an integer to represent V4L2Camera doesn't have the<br>
> ownership of the file descriptor.<br>
<br>
The FileDescriptor class doesn't imply ownership, as it's similar to a<br>
shared_ptr<>. I'm not against this patch, but why do you think<br>
FileDescriptor is bad here ?<br>
<br></blockquote><div><br></div><div>IMO, returning FileDescriptor implies that a caller shares the ownership of it.</div><div>It enables a caller to keep it outlive a callee and even share its ownership with others.</div><div>What do you think?</div><div><br></div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  src/v4l2/v4l2_camera.cpp       | 6 +++---<br>
>  src/v4l2/v4l2_camera.h         | 2 +-<br>
>  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---<br>
>  3 files changed, 7 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp<br>
> index 97825c71..cccc6749 100644<br>
> --- a/src/v4l2/v4l2_camera.cpp<br>
> +++ b/src/v4l2/v4l2_camera.cpp<br>
> @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()<br>
>       bufferAllocator_->free(stream);<br>
>  }<br>
>  <br>
> -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)<br>
> +int V4L2Camera::getBufferFd(unsigned int index)<br>
>  {<br>
>       Stream *stream = config_->at(0).stream();<br>
>       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =<br>
>               bufferAllocator_->buffers(stream);<br>
>  <br>
>       if (buffers.size() <= index)<br>
> -             return FileDescriptor();<br>
> +             return -1;<br>
>  <br>
> -     return buffers[index]->planes()[0].fd;<br>
> +     return buffers[index]->planes()[0].fd.fd();<br>
>  }<br>
>  <br>
>  int V4L2Camera::streamOn()<br>
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h<br>
> index d2380462..8efe1642 100644<br>
> --- a/src/v4l2/v4l2_camera.h<br>
> +++ b/src/v4l2/v4l2_camera.h<br>
> @@ -53,7 +53,7 @@ public:<br>
>  <br>
>       int allocBuffers(unsigned int count);<br>
>       void freeBuffers();<br>
> -     FileDescriptor getBufferFd(unsigned int index);<br>
> +     int getBufferFd(unsigned int index);<br>
>  <br>
>       int streamOn();<br>
>       int streamOff();<br>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp<br>
> index f8bfe595..8f417421 100644<br>
> --- a/src/v4l2/v4l2_camera_proxy.cpp<br>
> +++ b/src/v4l2/v4l2_camera_proxy.cpp<br>
> @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,<br>
>               return MAP_FAILED;<br>
>       }<br>
>  <br>
> -     FileDescriptor fd = vcam_->getBufferFd(index);<br>
> -     if (!fd.isValid()) {<br>
> +     int fd = vcam_->getBufferFd(index);<br>
> +     if (fd < 0) {<br>
>               errno = EINVAL;<br>
>               return MAP_FAILED;<br>
>       }<br>
>  <br>
>       void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,<br>
> -                                                            flags, fd.fd(), 0);<br>
> +                                                            flags, fd, 0);<br>
>       if (map == MAP_FAILED)<br>
>               return map;<br>
>  <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>