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

[Laurent Pinchart]

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>

> 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