[libcamera-devel] [PATCH v1 1/1] Android adapter: CameraDevice fixes shared internal buffer

[Jacopo Mondi]

Hi Laurent

On Thu, Sep 21, 2023 at 12:08:57PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hello,
>
> The subject line should start with "android: ".
>
> On Tue, Sep 19, 2023 at 09:52:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > On Mon, Sep 18, 2023 at 12:18:34PM +0800, Cheng-Hao Yang via libcamera-devel wrote:
> > > On Thu, Sep 14, 2023 at 3:47 PM Jacopo Mondi wrote:
> > > > On Wed, Sep 13, 2023 at 03:20:50PM +0000, Harvey Yang via libcamera-devel wrote:
> > > > > From: Harvey Yang <chenghaoyang at chromium.org>
> > > > >
> > > > > In CameraDevice::processCaptureRequest, we might need to add an internal
> > > > > buffer for Mapped streams. This patch fixes a case that more than one
> > > > > Mapped streams depend on a stream that is not requested in one capture
> > > > > request.
> > > >
> > > > Ah! you're right! I wonder how it went unoticed... maybe we never had
> > > > to create two Mapped streams from a single buffer ? CTS has been run
> > > > multiple times but we never hit this
> > > >
> > > > > Change-Id: I37a1bcc9c4c2db666a90d74c39883ff18ed11bd5
> > > >
> > > > This shouldn't be here. We can remove it if you don't have to re-send
>
> On the other hand, a Fixes: line would be nice. I think
>
> Fixes: 7ea83eba0df6 ("android: camera_device: Postpone mapped streams handling")
>
> is the right one.
>
> > > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > ---
> > > > >  src/android/camera_device.cpp | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > index 1f7ce440..25cedd44 100644
> > > > > --- a/src/android/camera_device.cpp
> > > > > +++ b/src/android/camera_device.cpp
> > > > > @@ -1077,7 +1077,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > > >               descriptor->request_->addBuffer(sourceStream->stream(),
> > > > >                                               frameBuffer, nullptr);
> > > > >
> > > > > -             requestedStreams.erase(sourceStream);
> > > > > +             requestedStreams.insert(sourceStream);
> > > >
> > > > So, assuming two Mapped streams that map on the same cameraStream.
> > > >
> > > > The first processed one won't find a sourceStream in requestedStream
> > > >
> > > >         if (requestedStreams.find(sourceStream) != requestedStreams.end())
> > > >                 continue;
> > > >
> > > > so we don't continue and we add create an internal buffer for it and
> > > > add the framebuffer for the sourceStream to the requet
> > > >
> > > >         FrameBuffer *frameBuffer = cameraStream->getBuffer();
> > > >         buffer.internalBuffer = frameBuffer;
> > > >
> > > >         descriptor->request_->addBuffer(sourceStream->stream(),
> > > >                                         frameBuffer, nullptr);
> > > >
> > > > And this clearly was a nop because of the above if () statement
> > > >
> > > >         requestedStreams.erase(sourceStream);
> > > >
> > > > However, since the second one is a mapped stream too, don't we need to
> > > > allocate
> > > > an internal buffer for it ?
> > > >
> > > >                 FrameBuffer *frameBuffer = cameraStream->getBuffer();
> > > >                 buffer.internalBuffer = frameBuffer;
> > > >
> > > >
> > > Not really. The second stream can use the same internal buffer allocated by
> > > the first stream
> > > to continue the processing. If we allocate a new internal buffer, this will
> > > fail anyway:
> > >
> > >     descriptor->request_->addBuffer(sourceStream->stream(),
> > > 				    frameBuffer, nullptr);
> > >
> > > , as libcamera::Request already adds the internal buffer to the
> > > libcamera::Stream.
>
> It could be nice to capture a bit more context in the commit message, I
> had to read through the implementation to understand the issue fixed by
> this patch.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

With Harvey's ack I'll push with the following commit message

-------------------------------------------------------------------------------
android: camera_device: Fix requestedStream handling

The Android CameraDevice class adds a sourceStream for each Mapped
stream requested by the framework.

When mapping multiple framework streams to the same sourceStream, the
implementation of CameraDevice::processCaptureRequest wrongly erases the
just added sourceStream from the list of streams to request to
libcamera.

Fix this by adding the stream instead of erasing it.
-------------------------------------------------------------------------------

> > Inded, you're very right! I clearly was confused as I thought an
> > "internal buffer" had to be allocated, but as we're here handling
> > mapped streams the destination buffer is provided by the framework.
> >
> > Sorry for the noise.
> >
> > > The only purpose of setting the internal buffer is to return the allocated
> > > buffer
> > > to the CameraStream which created the buffer. See:
> > >
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1246
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1370
> > >
> > > There are no other usages of
> > > `Camera3RequesetDescriptor::StreamBuffer::internalBuffer`.
> >
> > ack
> >
> > > descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });
> > >
> > > This will ensure the mapped stream to be processed in:
> > >
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1236
> >
> > I re-read the code, but I can't figure out if there are issue in
> > processing 2 mapped streams created from the same internal buffer, as
> > I fear that code path has never really been tested ?
> >
> > > > With your patch applied I presume the second mapped stream will hit
> > > >
> > > >         if (requestedStreams.find(sourceStream) != requestedStreams.end())
> > > >                 continue;
> > > >
> > > > and continue, so no buffer will be allocated for it ?
> > > >
> > > > Have you got a test case for this to try ?
> > >
> > > Sorry, I've checked with Han-lin, and we don't have such a test with CrOS
> > > or CTS.
> >
> > Exactly, would be nice to test, but in the meantime, your patch seems
> > to be fixing a bug indeed.
> >
> > Thanks!
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > > > >       }
> > > > >
> > > > >       /*
>
> --
> Regards,
>
> Laurent Pinchart