[libcamera-devel] [RFC PATCH v2] android: camera_device: Reorder configurations before request
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 2 18:16:58 CET 2020
Hello Hiro,
On Tue, Dec 01, 2020 at 01:25:14PM +0900, Hirokazu Honda wrote:
> This reorders configurations before calling
> CameraConfiguration::validate() so that the streams requested
> by an android HAL client can be achieved more likely.
This patch introduces a new intermediate type, which associate a
camera3_stream_t * with a libcamera::StreamConfiguration. I would
record it in the commit message, something like:
Re-order StreamConfiguration in the CameraConfiguration before
validating it to make it easier for the Camera to satisfy the
Android framework request.
Introduce a new Camera3StreamsToConfig type to associate
camera3_stream with StreamConfiguration and sort them before
using them to populate the CameraDevice::streams_ vector.
To be honest I would split this patch to make its consumption easier:
1) Introduce the new type
2) Collect the vector and create streams_ from it (this should be
functionally equivalent to the existing 'unsorted' implementation
afaict)
3) Sort the vector before creating streams_
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++-----
> 1 file changed, 137 insertions(+), 21 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4690346e..3de5a9f1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -9,9 +9,11 @@
> #include "camera_ops.h"
> #include "post_processor.h"
>
> +#include <string.h>
> #include <sys/mman.h>
> #include <tuple>
> #include <vector>
> +#include <optional>
'o' comes before 's'
Actually utils/checkstyle.py reports several style issues (some false
positives as well). I'll point out what I can spot but please re-check
with it.
>
> #include <libcamera/control_ids.h>
> #include <libcamera/controls.h>
> @@ -128,6 +130,107 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
> },
> };
>
> +struct Camera3StreamsToConfig {
Can you shorten names a bit ?
I was about to suggest StreamConfig, but we have StreamConfiguration
already. Camera3Stream ? But we have CameraStream... Ok, keep the long
name :) Maybe Camera3StreamConfig ?
> + std::vector<camera3_stream_t*> streams; // Destination(s).
We usually use: "typename *" and "typename &"
> + std::vector<CameraStream::Type> types; // CameraStream::Type(s).
> + StreamConfiguration config; // StreamConfiguration requested to a native camera.
No C++ comments please.
> +};
> +
> +/*
> + * Reorder the configurations so that CameraDevice can accept them as much as
> + * possible.
> + */
Nice! Can you provide a brief description of the sorting criteria as
well ?
> +std::vector<Camera3StreamsToConfig> createdSortedCamera3StreamsToConfigs(
maybe just 'sortStreamConfigs' ?
Can you move this function before its only user ?
> + std::vector<Camera3StreamsToConfig> unsortedStreamsToConfigs,
const & ?
Ah I see you move the vector here...
> + const camera3_stream_t *jpegStream) {
> + const size_t unsortedStreamsToConfigsSize = unsortedStreamsToConfigs.size();
> + std::optional<Camera3StreamsToConfig> streamsToConfigForJpeg = std::nullopt;
> + if (jpegStream) {
> + for (auto it = unsortedStreamsToConfigs.begin();
> + it != unsortedStreamsToConfigs.end(); it++) {
for (const auto &it : unsortedStreamsToConfigs) ?
> + const auto& streams = it->streams;
> + if (std::find(streams.begin(), streams.end(),
> + jpegStream) != streams.end()) {
> + streamsToConfigForJpeg = *it;
> + unsortedStreamsToConfigs.erase(it);
> + break;
> + }
> + }
> + assert(streamsToConfigForJpeg.has_value());
LOG(Fatal) would make the error verbose
> + }
> +
> + std::map<uint32_t, std::vector<Camera3StreamsToConfig>> formatToStreamsToConfigs;
It's internal stuff, just 'formatToConfig' or even 'formats' would
make this more readable.
> + for (const auto &streamsToConfig : unsortedStreamsToConfigs) {
> + const StreamConfiguration &config = streamsToConfig.config;
> + formatToStreamsToConfigs[config.pixelFormat].push_back(streamsToConfig);
> +
> + }
> + for (auto& [format, streamsToConfigs] : formatToStreamsToConfigs) {
auto &
the [] pair syntax is much nicer than having to use it.first, it.second!
> + /* Sorted by resolution. Smaller is put first. */
Size has an ordering defined, have you considered using it ?
> + std::sort(streamsToConfigs.begin(), streamsToConfigs.end(),
> + [](const auto &streamsToConfigA, const auto &streamsToConfigB) {
> + const Size &sizeA = streamsToConfigA.config.size;
> + const Size &sizeB = streamsToConfigA.config.size;
> + if (sizeA.width != sizeB.width)
> + return sizeA.width < sizeB.width;
> + return sizeA.height < sizeB.height;
> + });
> + }
> +
> + std::vector<Camera3StreamsToConfig> sortedStreamsToConfigs;
Maybe reserve space in all intermediate vectors to avoid relocations ?
Although numbers should be small..
> + /*
> + * NV12 is the most prioritized format. Put the configuration with NV12
> + * and the largest resolution first.
> + */
> + if (formatToStreamsToConfigs.find(formats::NV12) != formatToStreamsToConfigs.end()) {
> + auto& nv12StreamsToConfigs = formatToStreamsToConfigs[formats::NV12];
auto &nv12StreamsToConfigs
Can the double lookup be avoided with:
const auto nv12Stream = formatToStreamsToConfigs.find(formats::NV12)
if (nv12Stream != formatToStreamsToConfigs.end())
> + const Size& nv12LargestSize = nv12StreamsToConfigs.back().config.size;
const Size &nv12LargestSize
> + if (streamsToConfigForJpeg &&
> + streamsToConfigForJpeg->config.pixelFormat == formats::NV12) {
> + const Size& nv12SizeForJpeg = streamsToConfigForJpeg->config.size;
Can you move it after the comment block ?
> + /*
> + * If JPEG will be created from NV12 and the size is
> + * larger than the largest NV12 remained configurations,
Didn't get 'remained'
'configuration'
> + * then put the NV12 configuration for JPEG first.
^ rogue space
> + */
> + if (nv12LargestSize.width < nv12SizeForJpeg.width &&
> + nv12LargestSize.height < nv12SizeForJpeg.height) {
> + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);
> + streamsToConfigForJpeg = std::nullopt;
> + }
> + }
> + sortedStreamsToConfigs.push_back(nv12StreamsToConfigs.back());
> + nv12StreamsToConfigs.pop_back();
> + }
> +
> + /*
> + * If the configuration for JPEG is there, then put it.
> + */
Fits on one line
> + if (streamsToConfigForJpeg) {
> + sortedStreamsToConfigs.push_back(*streamsToConfigForJpeg);
> + streamsToConfigForJpeg = std::nullopt;
> + }
> +
> + /*
> + * Put configurations with different formats and larger resolutions
> + * earlier.
> + */
> + while (!formatToStreamsToConfigs.empty()) {
> + for (auto it = formatToStreamsToConfigs.begin();
> + it != formatToStreamsToConfigs.end();) {
> + auto& streamsToConfigs = it->second;
auto &streamsToConfigs
> + if (streamsToConfigs.empty()) {
> + it = formatToStreamsToConfigs.erase(it);
> + continue;
> + }
Can't this loop be expressed as:
for (const auto &format : formatToStreamsToConfigs) {
for (const auto stream = format.second.rbegin();
stream != format.second.rend(); ++stream)
sortedStreamsToConfigs.push_back(stream);
}
Feels more readable to me (not compiled, just an idea)
Or do you need to erase elements while you loop them ?
> + sortedStreamsToConfigs.push_back(streamsToConfigs.back());
> + streamsToConfigs.pop_back();
> + }
> + }
> + assert(sortedStreamsToConfigs.size() == unsortedStreamsToConfigsSize);
One empty line before return ?
> + return sortedStreamsToConfigs;
> +}
I think isolating the sorting algorithm in one patch would make it
easier to evaluate it in isolation along with the validate()
modification you are working on!
> +
> } /* namespace */
>
> LOG_DECLARE_CATEGORY(HAL)
> @@ -1225,6 +1328,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> streams_.clear();
> streams_.reserve(stream_list->num_streams);
>
> + std::vector<Camera3StreamsToConfig> streamsToConfigs;
> + streamsToConfigs.reserve(stream_list->num_streams);
> +
> /* First handle all non-MJPEG streams. */
> camera3_stream_t *jpegStream = nullptr;
> for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> @@ -1255,14 +1361,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> continue;
> }
>
> - StreamConfiguration streamConfiguration;
> - streamConfiguration.size = size;
> - streamConfiguration.pixelFormat = format;
> -
> - config_->addConfiguration(streamConfiguration);
> - streams_.emplace_back(this, CameraStream::Type::Direct,
> - stream, config_->size() - 1);
> - stream->priv = static_cast<void *>(&streams_.back());
> + Camera3StreamsToConfig streamsToConfig;
> + streamsToConfig.streams = {stream};
{ stream };
> + streamsToConfig.types = {CameraStream::Type::Direct};
{ CameraStream::Type::Direct };
> + streamsToConfig.config.size = size;
> + streamsToConfig.config.pixelFormat = format;
> + streamsToConfigs.push_back(std::move(streamsToConfig));
> }
>
> /* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1271,9 +1375,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> int index = -1;
>
> /* Search for a compatible stream in the non-JPEG ones. */
> - for (unsigned int i = 0; i < config_->size(); i++) {
> - StreamConfiguration &cfg = config_->at(i);
> -
> + for (size_t i = 0; i < streamsToConfigs.size(); ++i) {
> + const auto& cfg = streamsToConfigs[i].config;
const auto &cfg
> /*
> * \todo The PixelFormat must also be compatible with
> * the encoder.
> @@ -1295,28 +1398,41 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> * introduce a new stream to satisfy the request requirements.
> */
> if (index < 0) {
> - StreamConfiguration streamConfiguration;
> -
> + Camera3StreamsToConfig streamsToConfig;
I would keep the empty line or move it below the comment block.
> /*
> * \todo The pixelFormat should be a 'best-fit' choice
> * and may require a validation cycle. This is not yet
> * handled, and should be considered as part of any
> * stream configuration reworks.
> */
> - streamConfiguration.size.width = jpegStream->width;
> - streamConfiguration.size.height = jpegStream->height;
> - streamConfiguration.pixelFormat = formats::NV12;
> + streamsToConfig.config.size.width = jpegStream->width;
> + streamsToConfig.config.size.height = jpegStream->height;
> + streamsToConfig.config.pixelFormat = formats::NV12;
> + streamsToConfigs.push_back(std::move(streamsToConfig));
>
> - LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> + LOG(HAL, Info) << "Adding " << streamsToConfig.config.toString()
> << " for MJPEG support";
>
> type = CameraStream::Type::Internal;
> - config_->addConfiguration(streamConfiguration);
> - index = config_->size() - 1;
> + index = streamsToConfigs.size() - 1;
> }
>
> - streams_.emplace_back(this, type, jpegStream, index);
> - jpegStream->priv = static_cast<void *>(&streams_.back());
> + streamsToConfigs[index].streams.push_back(jpegStream);
> + streamsToConfigs[index].types.push_back(type);
> + }
> +
> + streamsToConfigs =
> + createdSortedCamera3StreamsToConfigs(std::move(streamsToConfigs),
> + jpegStream);
I'm sure we can find a shorter name for this function :)
> + for (auto& streamsToConfig : streamsToConfigs) {
const auto &streamsToConfig
> + config_->addConfiguration(streamsToConfig.config);
> + for (size_t i = 0; i < streamsToConfig.streams.size(); ++i) {
> + auto* stream = streamsToConfig.streams[i];
auto *stream
When the type is 'trivial' please spell it in full
> + const CameraStream::Type type = streamsToConfig.types[i];
> + streams_.emplace_back(this, type,
> + stream, config_->size() - 1);
> + stream->priv = static_cast<void*>(&streams_.back());
> + }
Style and a few minor issues apart, I think this is mostly ok. I tried
to think how CameraStream can be modified not to introduce a new type,
but what we're actually sorting are the StreamConfiguration, so a new
type is probably the right thing to do.
Thanks
j
> }
>
> switch (config_->validate()) {
> --
> 2.29.2.454.gaff20da3a2-goog
More information about the libcamera-devel
mailing list