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

19003 diff mbox series

From: Harvey Yang <chenghaoyang@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.

Change-Id: I37a1bcc9c4c2db666a90d74c39883ff18ed11bd5
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/camera_device.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Harvey

On Wed, Sep 13, 2023 at 03:20:50PM +0000, Harvey Yang via libcamera-devel wrote:
> From: Harvey Yang <chenghaoyang@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@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;

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 ?

Thanks
  j

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

Hi Jacopo,

On Thu, Sep 14, 2023 at 3:47 PM Jacopo Mondi <jacopo.mondi@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@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@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

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@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@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@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@ideasonboard.com>

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

Hi Jacopo,

On Tue, Sep 19, 2023 at 3:52 PM Jacopo Mondi <jacopo.mondi@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@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@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@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@ideasonboard.com>
>
> >
> > > Thanks
> > >   j
> > >
> > > >       }
> > > >
> > > >       /*
> > > > --
> > > > 2.42.0.283.g2d96d420d3-goog
> > > >
> > >
> >
> > Thanks for the review!
> >
> > Harvey
>

BR,
Harvey

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@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@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@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@ideasonboard.com>
> 
> > > >       }
> > > >
> > > >       /*

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@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@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@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@ideasonboard.com>
> >
> > > > >       }
> > > > >
> > > > >       /*
>
> --
> Regards,
>
> Laurent Pinchart

Thanks Jacopo! LGTM.
Also thanks Laurent for the suggestions.

BR,
Harvey

On Thu, Sep 21, 2023 at 6:50 PM Jacopo Mondi <jacopo.mondi@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@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@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@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@ideasonboard.com>
> > >
> > > > > >       }
> > > > > >
> > > > > >       /*
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>