[libcamera-devel] [PATCH v2 10/20] libcamera: ipu3: Adjust and assign streams in validate()
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 10 14:31:50 CEST 2020
Hi Laurent,
On Fri, Jul 10, 2020 at 02:28:54PM +0300, Laurent Pinchart wrote:
> 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 ?
>
As a general question, I refrained from using yuv as this could very
well be an RGB stream.. ah no, ImgU can only produce NV12 :)
> > > > + 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);
I get what's happening, but in a few weeks I'll re-read that, then I
have to open the expandedTo() implementation and realize this expands
to the largest of the two sizes. Maybe I'll add a comment, but I found
it hard to remember as a pattern :)
>
> (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.
>
Was there already, and I'm always carefull in changing existing code.
Phone niklas for details.
> > > > - 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.
>
Actually this was intentional as here a 'different' section of code
begins. I could change it, as it is not a common pattern indeed.
> > > > +
> > > > + /* 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>
>
Thanks
j
> > > > + * 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