[libcamera-devel] [PATCH 3/3] android: camera_device: Configure one stream for identical stream requests

[Jacopo Mondi]

Hi Hiro,

On Thu, Aug 05, 2021 at 10:45:30PM +0900, Hirokazu Honda wrote:
> An Android HAL client may request identical stream requests. It is
> redundant that a native camera device produces a separate stream for
> each of the identical requests.
> Configure camera one stream configuration for the identical stream
> requests.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 27 ++++++++++++++++++++++-----
>  src/android/camera_device.h   |  3 +--
>  src/android/camera_stream.cpp |  3 ++-
>  3 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 2466122d..68b2840c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -618,14 +618,32 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			continue;
>  		}
>
> +		/* This stream will be produced by hardware. */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
> +
> +		/*
> +		 * If they are the same capture request, associate with the same
> +		 * CameraStream.

                /*
                 * If a CameraStream with the same size and format of
                 * the current stream has already been requested
                 * associate the two.
                 */

> +		 */
> +		auto iter = std::find_if(
> +			streamConfigs.begin(), streamConfigs.end(),
> +			[size, format](const Camera3StreamConfig &streamConfig) {
> +				return streamConfig.config.size == size &&
> +				       streamConfig.config.pixelFormat == format;
> +			});

So this creates a mapped stream only if both the format AND the size
are identical. Shouldn't we use the YUV post-processor to downscale
NV12-to-NV12 as well ? Even if not part of this patch, the logic here
implemented won't work, unless the camera3_streams are presented to
the libcamera HAL ordered by size, doens't it ?

> +		if (iter != streamConfigs.end()) {
> +			/* Add usage to copy the buffer in streams[0] to stream. */
> +			iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> +			stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;

Should the GRALLOC_USAGE_HW_CAMERA_WRITE be kept or should it be
overridden ?


> +			iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> +			continue;
> +		}
> +
>  		Camera3StreamConfig streamConfig;
>  		streamConfig.streams = { { stream, CameraStream::Type::Direct } };
>  		streamConfig.config.size = size;
>  		streamConfig.config.pixelFormat = format;
>  		streamConfigs.push_back(std::move(streamConfig));
> -
> -		/* This stream will be produced by hardware. */
> -		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;

Depending on the answer to the previous question, should this be
moved ?

>  	}
>
>  	/* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1095,12 +1113,11 @@ void CameraDevice::requestComplete(Request *request)
>  		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
>  	}
>
> -	/* Handle any JPEG compression. */

I would keep the comment as something like

        /* Handle post-processing. */

>  	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
>
> -		if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> +		if (cameraStream->type() == CameraStream::Type::Direct)

This will accept Type::Internal. Is this intended ? Shouldn't this be
                if (cameraStream->type() != CameraStream::Type::Mapped)

>  			continue;
>
>  		FrameBuffer *src = request->findBuffer(cameraStream->stream());
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index cbc71be4..c5f96d32 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -48,6 +48,7 @@ public:
>
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> +	const CameraCapabilities *capabilties() const { return &capabilities_; }

Should be moved to 1/3

>  	const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
>
>  	const std::string &maker() const { return maker_; }
> @@ -63,8 +64,6 @@ public:
>  	int processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request);
>
> -	libcamera::PixelFormat toPixelFormat(int format) const;
> -

And this should be dropped from 1/3

>  protected:
>  	std::string logPrefix() const override;
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 8c02cb43..d880cc18 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -8,6 +8,7 @@
>  #include "camera_stream.h"
>
>  #include "camera_buffer.h"
> +#include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
>  #include "jpeg/post_processor_jpeg.h"
> @@ -62,7 +63,7 @@ int CameraStream::configure()
>  {
>  	if (type_ == Type::Internal || type_ == Type::Mapped) {
>  		const PixelFormat outFormat =
> -			cameraDevice_->toPixelFormat(camera3Stream_->format);
> +			cameraDevice_->capabilties()->toPixelFormat(camera3Stream_->format);
>  		StreamConfiguration output = configuration();
>  		output.pixelFormat = outFormat;
>  		switch (outFormat) {

Overall a very nice step in the right direction!

Thanks
   j

> --
> 2.32.0.554.ge1b32706d8-goog
>