[libcamera-devel] [PATCH v3 2/3] android: camera_device: Use Camera3StreamConfig in configureStreams()

[Hirokazu Honda]

On Mon, Dec 7, 2020 at 7:24 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Dec 07, 2020 at 08:42:17AM +0000, Hirokazu Honda wrote:
> > This uses Camera3StreamConfig in CameraDevice::configureStreams().
>
> I would be slightly more verbose.
>
> "Use the newly introduced Camera3StreamConfig to associate the
> Android requested streams with the associated StreamConfiguration
> in a vector of configurations.
>
> This change prepares to sort the vector of configuration before using
> it to configure the Camera and populate the streams_ vector.
>
> No functional changes intended."
>
> And the signed-off-by of course.
> > ---
> >  src/android/camera_device.cpp | 52 +++++++++++++++++++++--------------
> >  1 file changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 09269d95..b7bf3d88 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1240,6 +1240,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >       streams_.clear();
> >       streams_.reserve(stream_list->num_streams);
> >
> > +     std::vector<Camera3StreamConfig> streamConfigs;
> > +     streamConfigs.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) {
> > @@ -1270,14 +1273,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());
> > +             Camera3StreamConfig streamConfig;
> > +             streamConfig.streams = { stream };
> > +             streamConfig.types = { CameraStream::Type::Direct };
> > +             streamConfig.config.size = size;
> > +             streamConfig.config.pixelFormat = format;
> > +             streamConfigs.push_back(std::move(streamConfig));
>
> Nit: does this move help in any way with memory handling ? I mean,
> I do expect the auto-generated move constructor of Camera3StreamConfig
> not being able to magically move bits between the local variable and
> the vector storage ? Am I wrong or what move() actually does is to
> invalidate the local variable but copies data anyway in this case ?
>
> We're talking about a tiny number of operations not in an hot-path, so
> this i very minor, but I wonder if emplace() wouldn't do better (or
> just a push_back() it it's equivalent to move()).
>

Since std::move(streamConfig) move()s std::vectors (i.e. streams and
types) to the new instance, it should be efficient than without
std::move.
emplace_back(std::move(streamConfig)) is equivalent to
push_back(std::move(streamConfig)).
cf.) https://stackoverflow.com/a/26861516

Regards,
-Hiro


> >       }
> >
> >       /* Now handle the MJPEG streams, adding a new stream if required. */
> > @@ -1286,9 +1287,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 < streamConfigs.size(); ++i) {
> > +                     const auto &cfg = streamConfigs[i].config;
> >                       /*
> >                        * \todo The PixelFormat must also be compatible with
> >                        * the encoder.
> > @@ -1310,28 +1310,38 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >                * introduce a new stream to satisfy the request requirements.
> >                */
> >               if (index < 0) {
> > -                     StreamConfiguration streamConfiguration;
> > -
> >                       /*
> >                        * \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;
> > +                     Camera3StreamConfig streamConfig;
> > +                     streamConfig.config.size.width = jpegStream->width;
> > +                     streamConfig.config.size.height = jpegStream->height;
> > +                     streamConfig.config.pixelFormat = formats::NV12;
> > +                     streamConfigs.push_back(std::move(streamConfig));
> >
> > -                     LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> > +                     LOG(HAL, Info) << "Adding " << streamConfig.config.toString()
> >                                      << " for MJPEG support";
> >
> >                       type = CameraStream::Type::Internal;
> > -                     config_->addConfiguration(streamConfiguration);
> > -                     index = config_->size() - 1;
> > +                     index = streamConfigs.size() - 1;
> >               }
> >
> > -             streams_.emplace_back(this, type, jpegStream, index);
> > -             jpegStream->priv = static_cast<void *>(&streams_.back());
> > +             streamConfigs[index].streams.push_back(jpegStream);
> > +             streamConfigs[index].types.push_back(type);
> > +     }
> > +
> > +     for (const auto &streamConfig : streamConfigs) {
> > +             config_->addConfiguration(streamConfig.config);
> > +             for (size_t i = 0; i < streamConfig.streams.size(); ++i) {
> > +                     camera3_stream_t *stream = streamConfig.streams[i];
> > +                     const CameraStream::Type type = streamConfig.types[i];
> > +                     streams_.emplace_back(this, type,
> > +                                           stream, config_->size() - 1);
> > +                     stream->priv = static_cast<void*>(&streams_.back());
> > +             }
>
> I like where this is going!
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
>   j
>
> >       }
> >
> >       switch (config_->validate()) {
> > --
> > 2.29.2.576.ga3fc446d84-goog