| Message ID | 20200701123036.51922-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote: > Remove stream assignement from the IPU3 pipeline handler > generateConfiguration() implementation. > > The function aims to provide a suitable default for the requested use > cases. Defer stream assignement to validation and only assign sizes > and formats to stream configurations. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++-------------------- > 1 file changed, 27 insertions(+), 66 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1bdad209de6e..97fc8b60c3cb 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -31,6 +31,9 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > +static constexpr unsigned int IPU3_MAX_STREAMS = 3; > + > class IPU3CameraData : public CameraData > { > public: > @@ -61,9 +64,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 +293,55 @@ 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(); > + unsigned int rawCount = 0; > + unsigned int outCount = 0; Does it make sens to add check of number of streams here? In 7/15 you add the same to validate() and validate is called at the and of generateConfiguration(). Other then this open question I really like this patch! > 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. > + * Use the sensor resolution adjusted to respect the > + * ImgU output alignement contraints. > */ > - 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; > - } > + cfg.pixelFormat = formats::NV12; > + cfg.size = sensorResolution; > + cfg.size.width &= ~7; > + cfg.size.height &= ~3; > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > - /* > - * 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. > - */ > - cfg.size = { 2560, 1920 }; > + outCount++; > > break; > > case StreamRole::StillCaptureRaw: { > - if (streams.find(&data->rawStream_) == streams.end()) { > - LOG(IPU3, Error) > - << "Multiple raw streams are not supported"; > - break; > - } > + cfg = data->cio2_.generateConfiguration(sensorResolution); > + cfg.bufferCount = 1; > > - stream = &data->rawStream_; > + rawCount++; > > - cfg.size = data->cio2_.sensor()->resolution(); > - > - 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. > - */ > - 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); > + unsigned int width = std::min(1280U, sensorResolution.width); > + unsigned int height = std::min(720U, sensorResolution.height); > cfg.size = { width & ~7, height & ~3 }; > + cfg.pixelFormat = formats::NV12; > + cfg.bufferCount = IPU3_BUFFER_COUNT; > + > + outCount++; > > break; > } > @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > default: > LOG(IPU3, Error) > << "Requested stream role not supported: " << role; > - break; > + delete config; > + return nullptr; > } > > - if (!stream) { > + if (rawCount > 1 || outCount > 2) { > + LOG(IPU3, Error) << "Invalid stream roles requested"; > delete config; > return nullptr; > } > > - streams.erase(stream); > - > config->addConfiguration(cfg); > } > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Jul 01, 2020 at 06:23:06PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote: > > Remove stream assignement from the IPU3 pipeline handler > > generateConfiguration() implementation. > > > > The function aims to provide a suitable default for the requested use > > cases. Defer stream assignement to validation and only assign sizes > > and formats to stream configurations. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++-------------------- > > 1 file changed, 27 insertions(+), 66 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 1bdad209de6e..97fc8b60c3cb 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -31,6 +31,9 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > > +static constexpr unsigned int IPU3_MAX_STREAMS = 3; > > + > > class IPU3CameraData : public CameraData > > { > > public: > > @@ -61,9 +64,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 +293,55 @@ 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(); > > + unsigned int rawCount = 0; > > + unsigned int outCount = 0; > > Does it make sens to add check of number of streams here? In 7/15 you > add the same to validate() and validate is called at the and of > generateConfiguration(). It's a bit of a repetetion, I agree. Should we check for the status returned by validate() then ? > > Other then this open question I really like this patch! > > > 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. > > + * Use the sensor resolution adjusted to respect the > > + * ImgU output alignement contraints. > > */ > > - 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; > > - } > > + cfg.pixelFormat = formats::NV12; > > + cfg.size = sensorResolution; > > + cfg.size.width &= ~7; > > + cfg.size.height &= ~3; > > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > > > - /* > > - * 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. > > - */ > > - cfg.size = { 2560, 1920 }; > > + outCount++; > > > > break; > > > > case StreamRole::StillCaptureRaw: { > > - if (streams.find(&data->rawStream_) == streams.end()) { > > - LOG(IPU3, Error) > > - << "Multiple raw streams are not supported"; > > - break; > > - } > > + cfg = data->cio2_.generateConfiguration(sensorResolution); > > + cfg.bufferCount = 1; > > > > - stream = &data->rawStream_; > > + rawCount++; > > > > - cfg.size = data->cio2_.sensor()->resolution(); > > - > > - 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. > > - */ > > - 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); > > + unsigned int width = std::min(1280U, sensorResolution.width); > > + unsigned int height = std::min(720U, sensorResolution.height); > > cfg.size = { width & ~7, height & ~3 }; > > + cfg.pixelFormat = formats::NV12; > > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > + > > + outCount++; > > > > break; > > } > > @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > default: > > LOG(IPU3, Error) > > << "Requested stream role not supported: " << role; > > - break; > > + delete config; > > + return nullptr; > > } > > > > - if (!stream) { > > + if (rawCount > 1 || outCount > 2) { > > + LOG(IPU3, Error) << "Invalid stream roles requested"; > > delete config; > > return nullptr; > > } > > > > - streams.erase(stream); > > - > > config->addConfiguration(cfg); > > } > > > > -- > > 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, On 2020-07-02 09:33:36 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Wed, Jul 01, 2020 at 06:23:06PM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2020-07-01 14:30:23 +0200, Jacopo Mondi wrote: > > > Remove stream assignement from the IPU3 pipeline handler > > > generateConfiguration() implementation. > > > > > > The function aims to provide a suitable default for the requested use > > > cases. Defer stream assignement to validation and only assign sizes > > > and formats to stream configurations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++-------------------- > > > 1 file changed, 27 insertions(+), 66 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 1bdad209de6e..97fc8b60c3cb 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -31,6 +31,9 @@ namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > > > +static constexpr unsigned int IPU3_MAX_STREAMS = 3; > > > + > > > class IPU3CameraData : public CameraData > > > { > > > public: > > > @@ -61,9 +64,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 +293,55 @@ 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(); > > > + unsigned int rawCount = 0; > > > + unsigned int outCount = 0; > > > > Does it make sens to add check of number of streams here? In 7/15 you > > add the same to validate() and validate is called at the and of > > generateConfiguration(). > > It's a bit of a repetetion, I agree. > > Should we check for the status returned by validate() then ? I think that would be nicer as we then collect all checks on the configuration in validate() while at the same time guarantee the configuration returned from generateConfiguration() is OK. > > > > > Other then this open question I really like this patch! > > > > > 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. > > > + * Use the sensor resolution adjusted to respect the > > > + * ImgU output alignement contraints. > > > */ > > > - 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; > > > - } > > > + cfg.pixelFormat = formats::NV12; > > > + cfg.size = sensorResolution; > > > + cfg.size.width &= ~7; > > > + cfg.size.height &= ~3; > > > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > > > > > - /* > > > - * 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. > > > - */ > > > - cfg.size = { 2560, 1920 }; > > > + outCount++; > > > > > > break; > > > > > > case StreamRole::StillCaptureRaw: { > > > - if (streams.find(&data->rawStream_) == streams.end()) { > > > - LOG(IPU3, Error) > > > - << "Multiple raw streams are not supported"; > > > - break; > > > - } > > > + cfg = data->cio2_.generateConfiguration(sensorResolution); > > > + cfg.bufferCount = 1; > > > > > > - stream = &data->rawStream_; > > > + rawCount++; > > > > > > - cfg.size = data->cio2_.sensor()->resolution(); > > > - > > > - 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. > > > - */ > > > - 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); > > > + unsigned int width = std::min(1280U, sensorResolution.width); > > > + unsigned int height = std::min(720U, sensorResolution.height); > > > cfg.size = { width & ~7, height & ~3 }; > > > + cfg.pixelFormat = formats::NV12; > > > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > > + > > > + outCount++; > > > > > > break; > > > } > > > @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > default: > > > LOG(IPU3, Error) > > > << "Requested stream role not supported: " << role; > > > - break; > > > + delete config; > > > + return nullptr; > > > } > > > > > > - if (!stream) { > > > + if (rawCount > 1 || outCount > 2) { > > > + LOG(IPU3, Error) << "Invalid stream roles requested"; > > > delete config; > > > return nullptr; > > > } > > > > > > - streams.erase(stream); > > > - > > > config->addConfiguration(cfg); > > > } > > > > > > -- > > > 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, On 01/07/2020 13:30, Jacopo Mondi wrote: > Remove stream assignement from the IPU3 pipeline handler > generateConfiguration() implementation. > > The function aims to provide a suitable default for the requested use > cases. Defer stream assignement to validation and only assign sizes > and formats to stream configurations. s/assignement/assignment/ in two locations above Excellent, this sounds like exactly what will be needed as part of the use of empty configurations for the Android HAL multi-stream work. Is all the code being removed already in validate()? I see an 'assignStreams()' there so I guess that covers it. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Trivial comment below but no blocker, and I saw Niklas had a couple of minor comments too, but those aside, I'll see how the rest of this series affects the HAL multi-stream work. -- Kieran > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++-------------------- > 1 file changed, 27 insertions(+), 66 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1bdad209de6e..97fc8b60c3cb 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -31,6 +31,9 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > +static constexpr unsigned int IPU3_MAX_STREAMS = 3; > + > class IPU3CameraData : public CameraData > { > public: > @@ -61,9 +64,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 +293,55 @@ 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(); > + unsigned int rawCount = 0; > + unsigned int outCount = 0; > 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. > + * Use the sensor resolution adjusted to respect the > + * ImgU output alignement contraints. > */ > - 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; > - } > + cfg.pixelFormat = formats::NV12; > + cfg.size = sensorResolution; > + cfg.size.width &= ~7; > + cfg.size.height &= ~3; Hrm ... do we need some align macros to make this more readable? Not necessarily required for this patch though. > + cfg.bufferCount = IPU3_BUFFER_COUNT; > > - /* > - * 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. > - */ > - cfg.size = { 2560, 1920 }; > + outCount++; > > break; > > case StreamRole::StillCaptureRaw: { > - if (streams.find(&data->rawStream_) == streams.end()) { > - LOG(IPU3, Error) > - << "Multiple raw streams are not supported"; > - break; > - } > + cfg = data->cio2_.generateConfiguration(sensorResolution); > + cfg.bufferCount = 1; > > - stream = &data->rawStream_; > + rawCount++; > > - cfg.size = data->cio2_.sensor()->resolution(); > - > - 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. > - */ > - 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); > + unsigned int width = std::min(1280U, sensorResolution.width); > + unsigned int height = std::min(720U, sensorResolution.height); > cfg.size = { width & ~7, height & ~3 }; > + cfg.pixelFormat = formats::NV12; > + cfg.bufferCount = IPU3_BUFFER_COUNT; > + > + outCount++; > > break; > } > @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > default: > LOG(IPU3, Error) > << "Requested stream role not supported: " << role; > - break; > + delete config; > + return nullptr; > } > > - if (!stream) { > + if (rawCount > 1 || outCount > 2) { > + LOG(IPU3, Error) << "Invalid stream roles requested"; > delete config; > return nullptr; > } > > - streams.erase(stream); > - > config->addConfiguration(cfg); > } > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 1bdad209de6e..97fc8b60c3cb 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -31,6 +31,9 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +static constexpr unsigned int IPU3_BUFFER_COUNT = 4; +static constexpr unsigned int IPU3_MAX_STREAMS = 3; + class IPU3CameraData : public CameraData { public: @@ -61,9 +64,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 +293,55 @@ 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(); + unsigned int rawCount = 0; + unsigned int outCount = 0; 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. + * Use the sensor resolution adjusted to respect the + * ImgU output alignement contraints. */ - 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; - } + cfg.pixelFormat = formats::NV12; + cfg.size = sensorResolution; + cfg.size.width &= ~7; + cfg.size.height &= ~3; + cfg.bufferCount = IPU3_BUFFER_COUNT; - /* - * 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. - */ - cfg.size = { 2560, 1920 }; + outCount++; break; case StreamRole::StillCaptureRaw: { - if (streams.find(&data->rawStream_) == streams.end()) { - LOG(IPU3, Error) - << "Multiple raw streams are not supported"; - break; - } + cfg = data->cio2_.generateConfiguration(sensorResolution); + cfg.bufferCount = 1; - stream = &data->rawStream_; + rawCount++; - cfg.size = data->cio2_.sensor()->resolution(); - - 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. - */ - 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); + unsigned int width = std::min(1280U, sensorResolution.width); + unsigned int height = std::min(720U, sensorResolution.height); cfg.size = { width & ~7, height & ~3 }; + cfg.pixelFormat = formats::NV12; + cfg.bufferCount = IPU3_BUFFER_COUNT; + + outCount++; break; } @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, default: LOG(IPU3, Error) << "Requested stream role not supported: " << role; - break; + delete config; + return nullptr; } - if (!stream) { + if (rawCount > 1 || outCount > 2) { + LOG(IPU3, Error) << "Invalid stream roles requested"; delete config; return nullptr; } - streams.erase(stream); - config->addConfiguration(cfg); }
Remove stream assignement from the IPU3 pipeline handler generateConfiguration() implementation. The function aims to provide a suitable default for the requested use cases. Defer stream assignement to validation and only assign sizes and formats to stream configurations. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++-------------------- 1 file changed, 27 insertions(+), 66 deletions(-)