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

[Jacopo Mondi]

Hi Harvey

On Mon, Sep 18, 2023 at 12:18:34PM +0800, Cheng-Hao Yang via libcamera-devel wrote:
> Hi Jacopo,
>
> On Thu, Sep 14, 2023 at 3:47 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hi Harvey
> >
> > 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
> >
> > > 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.

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>

>
> > Thanks
> >   j
> >
> > >       }
> > >
> > >       /*
> > > --
> > > 2.42.0.283.g2d96d420d3-goog
> > >
> >
>
> Thanks for the review!
>
> Harvey