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

[Hirokazu Honda]

Hi Laurent, thank you for reviewing.

On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
> > >

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.

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

Best Regards,
-Hiro
> > [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