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

[Laurent Pinchart]

Hi Tomasz,

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 ?
> >
> > >       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.

> [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