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

[Cheng-Hao Yang]

Hi Jacopo,

On Tue, Sep 19, 2023 at 3:52 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:

> 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 ?
>
>
Yeah I understand. Logically, libcamera::Request doesn't care how the
buffer is created, while true that we don't test it at all yet.


> >
> >
> > > 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.
>
>
Hope it's worth merging now :)


> Thanks!
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> >
> > > Thanks
> > >   j
> > >
> > > >       }
> > > >
> > > >       /*
> > > > --
> > > > 2.42.0.283.g2d96d420d3-goog
> > > >
> > >
> >
> > Thanks for the review!
> >
> > Harvey
>

BR,
Harvey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230920/6e06771b/attachment.htm>