[libcamera-devel] [RFC PATCH 09/10] android: camera_device: Fill offset and right length in CreateFrameBuffer()

[Laurent Pinchart]

Hi Hiro,

On Fri, Aug 20, 2021 at 08:31:59PM +0900, Hirokazu Honda wrote:
> On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart wrote:
> > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote:
> > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote:
> > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:
> > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:
> > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:
> > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to
> > > > > > > each FrameBuffer::Plane::length. It should rather be the length of
> > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of
> > > > > > > FrameBuffer::Plane.
> > > > > > >
> > > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > > ---
> > > > > > >  src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------
> > > > > > >  1 file changed, 20 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > > index a69b687a..1ded5cb1 100644
> > > > > > > --- a/src/android/camera_device.cpp
> > > > > > > +++ b/src/android/camera_device.cpp
> > > > > > > @@ -12,6 +12,7 @@
> > > > > > >
> > > > > > >  #include <algorithm>
> > > > > > >  #include <fstream>
> > > > > > > +#include <sys/mman.h>
> > > > > > >  #include <unistd.h>
> > > > > > >  #include <vector>
> > > > > > >
> > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > > >
> > > > > > >  FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)
> > > > > > >  {
> > > > > > > -     std::vector<FrameBuffer::Plane> planes;
> > > > > > > +     FileDescriptor fd;
> > > > > > > +     /* This assumes all the planes are in the same buffer. */
> > > > > >
> > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption
> > > > > > always hold ?
> > > > > >
> > >
> > > I think this assumption holds at least on ChromeOS.
> > > Hmm, ASSERT() is difficult because fds are duplicated and thus
> > > different from each other in terms of integer.
> >
> > So Chrome OS will give different fds, all referring to the same dmabuf ?
> 
> I didn't know they are always dup()ed. It depends on HAL client implementation.
> +Ricky, do you know about it?

For what it's worth, I believe you can check if two different fds refer
to the same dmabuf by comparing their inode number, as returned by
fstat(). This is something that should likely be done in
V4L2VideoDevice::queueBuffer(), as from the point of view of the
libcamera public API, we want to support different buffers and offsets.

> > > > > > >       for (int i = 0; i < camera3buffer->numFds; i++) {
> > > > > > > -             /* Skip unused planes. */
> > > > > > > -             if (camera3buffer->data[i] == -1)
> > > > > > > +             if (camera3buffer->data[i] != -1) {
> > > > > > > +                     fd = FileDescriptor(camera3buffer->data[i]);
> > > > > > >                       break;
> > > > > > > -
> > > > > > > -             FrameBuffer::Plane plane;
> > > > > > > -             plane.fd = FileDescriptor(camera3buffer->data[i]);
> > > > > > > -             if (!plane.fd.isValid()) {
> > > > > > > -                     LOG(HAL, Error) << "Failed to obtain FileDescriptor ("
> > > > > > > -                                     << camera3buffer->data[i] << ") "
> > > > > > > -                                     << " on plane " << i;
> > > > > > > -                     return nullptr;
> > > > > > >               }
> > > > > > > +     }
> > > > > >
> > > > > > Blank line.
> > > > > >
> > > > > > > +     if (!fd.isValid()) {
> > > > > > > +             LOG(HAL, Fatal) << "No valid fd";
> > > > > > > +             return nullptr;
> > > > > > > +     }
> > > > > > >
> > > > > > > -             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
> > > > > > > -             if (length == -1) {
> > > > > > > -                     LOG(HAL, Error) << "Failed to query plane length";
> > > > > > > -                     return nullptr;
> > > > > > > -             }
> > > > > > > +     CameraBuffer buf(camera3buffer, PROT_READ);
> > > > > >
> > > > > > I suppose there's no other way than using the cros::CameraBufferManager
> > > > > > to get the necessary information. I'm annoyed that this involves mapping
> > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive
> > > > > > operation :-( It's of course not a prerequisite for this series, but I'm
> > > > > > wondering if the cros::CameraBufferManager API could be improved to
> > > > > > separate retrieving plane information from mapping the buffer. What do
> > > > > > you think ?
> > > > >
> > > > > Which mapping are you referring to?
> > > > >
> > > > > Also, there are already methods to retrieve this information, see
> > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].
> > > >
> > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer
> > > > constructor, but that's not required to get the plane size or offset. We
> > > > should thus be able to improve the implementation here, without any
> > > > change on the CrOS side. Thanks for the feedback.
> > >
> > > I actually tried to add the option to not map in the constructor
> > > https://patchwork.libcamera.org/patch/11837/.
> > > Would you tell me your preferred way?
> >
> > I had missed that, sorry. I've now replied to that patch.
> >
> > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305
> > > > >
> > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct
> > > > > > information ?
> > > > > >
> > > > > > > +     if (!buf.isValid()) {
> > > > > > > +             LOG(HAL, Fatal) << "Failed mapping buffer";
> > > > > > > +             return nullptr;
> > > > > > > +     }
> > > > > > >
> > > > > > > -             plane.length = length;
> > > > > > > -             planes.push_back(std::move(plane));
> > > > > > > +     std::vector<FrameBuffer::Plane> planes(buf.numPlanes());
> > > > > > > +     for (size_t i = 0; i < buf.numPlanes(); ++i) {
> > > > > > > +             planes[i].fd = fd;
> > > > > > > +             planes[i].offset = buf.plane(i).data() - buf.plane(0).data();
> > > > > > > +             planes[i].length = buf.plane(i).size();
> > > > > > >       }
> > > > > > >
> > > > > > >       return new FrameBuffer(std::move(planes));

-- 
Regards,

Laurent Pinchart