[libcamera-devel] [PATCH v2 10/20] libcamera: ipu3: Adjust and assign streams in validate()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 10 13:28:54 CEST 2020
Hi Jacopo,
Thank you for the patch.
On Fri, Jul 10, 2020 at 09:14:20AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 09, 2020 at 03:38:50PM +0200, Niklas Söderlund wrote:
> > 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
s/assume/assumes/
> > > 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;
yuvCount ?
> > > + 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.
> > > + */
Maybe maxYuvSize instead of yuvSize then ?
> > > + if (cfg.size > yuvSize)
> > > + yuvSize = cfg.size;
Note that the > operator on Size is implemented based on the < operator,
which is defined as
- A size with smaller width and smaller height is smaller.
- A size with smaller area is smaller.
- A size with smaller width is smaller.
I wonder if here you don't want the bounding rectangle instead, which
could be calculated with
yuvSize = yuvSize.expandedTo(cfg.size);
(Size::expandedTo() is a new function from the not-yet-merged
"libcamera: geometry: Add helper functions to the Size class" patch)
> > > + }
> > > +
> > > 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;
I'd name this requestedCfg or originalCfg.
> > > - const Stream *stream = streams_[i];
> > > + const PixelFormatInfo &info =
> > > + PixelFormatInfo::info(cfg.pixelFormat);
> > >
> > > - if (stream == &data_->rawStream_) {
> > > + LOG(IPU3, Debug) << "Validating configuration: " << cfg.toString();
Maybe s/configuration/stream/ ?
> > > +
> > > + /* 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.
> > > + */
This holds on a single line.
> > > +
> > > + /* 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
s/stream/streams/
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > + * 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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list