[libcamera-devel] [PATCH v2 04/10] libcamera: ipu3: Breakout stream assignment to new function
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 23:18:39 CEST 2020
Hi Niklas,
Thank you for the patch.
On Sat, Jun 06, 2020 at 05:04:30PM +0200, Niklas Söderlund wrote:
> Selecting which stream is the most suitable for the requested
> configuration is mixed with adjusting the requested format when
> validating configurations. This is hard to read and got worse when
> support for Bayer formats was added, break it out to a separate
s/, break/. Break/
> function.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Update commit message.
> - Rename updateStreams() to assignStreams().
> - Revert and keep old comment on how streams are picked.
> - Do not modify behavior on how streams are picked which means
> assignStreams() now can't fail so no need for it to return an int,
> switch to void.
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 79 ++++++++++++++++------------
> 1 file changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f7363244e1d2d0ff..6df93eedb365b904 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -190,6 +190,7 @@ private:
> static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> static constexpr unsigned int IPU3_MAX_STREAMS = 3;
>
> + void assignStreams();
> void adjustStream(StreamConfiguration &cfg, bool scale);
>
> /*
> @@ -256,6 +257,42 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> +void IPU3CameraConfiguration::assignStreams()
> +{
> + /*
> + * Verify and update all configuration entries, and assign a stream to
> + * each of them. The viewfinder stream can scale, while the output
> + * stream can crop only, so select the output stream when the requested
> + * resolution is equal to the sensor resolution, and the viewfinder
> + * stream otherwise.
> + */
> + std::set<const IPU3Stream *> availableStreams = {
> + &data_->outStream_,
> + &data_->vfStream_,
> + &data_->rawStream_,
> + };
> +
> + streams_.clear();
> + streams_.reserve(config_.size());
> +
> + for (const StreamConfiguration &cfg : config_) {
> + const IPU3Stream *stream;
> +
> + if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> + stream = &data_->rawStream_;
> + else if (cfg.size == sensorFormat_.size)
> + stream = &data_->outStream_;
> + else
> + stream = &data_->vfStream_;
> +
> + if (availableStreams.find(stream) == availableStreams.end())
> + stream = *availableStreams.begin();
> +
> + streams_.push_back(stream);
> + availableStreams.erase(stream);
I was going to comment that you should check that availableStreams
doesn't become empty, but then realized the caller already clamps the
number of streams. Maybe a comment above this function should capture
the pre-conditions ?
I think we'll have to extend the logic here to cover the case where the
user requests three streams and none of them has a raw format. The above
code will select the raw stream as the third stream. Wouldn't it make
more sense in that case to only return two streams ? This can be
discussed and addressed separately from this patch, and in general I
think we need to document the heuristics that pipeline handlers shall
implement to validate the camera configuration.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + }
> +}
> +
> void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> {
> /* The only pixel format the driver supports is NV12. */
> @@ -342,40 +379,14 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> sensorFormat_.size = sensor->resolution();
>
> - /*
> - * Verify and update all configuration entries, and assign a stream to
> - * each of them. The viewfinder stream can scale, while the output
> - * stream can crop only, so select the output stream when the requested
> - * resolution is equal to the sensor resolution, and the viewfinder
> - * stream otherwise.
> - */
> - std::set<const IPU3Stream *> availableStreams = {
> - &data_->outStream_,
> - &data_->vfStream_,
> - &data_->rawStream_,
> - };
> -
> - streams_.clear();
> - streams_.reserve(config_.size());
> + /* Assign streams to each configuration entry. */
> + assignStreams();
>
> + /* Verify and adjust configuration if needed. */
> for (unsigned int i = 0; i < config_.size(); ++i) {
> StreamConfiguration &cfg = config_[i];
> - const PixelFormat pixelFormat = cfg.pixelFormat;
> - const Size size = cfg.size;
> - const IPU3Stream *stream;
> -
> - if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> - stream = &data_->rawStream_;
> - else if (cfg.size == sensorFormat_.size)
> - stream = &data_->outStream_;
> - else
> - stream = &data_->vfStream_;
> -
> - if (availableStreams.find(stream) == availableStreams.end())
> - stream = *availableStreams.begin();
> -
> - LOG(IPU3, Debug)
> - << "Assigned '" << stream->name_ << "' to stream " << i;
> + const StreamConfiguration oldCfg = cfg;
> + const IPU3Stream *stream = streams_[i];
>
> if (stream->raw_) {
> const auto &itFormat =
> @@ -392,15 +403,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>
> cfg.bufferCount = IPU3_BUFFER_COUNT;
>
> - if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> + if (cfg.pixelFormat != oldCfg.pixelFormat ||
> + cfg.size != oldCfg.size) {
> LOG(IPU3, Debug)
> << "Stream " << i << " configuration adjusted to "
> << cfg.toString();
> status = Adjusted;
> }
> -
> - streams_.push_back(stream);
> - availableStreams.erase(stream);
> }
>
> return status;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list