| Message ID | 20190419132531.17856-5-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Apr 19, 2019 at 03:25:27PM +0200, Jacopo Mondi wrote: > Use and inspect the stream roles provided by the application to > streamConfiguration() to assign streams to their intended roles and > return a default configuration associated with them. > > Support a limited number of usages, with the viewfinder stream able to > capture both continuous video streams and still images, and the main > output stream supporting still images only. This is an artificial > limitation until we figure out the exact capabilities of the hardware. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 124 +++++++++++++++++++++------ > 1 file changed, 98 insertions(+), 26 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 46384d88dddd..0130a83973ca 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -5,6 +5,7 @@ > * ipu3.cpp - Pipeline handler for Intel IPU3 > */ > > +#include <algorithm> > #include <iomanip> > #include <memory> > #include <vector> > @@ -221,34 +222,105 @@ CameraConfiguration > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > const std::vector<StreamUsage> &usages) > { > - CameraConfiguration configs; > IPU3CameraData *data = cameraData(camera); > - StreamConfiguration config = {}; > + CameraConfiguration cameraConfig = {}; > + std::set<IPU3Stream *> streams = { > + &data->outStream_, > + &data->vfStream_, > + }; > > - /* > - * 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 alignement requirements. > - */ > - config.width = 2560; > - config.height = 1920; > - config.pixelFormat = V4L2_PIX_FMT_NV12; > - config.bufferCount = IPU3_BUFFER_COUNT; > - > - configs[&data->outStream_] = config; > - LOG(IPU3, Debug) > - << "Stream '" << data->outStream_.name_ << "' format set to " > - << config.toString(); > - > - configs[&data->vfStream_] = config; > - LOG(IPU3, Debug) > - << "Stream '" << data->vfStream_.name_ << "' format set to " > - << config.toString(); > - > - return configs; > + for (const StreamUsage &usage : usages) { > + StreamConfiguration streamConfig = {}; > + StreamUsage::Role role = usage.role(); > + IPU3Stream *stream = nullptr; > + > + switch (role) { > + case StreamUsage::Role::StillCapture: > + /* > + * Don't allow viewfinder or video capture on the > + * 'output' stream. This is an artificial limitation > + * until we figure out the capabilities of the > + * hardware. This comment is a bit confusing. How about "Pick the output stream by default as the Viewfinder and VideoRecording roles are not allowed on the viewfinder 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. > + */ > + streamConfig.width = 2560; > + streamConfig.height = 1920; > + > + break; Please add a blank line here. > + case StreamUsage::Role::Viewfinder: > + case StreamUsage::Role::VideoRecording: { > + /* > + * We can't use the 'output' stream for viewfinder or > + * video capture usages. > + * > + * \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 requested 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(usage.size().width, > + res.width); > + unsigned int height = std::min(usage.size().height, > + res.height); > + streamConfig.width = width & ~7; > + streamConfig.height = height & ~3; > + > + break; > + } And a blank line here too. > + default: > + LOG(IPU3, Error) > + << "Requested stream role not supported: " << role; > + break; > + } > + > + if (!stream) > + return cameraConfig; This will return a valid configuration if the first role was fulfilled but the first one wasn't. You need to return CameraConfiguration{} here. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + streams.erase(stream); > + > + streamConfig.pixelFormat = V4L2_PIX_FMT_NV12; > + streamConfig.bufferCount = IPU3_BUFFER_COUNT; > + > + cameraConfig[stream] = streamConfig; > + > + LOG(IPU3, Debug) > + << "Stream '" << stream->name_ << "' format set to " > + << streamConfig.toString(); > + } > + > + return cameraConfig; > } > > int PipelineHandlerIPU3::configureStreams(Camera *camera,
Hi Laurent, On Fri, Apr 19, 2019 at 04:35:27PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Apr 19, 2019 at 03:25:27PM +0200, Jacopo Mondi wrote: > > Use and inspect the stream roles provided by the application to > > streamConfiguration() to assign streams to their intended roles and > > return a default configuration associated with them. > > > > Support a limited number of usages, with the viewfinder stream able to > > capture both continuous video streams and still images, and the main > > output stream supporting still images only. This is an artificial > > limitation until we figure out the exact capabilities of the hardware. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 124 +++++++++++++++++++++------ > > 1 file changed, 98 insertions(+), 26 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 46384d88dddd..0130a83973ca 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -5,6 +5,7 @@ > > * ipu3.cpp - Pipeline handler for Intel IPU3 > > */ > > > > +#include <algorithm> > > #include <iomanip> > > #include <memory> > > #include <vector> > > @@ -221,34 +222,105 @@ CameraConfiguration > > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > const std::vector<StreamUsage> &usages) > > { > > - CameraConfiguration configs; > > IPU3CameraData *data = cameraData(camera); > > - StreamConfiguration config = {}; > > + CameraConfiguration cameraConfig = {}; > > + std::set<IPU3Stream *> streams = { > > + &data->outStream_, > > + &data->vfStream_, > > + }; > > > > - /* > > - * 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 alignement requirements. > > - */ > > - config.width = 2560; > > - config.height = 1920; > > - config.pixelFormat = V4L2_PIX_FMT_NV12; > > - config.bufferCount = IPU3_BUFFER_COUNT; > > - > > - configs[&data->outStream_] = config; > > - LOG(IPU3, Debug) > > - << "Stream '" << data->outStream_.name_ << "' format set to " > > - << config.toString(); > > - > > - configs[&data->vfStream_] = config; > > - LOG(IPU3, Debug) > > - << "Stream '" << data->vfStream_.name_ << "' format set to " > > - << config.toString(); > > - > > - return configs; > > + for (const StreamUsage &usage : usages) { > > + StreamConfiguration streamConfig = {}; > > + StreamUsage::Role role = usage.role(); > > + IPU3Stream *stream = nullptr; > > + > > + switch (role) { > > + case StreamUsage::Role::StillCapture: > > + /* > > + * Don't allow viewfinder or video capture on the > > + * 'output' stream. This is an artificial limitation > > + * until we figure out the capabilities of the > > + * hardware. > > This comment is a bit confusing. How about "Pick the output stream by > default as the Viewfinder and VideoRecording roles are not allowed on > the viewfinder stream." ? > Taken in with s/viewfinder stream/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. > > + */ > > + streamConfig.width = 2560; > > + streamConfig.height = 1920; > > + > > + break; > > Please add a blank line here. > > > + case StreamUsage::Role::Viewfinder: > > + case StreamUsage::Role::VideoRecording: { > > + /* > > + * We can't use the 'output' stream for viewfinder or > > + * video capture usages. > > + * > > + * \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 requested 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(usage.size().width, > > + res.width); > > + unsigned int height = std::min(usage.size().height, > > + res.height); > > + streamConfig.width = width & ~7; > > + streamConfig.height = height & ~3; > > + > > + break; > > + } > > And a blank line here too. > > > + default: > > + LOG(IPU3, Error) > > + << "Requested stream role not supported: " << role; > > + break; > > + } > > + > > + if (!stream) > > + return cameraConfig; > > This will return a valid configuration if the first role was fulfilled > but the first one wasn't. You need to return CameraConfiguration{} here. Indeed. > > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks j > > > + > > + streams.erase(stream); > > + > > + streamConfig.pixelFormat = V4L2_PIX_FMT_NV12; > > + streamConfig.bufferCount = IPU3_BUFFER_COUNT; > > + > > + cameraConfig[stream] = streamConfig; > > + > > + LOG(IPU3, Debug) > > + << "Stream '" << stream->name_ << "' format set to " > > + << streamConfig.toString(); > > + } > > + > > + return cameraConfig; > > } > > > > int PipelineHandlerIPU3::configureStreams(Camera *camera, > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 46384d88dddd..0130a83973ca 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -5,6 +5,7 @@ * ipu3.cpp - Pipeline handler for Intel IPU3 */ +#include <algorithm> #include <iomanip> #include <memory> #include <vector> @@ -221,34 +222,105 @@ CameraConfiguration PipelineHandlerIPU3::streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) { - CameraConfiguration configs; IPU3CameraData *data = cameraData(camera); - StreamConfiguration config = {}; + CameraConfiguration cameraConfig = {}; + std::set<IPU3Stream *> streams = { + &data->outStream_, + &data->vfStream_, + }; - /* - * 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 alignement requirements. - */ - config.width = 2560; - config.height = 1920; - config.pixelFormat = V4L2_PIX_FMT_NV12; - config.bufferCount = IPU3_BUFFER_COUNT; - - configs[&data->outStream_] = config; - LOG(IPU3, Debug) - << "Stream '" << data->outStream_.name_ << "' format set to " - << config.toString(); - - configs[&data->vfStream_] = config; - LOG(IPU3, Debug) - << "Stream '" << data->vfStream_.name_ << "' format set to " - << config.toString(); - - return configs; + for (const StreamUsage &usage : usages) { + StreamConfiguration streamConfig = {}; + StreamUsage::Role role = usage.role(); + IPU3Stream *stream = nullptr; + + switch (role) { + case StreamUsage::Role::StillCapture: + /* + * Don't allow viewfinder or video capture on the + * 'output' stream. This is an artificial limitation + * until we figure out the capabilities of the + * hardware. + */ + 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. + */ + streamConfig.width = 2560; + streamConfig.height = 1920; + + break; + case StreamUsage::Role::Viewfinder: + case StreamUsage::Role::VideoRecording: { + /* + * We can't use the 'output' stream for viewfinder or + * video capture usages. + * + * \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 requested 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(usage.size().width, + res.width); + unsigned int height = std::min(usage.size().height, + res.height); + streamConfig.width = width & ~7; + streamConfig.height = height & ~3; + + break; + } + default: + LOG(IPU3, Error) + << "Requested stream role not supported: " << role; + break; + } + + if (!stream) + return cameraConfig; + + streams.erase(stream); + + streamConfig.pixelFormat = V4L2_PIX_FMT_NV12; + streamConfig.bufferCount = IPU3_BUFFER_COUNT; + + cameraConfig[stream] = streamConfig; + + LOG(IPU3, Debug) + << "Stream '" << stream->name_ << "' format set to " + << streamConfig.toString(); + } + + return cameraConfig; } int PipelineHandlerIPU3::configureStreams(Camera *camera,
Use and inspect the stream roles provided by the application to streamConfiguration() to assign streams to their intended roles and return a default configuration associated with them. Support a limited number of usages, with the viewfinder stream able to capture both continuous video streams and still images, and the main output stream supporting still images only. This is an artificial limitation until we figure out the exact capabilities of the hardware. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 124 +++++++++++++++++++++------ 1 file changed, 98 insertions(+), 26 deletions(-)