[libcamera-devel] [RFC PATCH 7/8] android: camera_device: Query plane length

[Tomasz Figa]

On Fri, Jul 24, 2020 at 7:16 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> (CC'ing Tomasz to get a review from a Chrome OS point of view)
>
> Thank you for the patch.
>
> On Mon, Jul 20, 2020 at 11:42:31PM +0100, Kieran Bingham wrote:
> > Use lseek to query the length of planes where possible rather than leaving
> > the plane.length as zero, which prevents mapping buffers for software
> > processing.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >
> > I like this one ;-) We need to know the plane lengths for software
> > streams now, but the correct way to get this would be to talk to the
> > gralloc allocator. This would mean pulling in various extra
> > cros-specific libraries, where instead we can query the size of the
> > dmabuf by getting the offset when we SEEK_END.
> >
> >
> >  src/android/camera_device.cpp | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 538b8ab5da03..6212ccdd61ec 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1045,12 +1045,18 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >                       continue;
> >               }
> >
> > -             /*
> > -              * Setting length to zero here is OK as the length is only used
> > -              * to map the memory of the plane. Libcamera do not need to poke
> > -              * at the memory content queued by the HAL.
> > -              */
> > -             plane.length = 0;
> > +             off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
> > +             if (length == -1) {
> > +                     /*
> > +                      * A zero length plane is not fatal unless the
> > +                      * FrameBuffer is used for a software stream, libcamera
> > +                      * itself will not access the internal frame content.
> > +                      */
> > +                     LOG(HAL, Error) << "Failed to query plane length";
> > +                     length = 0;
>
> Do you expect this to happen ? If not, I'd return an error. If yes, an
> error message would be printed in a loop, which isn't very nice. My
> preference would be to return an error if possible.
>
> Otherwise, nice hack :-)

A zero-length plane would be strange and could suggest that the
backing file is not a DMA-buf, so it could indeed make sense to error
out.

Otherwise, we tend to do this in a number of places in Chrome OS and
seems to be the official way to query a DMA-buf for its size. However
I wonder if we shouldn't restore the original position after the seek?

Best regards,
Tomasz