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

[Jacopo Mondi]

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;

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
>