[libcamera-devel] [RFC PATCH] android: camera_device: Reorder configurations given by a HAL client

Jacopo Mondi jacopo at jmondi.org
Wed Nov 4 15:48:21 CET 2020


Hi Hiro,
   sorry for the delay.

Not a really detailed review but more generic questions on the design

On Wed, Oct 28, 2020 at 07:01:34PM +0900, Hirokazu Honda wrote:
> Reorder configurations given by a HAL client in configureStreams()
> so that CameraDevice and pipeline handlers can accept them as much
> as possible.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-
>  src/android/camera_device.h   |   3 +-
>  2 files changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ca60f51..eee7248 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -9,6 +9,7 @@
>  #include "camera_ops.h"
>  #include "post_processor.h"
>
> +#include <string.h>
>  #include <sys/mman.h>
>  #include <tuple>
>  #include <vector>
> @@ -132,6 +133,16 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
>
>  LOG_DECLARE_CATEGORY(HAL)
>
> +void printStreamList(const camera3_stream_configuration_t& streamList) {
> +	LOG(HAL, Error) << "num_streams=" << streamList.num_streams;
> +	for (unsigned int i = 0; i < streamList.num_streams; i++) {
> +		const auto* stream = streamList.streams[i];
> +		LOG(HAL, Error) << i << ": width=" << stream->width
> +				<< ", height=" << stream->height
> +				<< ", format=" << stream->format;
> +	}
> +}
> +
>  MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  					 int flags)
>  {
> @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
>  	return requestTemplate->get();
>  }
>
> -PixelFormat CameraDevice::toPixelFormat(int format)
> +PixelFormat CameraDevice::toPixelFormat(int format) const
>  {
>  	/* Translate Android format code to libcamera pixel format. */
>  	auto it = formatsMap_.find(format);
> @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>  	return it->second;
>  }
>
> +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const

Is the intended sorting order of the Android streams the following one?

 { largest NV12, JPEG, NV12 same as JPEG, other streams.. }

It see you are sorting the stream requested by Android.
I might see the reason to do so, as StreamConfiguration are added to
the CameraConfiguration while we walk the Android streams, so
presenting the android streams ordered has the effect of ordering the
StreamConfiguration consequentally. Is this the intent ?

In example:

{ NV12-max, JPEG-vga, NV12-vga, .. }

works because the CameraDevice::configureStreams() function will associate
JPEG-vga with NV12-vga and the resulting StreamConfiguration will be

{ NV12-max, NV12-vga, .. }

If there's no NV12-vga to generate JPEG-vga from, if I'm not mistaken,
the result will be

{ NV12-max, ... , NV12-vga }

as configureStreams inspects the JPEG streams after all the other
ones, and eventually adds an NV12 stream to generate JPEG streams
from.

Apart from this, what concerns me a bit is that this ordering is meant
to satisfy the configureStreams() implementation, and if one changes,
the one has to change as well, as they are interdependent.

If the final goal is to be able to sort the StreamConfiguration before
presenting them to the libcamera::Camera I wonder if it wouldn't be
better to rework configureStream() to:
1) Store StreamConfiguration in an std::vector<>
2) Order the vector of StreamConfiguration with the same logic you
   have here
3) Add the sorted StreamConfiguration to the CameraConfiguration

This implies the CameraStream class will have to change, as the index
of the associated StreamConfiguration will have to be injected later,
after the StreamConfiguration is CameraConfiguration::add() to the
CameraConfiguration. It's quite a substantial change, but I feel like
it would be more robust than sorting the android stream to please code
that we're in control of.

What do you think ?


