[libcamera-devel] [PATCH v2 02/20] libcamera: ipu3: Remove streams from generateConfiguration
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 10 08:58:54 CEST 2020
Hi Niklas,
thanks for review
On Thu, Jul 09, 2020 at 02:59:42PM +0200, Niklas Söderlund wrote:
> 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?
>
It seems so
drivers/staging/media/ipu3/ipu3.h-#define IPU3_OUTPUT_MAX_HEIGHT 34004U
> > +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.
Ah, this was not intentional in first place to be honest.
I would be happy to discuss (later) what is the best to return from
the CIO2 configuration, as I would have preferred not returning a
format, but that's oke for now.
>
> 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.
>
What confused me is the pattern
cfg.size = data->cio2_.sensor()->resolution();
cfg = data->cio2_.generateConfiguration(cfg.size);
As using cfg.size just a place holder to pass it to the
generateConfiguration() function is not nice imho. But removing the
assignement of the whole configuration was not intentional, so I'll
fix it.
> >
> > - 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?
>
Yes, I'll update the comment
> > */
> > - 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.
>
The code now looks like this
default:
LOG(IPU3, Error)
<< "Requested stream role not supported: " << role;
delete config;
return nullptr;
I think it's right
Thanks
j
> > - }
> > -
> > - 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