[libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove streams from generateConfiguration
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jul 9 14:59:42 CEST 2020
Hi Jacopo,
Thanks for your work.
On 2020-07-09 10:41:10 +0200, Jacopo Mondi wrote:
> Remove stream assignment from the IPU3 pipeline handler
> generateConfiguration() implementation.
>
> The function aims to provide a suitable default for the requested use
> cases. Defer stream assignment to validation and only initialize sizes
> and formats.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 104 ++++++++-------------------
> 1 file changed, 30 insertions(+), 74 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index af51fb2d1718..85d21b4db046 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -31,6 +31,14 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPU3)
>
> +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> +static constexpr unsigned int IPU3_OUTPUT_MAX_WIDTH = 4480;
> +static constexpr unsigned int IPU3_OUTPUT_MAX_HEIGHT = 34004;
Just checking is 34004 the max height?
> +static const Size minIPU3OutputSize = { 2, 2 };
> +static constexpr unsigned int IPU3_OUTPUT_WIDTH_ALIGN = 0x3f;
> +static constexpr unsigned int IPU3_OUTPUT_HEIGHT_ALIGN = 0x3;
> +
> class IPU3CameraData : public CameraData
> {
> public:
> @@ -61,9 +69,6 @@ public:
> const std::vector<const Stream *> &streams() { return streams_; }
>
> private:
> - static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> - static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> -
> void assignStreams();
> void adjustStream(StreamConfiguration &cfg, bool scale);
>
> @@ -293,94 +298,51 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> {
> IPU3CameraData *data = cameraData(camera);
> IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
> - std::set<Stream *> streams = {
> - &data->outStream_,
> - &data->vfStream_,
> - &data->rawStream_,
> - };
>
> if (roles.empty())
> return config;
>
> + Size sensorResolution = data->cio2_.sensor()->resolution();
> for (const StreamRole role : roles) {
> StreamConfiguration cfg = {};
> - Stream *stream = nullptr;
> -
> - cfg.pixelFormat = formats::NV12;
>
> switch (role) {
> case StreamRole::StillCapture:
> /*
> - * Pick the output stream by default as the Viewfinder
> - * and VideoRecording roles are not allowed on
> - * the output stream.
> - */
> - if (streams.find(&data->outStream_) != streams.end()) {
> - stream = &data->outStream_;
> - } else if (streams.find(&data->vfStream_) != streams.end()) {
> - stream = &data->vfStream_;
> - } else {
> - LOG(IPU3, Error)
> - << "No stream available for requested role "
> - << role;
> - break;
> - }
> -
> - /*
> - * FIXME: Soraka: the maximum resolution reported by
> - * both sensors (2592x1944 for ov5670 and 4224x3136 for
> - * ov13858) are returned as default configurations but
> - * they're not correctly processed by the ImgU.
> - * Resolutions up tp 2560x1920 have been validated.
> - *
> - * \todo Clarify ImgU alignment requirements.
> + * Use the sensor resolution aligned to the ImgU
> + * output constraints.
> */
> - cfg.size = { 2560, 1920 };
> + cfg.size.width = std::min(sensorResolution.width,
> + IPU3_OUTPUT_MAX_WIDTH);
> + cfg.size.height = std::min(sensorResolution.height,
> + IPU3_OUTPUT_MAX_HEIGHT);
> + cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> + cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> + cfg.pixelFormat = formats::NV12;
> + cfg.bufferCount = IPU3_BUFFER_COUNT;
>
> break;
>
> case StreamRole::StillCaptureRaw: {
> - if (streams.find(&data->rawStream_) == streams.end()) {
> - LOG(IPU3, Error)
> - << "Multiple raw streams are not supported";
> - break;
> - }
> -
> - stream = &data->rawStream_;
> -
> - cfg.size = data->cio2_.sensor()->resolution();
> + cfg = data->cio2_.generateConfiguration(sensorResolution);
> + cfg.bufferCount = 1;
Setting bufferCount here is goes against the idea of having
CIO2Device::generateConfiguration() control all the parameters of the
RAW stream. I know there was some discussion if returning a
StreamConfiguration from the CIO2Device is a good idea or not. I still
think it's the best design but I'm open to change/discuss it.
But if we really want to modify parameters of it here I agree returning
a StreamConfiguration here is confusing, double so as bufferCount is set
to CIO2_BUFFER_COUNT and then changed to 1 here.
>
> - cfg = data->cio2_.generateConfiguration(cfg.size);
> break;
> }
>
> case StreamRole::Viewfinder:
> case StreamRole::VideoRecording: {
> /*
> - * We can't use the 'output' stream for viewfinder or
> - * video capture roles.
> - *
> - * \todo This is an artificial limitation until we
> - * figure out the exact capabilities of the hardware.
> + * Default viewfinder to 1280x720, capped to the maximum
> + * sensor resolution and aligned to the ImgU output
> + * constraints.
This is done for both Viewfinder and VideoRecording right?
> */
> - if (streams.find(&data->vfStream_) == streams.end()) {
> - LOG(IPU3, Error)
> - << "No stream available for requested role "
> - << role;
> - break;
> - }
> -
> - stream = &data->vfStream_;
> -
> - /*
> - * Align the default viewfinder size to the maximum
> - * available sensor resolution and to the IPU3
> - * alignment constraints.
> - */
> - const Size &res = data->cio2_.sensor()->resolution();
> - unsigned int width = std::min(1280U, res.width);
> - unsigned int height = std::min(720U, res.height);
> - cfg.size = { width & ~7, height & ~3 };
> + cfg.size.width = std::min(1280U, sensorResolution.width);
> + cfg.size.height = std::min(720U, sensorResolution.height);
> + cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> + cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> + cfg.pixelFormat = formats::NV12;
> + cfg.bufferCount = IPU3_BUFFER_COUNT;
>
> break;
> }
> @@ -388,16 +350,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> default:
> LOG(IPU3, Error)
> << "Requested stream role not supported: " << role;
> - break;
I would keep the break as it is the style elsewhere in camera is to
terminate the default case with either break, return or continue.
> - }
> -
> - if (!stream) {
> delete config;
> return nullptr;
> }
>
> - streams.erase(stream);
> -
> config->addConfiguration(cfg);
> }
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list