> +{
> +	std::map<uint32_t, std::vector<const camera3_stream_t*>> streamMap;
> +	for (unsigned int i = 0; i < streamList->num_streams; ++i) {
> +		const camera3_stream_t* stream = streamList->streams[i];
> +		PixelFormat format = toPixelFormat(stream->format);
> +		if (!format.isValid())
> +			return -EINVAL;
> +		streamMap[format.fourcc()].push_back(stream);
> +	}
> +	std::vector<uint32_t> formats;
> +	for (auto& s: streamMap) {
> +		formats.push_back(s.first);
> +		auto& streams = s.second;
> +		/* Sorted by resolution. Smaller is put first. */
> +		std::sort(streams.begin(), streams.end(),
> +			  [](const camera3_stream_t* streamA,
> +			     const camera3_stream_t* streamB) {
> +				  if (streamA->width != streamB->width)
> +					  return streamA->width < streamB->width;
> +				  return streamA->height < streamB->height;
> +			  });
> +	}
> +
> +	if (streamMap.find(formats::MJPEG) != streamMap.end() &&
> +	    streamMap[formats::MJPEG].size() > 1) {
> +		LOG(HAL, Error) << "Multiple JPEG streams are not supported";
> +		return -EINVAL;
> +	}
> +
> +	std::vector<const camera3_stream_t*> sortedStreams;
> +	/*
> +	 * NV12 is the most prioritized format. Put the configuration with NV12
> +	 * and the largest resolution first.
> +	 */
> +	auto nv12StreamsIt = streamMap.find(formats::NV12);
> +	if (nv12StreamsIt != streamMap.end()) {
> +		auto& nv12Streams = nv12StreamsIt->second;
> +		const camera3_stream_t* nv12Stream = nv12Streams.back();
> +		sortedStreams.push_back(nv12Stream);
> +		nv12Streams.pop_back();
> +	}
> +
> +	/*
> +	 * If JPEG is there, then put the JPEG configuration second and a
> +	 * configuration whose specified stream the JPEG is encoded from third
> +	 * if there is.
> +	 */
> +	auto jpegStreamsIt = streamMap.find(formats::MJPEG);
> +	if (jpegStreamsIt != streamMap.end()) {
> +		auto& jpegStreams = jpegStreamsIt->second;
> +		const camera3_stream_t* jpegStream = jpegStreams[0];
> +		sortedStreams.push_back(jpegStream);
> +		streamMap.erase(jpegStreamsIt);
> +		if (nv12StreamsIt != streamMap.end()) {
> +			auto& nv12Streams = nv12StreamsIt->second;
> +			auto nv12StreamForJpegIt =
> +				std::find_if(nv12Streams.begin(),
> +					     nv12Streams.end(),
> +					     [&jpegStream](const auto* nv12Stream) {
> +						     return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;
> +					     });
> +			if (nv12StreamForJpegIt != nv12Streams.end()) {
> +				sortedStreams.push_back(*nv12StreamForJpegIt);
> +				nv12Streams.erase(nv12StreamForJpegIt);
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * Put configurations with different formats and larger resolutions
> +	 * earlier.
> +	 */
> +	while (!streamMap.empty()) {
> +		for (auto it = streamMap.begin(); it != streamMap.end();) {
> +			auto& streams = it->second;
> +			if (streams.empty()) {
> +				it = streamMap.erase(it);
> +				continue;
> +			}
> +			sortedStreams.push_back(streams.back());
> +			streams.pop_back();
> +		}
> +	}
> +
> +	assert(sortedStreams.size() == streamList->num_streams);
> +	memcpy(streamList->streams, sortedStreams.data(),
> +	       sizeof(camera3_stream_t*) * streamList->num_streams);
> +	return 0;
> +}
> +
>  /*
>   * Inspect the stream_list to produce a list of StreamConfiguration to
>   * be use to configure the Camera.
> @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	streams_.clear();
>  	streams_.reserve(stream_list->num_streams);
>
> +	printStreamList(*stream_list);
> +	sortStreamConfig(stream_list);
> +	printStreamList(*stream_list);
> +
>  	/* First handle all non-MJPEG streams. */
>  	camera3_stream_t *jpegStream = nullptr;
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index fd08738..f8dc28b 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -104,7 +104,8 @@ private:
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>  	CameraMetadata *requestTemplatePreview();
> -	libcamera::PixelFormat toPixelFormat(int format);
> +	libcamera::PixelFormat toPixelFormat(int format) const;
> +	int sortStreamConfig(camera3_stream_configuration_t* streamList) const;
>  	std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
>  							  int64_t timestamp);
>
> --
> 2.29.0.rc2.309.g374f81d7ae-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel


More information about the libcamera-devel mailing list