[libcamera-devel] [PATCH v1 1/1] Android adapter: CameraDevice fixes shared internal buffer
Cheng-Hao Yang
chenghaoyang at chromium.org
Thu Sep 21 14:13:15 CEST 2023
Thanks Jacopo! LGTM.
Also thanks Laurent for the suggestions.
BR,
Harvey
On Thu, Sep 21, 2023 at 6:50 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230921/2365c317/attachment.htm>
More information about the libcamera-devel
mailing list