| Message ID | 20200701123036.51922-7-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-07-01 14:30:27 +0200, Jacopo Mondi wrote: > With the goal of moving stream assignement to configuration from > validate() to configure(), rework the validate() function to not > inspect the stream assigned to a configuration to identify it. > > Re-work the validation logic to follow this simpler flow: > - if a stream is a raw stream use the sensor configuration > - if a stream is a processed one, make sure it respects the ImgU > output size and alignment constraints. > > Remove the 'adjustStream()' function as it depends on stream assignment > and was built on the assumption the main output should always work at > the maximum available resolution, and it addressed the case where no > width or height where supplied for a viewfinder stream, which should > only be validated against the ImgU output constraints, not defaulted to a > sane value. > > Retain the call to assignStreams() in validate() for the moment in order > not to break capture operations, but while at it clean up the code a bit > by removing a rogue empty line and make a conditional check fit on a > single line. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 85 ++++++++++------------------ > 1 file changed, 31 insertions(+), 54 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index ed2360347fb4..daa6d71dae72 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -68,7 +68,6 @@ public: > > private: > void assignStreams(); > - void adjustStream(StreamConfiguration &cfg, bool scale); > > /* > * The IPU3CameraData instance is guaranteed to be valid as long as the > @@ -178,52 +177,6 @@ void IPU3CameraConfiguration::assignStreams() > } > } > > -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.width || !cfg.size.height) { > - 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; > @@ -244,7 +197,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > * resolution is large enough, pick the largest one. > */ > Size size = {}; > - > for (const StreamConfiguration &cfg : config_) { > if (cfg.size.width > size.width) > size.width = cfg.size.width; > @@ -264,20 +216,45 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > 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_) { > + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > cfg.size = cio2Configuration_.size; > cfg.pixelFormat = cio2Configuration_.pixelFormat; > cfg.bufferCount = cio2Configuration_.bufferCount; > } else { > - bool scale = stream == &data_->vfStream_; > - adjustStream(config_[i], scale); > + /* > + * Clamp the size to match the ImgU alignment > + * constraints. > + */ > + size.width = std::min(cfg.size.width, > + IPU3_OUTPUT_MAX_WIDTH); > + size.height = std::min(cfg.size.height, > + IPU3_OUTPUT_MAX_HEIGHT); > + size.width = std::max(cfg.size.width, > + minIPU3OutputSize.width); > + size.height = std::max(cfg.size.height, > + minIPU3OutputSize.height); > + if (cfg.size.width % 8 || cfg.size.height % 4) { > + cfg.size.width &= ~7; > + cfg.size.height &= ~3; > + } > + cfg.pixelFormat = formats::NV12; > cfg.bufferCount = IPU3_BUFFER_COUNT; > + > + /* > + * \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. > + */ > } > > - if (cfg.pixelFormat != oldCfg.pixelFormat || > - cfg.size != oldCfg.size) { > + if (cfg.pixelFormat != oldCfg.pixelFormat || cfg.size != oldCfg.size) { > LOG(IPU3, Debug) > << "Stream " << i << " configuration adjusted to " > << cfg.toString(); > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index ed2360347fb4..daa6d71dae72 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -68,7 +68,6 @@ public: private: void assignStreams(); - void adjustStream(StreamConfiguration &cfg, bool scale); /* * The IPU3CameraData instance is guaranteed to be valid as long as the @@ -178,52 +177,6 @@ void IPU3CameraConfiguration::assignStreams() } } -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.width || !cfg.size.height) { - 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; @@ -244,7 +197,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() * resolution is large enough, pick the largest one. */ Size size = {}; - for (const StreamConfiguration &cfg : config_) { if (cfg.size.width > size.width) size.width = cfg.size.width; @@ -264,20 +216,45 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() 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_) { + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { cfg.size = cio2Configuration_.size; cfg.pixelFormat = cio2Configuration_.pixelFormat; cfg.bufferCount = cio2Configuration_.bufferCount; } else { - bool scale = stream == &data_->vfStream_; - adjustStream(config_[i], scale); + /* + * Clamp the size to match the ImgU alignment + * constraints. + */ + size.width = std::min(cfg.size.width, + IPU3_OUTPUT_MAX_WIDTH); + size.height = std::min(cfg.size.height, + IPU3_OUTPUT_MAX_HEIGHT); + size.width = std::max(cfg.size.width, + minIPU3OutputSize.width); + size.height = std::max(cfg.size.height, + minIPU3OutputSize.height); + if (cfg.size.width % 8 || cfg.size.height % 4) { + cfg.size.width &= ~7; + cfg.size.height &= ~3; + } + cfg.pixelFormat = formats::NV12; cfg.bufferCount = IPU3_BUFFER_COUNT; + + /* + * \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. + */ } - if (cfg.pixelFormat != oldCfg.pixelFormat || - cfg.size != oldCfg.size) { + if (cfg.pixelFormat != oldCfg.pixelFormat || cfg.size != oldCfg.size) { LOG(IPU3, Debug) << "Stream " << i << " configuration adjusted to " << cfg.toString();
With the goal of moving stream assignement to configuration from validate() to configure(), rework the validate() function to not inspect the stream assigned to a configuration to identify it. Re-work the validation logic to follow this simpler flow: - if a stream is a raw stream use the sensor configuration - if a stream is a processed one, make sure it respects the ImgU output size and alignment constraints. Remove the 'adjustStream()' function as it depends on stream assignment and was built on the assumption the main output should always work at the maximum available resolution, and it addressed the case where no width or height where supplied for a viewfinder stream, which should only be validated against the ImgU output constraints, not defaulted to a sane value. Retain the call to assignStreams() in validate() for the moment in order not to break capture operations, but while at it clean up the code a bit by removing a rogue empty line and make a conditional check fit on a single line. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 85 ++++++++++------------------ 1 file changed, 31 insertions(+), 54 deletions(-)