[libcamera-devel] [RFC PATCH v2] android: camera_device: Reorder configurations before request
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 7 11:02:32 CET 2020
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 at 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 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 ?
> >
>
> 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
More information about the libcamera-devel
mailing list