[libcamera-devel,RFC,v2] android: camera_device: Reorder configurations before request

10524 diff mbox series

This reorders configurations before calling
CameraConfiguration::validate() so that the streams requested
by an android HAL client can be achieved more likely.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++-----
 1 file changed, 137 insertions(+), 21 deletions(-)

--
2.29.2.454.gaff20da3a2-goog

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@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

Hi Jacopo,

On Thu, Dec 3, 2020 at 2:16 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> 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_
>

Sure, I splitted.

> >
> > Signed-off-by: Hirokazu Honda <hiroh@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 ?
>

The while-loop is needed to assort formats.
In your way, the configurations with the same format are put earlier,
e.g., nv12, nv12, yv12, yv12,.. rather than nv12, yv12, nv12, yv12...

Best Regards,
-Hiro



> > +                     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

Hi Hiro,

On Mon, Dec 07, 2020 at 05:47:27PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Thu, Dec 3, 2020 at 2:16 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > 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_
> >
>
> Sure, I splitted.
>

Thanks I'll review soon

> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@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 ?
> >
>
> The while-loop is needed to assort formats.
> In your way, the configurations with the same format are put earlier,
> e.g., nv12, nv12, yv12, yv12,.. rather than nv12, yv12, nv12, yv12...
>

I'm sorry, I don't want to be insistent and I'm surely reading some
piece wrong, but if I iterate your implementation on the following
example map (afaict the vector of configs is sorted by size, smaller
goes first))

map = { {NV12: {VGA, XGA},}, {YV12: {QVGA, VGA}} }

The pseudo-code of the implementation I read looks like:

        while (!map.empty()) {
                for (it = map.begin(); it != map.end()) {
                        vector &v = it.second;

                        if (v.empty())
                                it = map.erase(it);
                                continue;

                        sorted.push_back(v.back());
                        v.pop_back();
                }
        }

And if I manually iterate it on the map I get:

map = { {NV12: {VGA, XGA},}, {YV12: {QVGA, VGA}} }
0:
        it = { NV12, {VGA, XGA}}
        v = { VGA, XGA }
        sorted = { [NV12,XGA] }
        v = { VGA }

1:
        it = { NV12, { VGA }}
        v = { VGA }
        sorted = { [NV12,XGA], [NV12, VGA] }
        v = {}

2:
        it = {NV12, {}}
        v = {} {
                map = { {YV12: {QVGA, VGA}} }
                it = {YV12: {QVGA, VGA}}
        }

3:
        it = {YV12: {QVGA, VGA}}
        v = { QVGA, VGA }
        sorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA] }
        v = { QVGA }

4:
        it = {YV12: {QVGA}}
        v = { QVGA }
        sorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA], [YV12, VGA]}
        v = { }

5:
        it = {Y12: {}}
        v = {} {
                map = {}
                it = map.end();
        }


sorted = { [NV12,XGA], [NV12, VGA], [YV12, VGA], [YV12, VGA]}

Which seems to suggest the same result as the two nested for loops
I've proposed. What am I missing ?

Thanks
  j

> Best Regards,
> -Hiro
>
>
>
> > > +                     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