| Message ID | 20200709084128.5316-11-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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? 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? > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@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@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);
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@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@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
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; + + /* + * 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. + */ + 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. + */ + 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);
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@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++--------------- 1 file changed, 91 insertions(+), 113 deletions(-)