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

[Cheng-Hao Yang]

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.

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`.

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



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


> Thanks
>   j
>
> >       }
> >
> >       /*
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >
>

Thanks for the review!

Harvey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230918/9587244d/attachment.htm>