[libcamera-devel] [PATCH v2 10/20] libcamera: ipu3: Adjust and assign streams in validate()
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 10 09:14:20 CEST 2020
Hi Niklas,
On Thu, Jul 09, 2020 at 03:38:50PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work, I like this change.
>
> On 2020-07-09 10:41:18 +0200, Jacopo Mondi wrote:
> > Remove the adjustStream() and assignStream() methods, and perform stream
> > adjustment and assignment while iterating the StreamConfiguration
> > items.
> >
> > The adjustStream() implementation had some arbitrary assumption, like
> > the main output having to be as large as the sensor resolution, and did
> > not take into account the different alignment requirements between the
> > main output and the viewfinder output.
> >
> > The assignStream() implementation also assume only full-size streams
> > can be produced by the main output, and having it as a separate function
> > prevents adjusting streams according to which output they are assigned.
> >
> > Blend the two implementation in a single loop and perform the required
> > stream adjustment and assignment in one go.
> >
> > As streams are now assigned at validate() time, remove the same
> > operation from the configure() function.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------
> > 1 file changed, 91 insertions(+), 113 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 9128e42d42ed..18f4a02cc270 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -69,9 +69,6 @@ public:
> > const std::vector<const Stream *> &streams() { return streams_; }
> >
> > private:
> > - void assignStreams();
> > - void adjustStream(StreamConfiguration &cfg, bool scale);
> > -
> > /*
> > * The IPU3CameraData instance is guaranteed to be valid as long as the
> > * corresponding Camera instance is valid. In order to borrow a
> > @@ -136,96 +133,6 @@ 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 Stream *> availableStreams = {
> > - &data_->outStream_,
> > - &data_->vfStream_,
> > - &data_->rawStream_,
> > - };
> > -
> > - /*
> > - * The caller is responsible to limit the number of requested streams
> > - * to a number supported by the pipeline before calling this function.
> > - */
> > - ASSERT(availableStreams.size() >= config_.size());
> > -
> > - streams_.clear();
> > - streams_.reserve(config_.size());
> > -
> > - for (const StreamConfiguration &cfg : config_) {
> > - const PixelFormatInfo &info =
> > - PixelFormatInfo::info(cfg.pixelFormat);
> > - const Stream *stream;
> > -
> > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > - stream = &data_->rawStream_;
> > - else if (cfg.size == cio2Configuration_.size)
> > - stream = &data_->outStream_;
> > - else
> > - stream = &data_->vfStream_;
> > -
> > - if (availableStreams.find(stream) == availableStreams.end())
> > - stream = *availableStreams.begin();
> > -
> > - streams_.push_back(stream);
> > - availableStreams.erase(stream);
> > - }
> > -}
> > -
> > -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> > -{
> > - /* The only pixel format the driver supports is NV12. */
> > - cfg.pixelFormat = formats::NV12;
> > -
> > - if (scale) {
> > - /*
> > - * Provide a suitable default that matches the sensor aspect
> > - * ratio.
> > - */
> > - if (cfg.size.isNull()) {
> > - cfg.size.width = 1280;
> > - cfg.size.height = 1280 * cio2Configuration_.size.height
> > - / cio2Configuration_.size.width;
> > - }
> > -
> > - /*
> > - * \todo: Clamp the size to the hardware bounds when we will
> > - * figure them out.
> > - *
> > - * \todo: Handle the scaler (BDS) restrictions. The BDS can
> > - * only scale with the same factor in both directions, and the
> > - * scaling factor is limited to a multiple of 1/32. At the
> > - * moment the ImgU driver hides these constraints by applying
> > - * additional cropping, this should be fixed on the driver
> > - * side, and cropping should be exposed to us.
> > - */
> > - } else {
> > - /*
> > - * \todo: Properly support cropping when the ImgU driver
> > - * interface will be cleaned up.
> > - */
> > - cfg.size = cio2Configuration_.size;
> > - }
> > -
> > - /*
> > - * Clamp the size to match the ImgU alignment constraints. The width
> > - * shall be a multiple of 8 pixels and the height a multiple of 4
> > - * pixels.
> > - */
> > - if (cfg.size.width % 8 || cfg.size.height % 4) {
> > - cfg.size.width &= ~7;
> > - cfg.size.height &= ~3;
> > - }
> > -}
> > -
> > CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > {
> > Status status = Valid;
> > @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > */
> > unsigned int rawCount = 0;
> > unsigned int outCount = 0;
> > + Size yuvSize;
> > Size size;
> >
> > for (const StreamConfiguration &cfg : config_) {
> > const PixelFormatInfo &info =
> > PixelFormatInfo::info(cfg.pixelFormat);
> >
> > - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > rawCount++;
> > - else
> > + } else {
> > outCount++;
> >
> > + /*
> > + * Collect the maximum processed size to later assign
> > + * streams to configurations.
> > + */
> > + if (cfg.size > yuvSize)
> > + yuvSize = cfg.size;
> > + }
> > +
> > if (cfg.size.width > size.width)
> > size.width = cfg.size.width;
> > if (cfg.size.height > size.height)
> > @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > if (!cio2Configuration_.pixelFormat.isValid())
> > return Invalid;
> >
> > - /* Assign streams to each configuration entry. */
> > - assignStreams();
> > -
> > - /* Verify and adjust configuration if needed. */
> > + /*
> > + * Adjust the configurations if needed and assign streams while
> > + * iterating them.
> > + */
> > + bool mainOutputAvailable = true;
> > for (unsigned int i = 0; i < config_.size(); ++i) {
> > StreamConfiguration &cfg = config_[i];
> > const StreamConfiguration oldCfg = cfg;
> > - const Stream *stream = streams_[i];
> > + const PixelFormatInfo &info =
> > + PixelFormatInfo::info(cfg.pixelFormat);
> >
> > - if (stream == &data_->rawStream_) {
> > + LOG(IPU3, Debug) << "Validating configuration: " << cfg.toString();
> > +
> > + /* Initialize the RAW stream with the CIO2 configuration. */
> > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > cfg.size = cio2Configuration_.size;
> > cfg.pixelFormat = cio2Configuration_.pixelFormat;
> > cfg.bufferCount = cio2Configuration_.bufferCount;
> > + cfg.setStream(const_cast<Stream *>(&data_->rawStream_));
> > +
> > + LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> > + << " to the raw stream";
> > + continue;
> > + }
> > +
> > + /*
> > + * Assign and configure the main and viewfinder outputs.
> > + */
> > +
> > + /* Clamp the size to match the ImgU size limits. */
> > + cfg.size.width = utils::clamp(cfg.size.width,
> > + minIPU3OutputSize.width,
> > + IPU3_OUTPUT_MAX_WIDTH);
> > + cfg.size.height = utils::clamp(cfg.size.height,
> > + minIPU3OutputSize.height,
> > + IPU3_OUTPUT_MAX_HEIGHT);
> > +
> > + /*
> > + * All the ImgU output stream should be smaller than the ImgU
> > + * input frame, to give space to the IF and BDS to crop the
> > + * image.
> > + *
> > + * \todo Give the BDS rectangle at least 32 pixels from the
> > + * input frame size. This is an assumption deduced
> > + * from inspecting the pipe configuration tool output
> > + * and the platform configuration files shipped for the
> > + * Soraka device by ChromeOS.
> > + */
> > + if (cfg.size.width >= (cio2Configuration_.size.width - 31))
> > + cfg.size.width = cio2Configuration_.size.width - 32;
> > +
> > + if (cfg.size.height >= (cio2Configuration_.size.height - 31))
> > + cfg.size.height = cio2Configuration_.size.height - 32;
>
> Could you not clamp cfg.size.width within minIPU3OutputSize.width and
> cio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width
> and IPU3_OUTPUT_MAX_WIDTH?
I probably should! I had this two steps procedure, but there's not
reason to keep it
>
> Same for hight of course.
>
> > +
> > + /*
> > + * Adjust to match the main output or the viewfinder
> > + * output constraints and assign streams.
> > + *
> > + * Use the main output stream in case only one stream is
> > + * requested or if the current configuration is the one with
> > + * the maximum RGB/YUV output size.
> > + */
>
> This comment is 1 (see below).
>
> > + cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> > + cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> > + cfg.bufferCount = IPU3_BUFFER_COUNT;
> > + cfg.pixelFormat = formats::NV12;
> > +
> > + /*
> > + * Use a const_cast<> here instead of storing a mutable stream
> > + * pointer in the configuration to let the compiler catch
> > + * unwanted modifications of camera data in the configuration
> > + * validate() implementation.
> > + */
>
> I would drop this comment and move comment [1] here. There is no need to
> document const_cast<>() for the setStream() call for the RAW stream what
> is different here?
Just copied from configure(). Later in the series a commit will remove
it explictely instead of slipping its removal in with this change.
>
> > + Stream *stream;
> > + if (mainOutputAvailable &&
> > + (oldCfg.size == yuvSize || outCount == 1)) {
> > + stream = const_cast<Stream *>(&data_->outStream_);
> > + mainOutputAvailable = false;
> > +
> > + LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> > + << " to the main output";
> > } else {
> > - bool scale = stream == &data_->vfStream_;
> > - adjustStream(config_[i], scale);
> > - cfg.bufferCount = IPU3_BUFFER_COUNT;
> > + stream = const_cast<Stream *>(&data_->vfStream_);
> > +
> > + LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> > + << " to the viewfinder output";
> > }
> > + cfg.setStream(stream);
> >
> > if (cfg.pixelFormat != oldCfg.pixelFormat ||
> > cfg.size != oldCfg.size) {
> > @@ -472,16 +458,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > bool vfActive = false;
> >
> > for (unsigned int i = 0; i < config->size(); ++i) {
> > - /*
> > - * Use a const_cast<> here instead of storing a mutable stream
> > - * pointer in the configuration to let the compiler catch
> > - * unwanted modifications of camera data in the configuration
> > - * validate() implementation.
> > - */
> > - Stream *stream = const_cast<Stream *>(config->streams()[i]);
> > StreamConfiguration &cfg = (*config)[i];
> > -
> > - cfg.setStream(stream);
> > + Stream *stream = cfg.stream();
> >
> > if (stream == outStream) {
> > ret = imgu->configureOutput(cfg, &outputFormat);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list