| Message ID | 20190409192548.20325-12-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-04-09 21:25:47 +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, as the viewfinder stream can > optionally be used for capturing still images, but the main output > stream cannot be used as viewfinder or for video recording purposes. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++------- > 1 file changed, 79 insertions(+), 26 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 75ffdc56d157..70a92783076f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -227,36 +227,89 @@ CameraConfiguration > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > const std::vector<StreamUsage> &usages) > { > - CameraConfiguration configs; > IPU3CameraData *data = cameraData(camera); > - StreamConfiguration config = {}; > + CameraConfiguration configs; > + std::vector<IPU3Stream *> availableStreams = { Using a std::set instead of std::vector here would be useful. As you can't have the same stream in the set twice there is no limitation moving from a vector to a set. > + &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 'output' format set to " << config.width << "x" > - << config.height << "-0x" << std::hex << std::setfill('0') > - << std::setw(8) << config.pixelFormat; > - > - configs[&data->vfStream_] = config; > - LOG(IPU3, Debug) > - << "Stream 'viewfinder' format set to " << config.width << "x" > - << config.height << "-0x" << std::hex << std::setfill('0') > - << std::setw(8) << config.pixelFormat; > + for (const StreamUsage &usage : usages) { > + std::vector<IPU3Stream *>::iterator found; Can be refactored away if availableStreams is a std::set, see example bellow. > + enum StreamUsage::Role r = usage.role(); s/enum// > + StreamConfiguration config = {}; > + IPU3Stream *stream = nullptr; > + > + std::vector<IPU3Stream *>::iterator s = availableStreams.begin(); > + if (r == StreamUsage::Role::StillCapture) { > + for (; s < availableStreams.end(); ++s) { > + /* > + * We can use the viewfinder stream in case > + * the 'StillCapture' usage is required > + * multiple times. > + */ > + if (*s == &data->outStream_) { > + stream = &data->outStream_; > + found = s; > + break; > + } else { > + stream = &data->vfStream_; > + found = s; > + } > + } The for() loop can be replaced with this if availableStreams is std::set if (availableStreams.find(&data->outStream_) != availableStreams.end()) stream = &data->outStream_; else stream = &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 alignment requirements. > + */ > + config.width = 2560; > + config.height = 1920; > + } else if (r == StreamUsage::Role::Viewfinder || > + r == StreamUsage::Role::VideoRecording) { > + for (; s < availableStreams.end(); ++s) { > + /* > + * We can't use the 'output' stream for > + * viewfinder or video capture usages. > + */ > + if (*s != &data->vfStream_) > + continue; > + > + stream = &data->vfStream_; > + found = s; > + break; > + } > + > + config.width = 640; > + config.height = 480; > + } > + > + if (stream == nullptr) > + goto error; > + > + availableStreams.erase(found); With std::set availableStreams.erase(stream); > + > + config.pixelFormat = V4L2_PIX_FMT_NV12; > + config.bufferCount = IPU3_BUFFER_COUNT; > + > + configs[stream] = config; > + > + LOG(IPU3, Debug) > + << "Stream " << stream->name_ << " format set to " > + << config.width << "x" << config.height > + << "-0x" << std::hex << std::setfill('0') > + << std::setw(8) << config.pixelFormat; > + } > > return configs; > + > +error: > + LOG(IPU3, Error) << "Requested stream roles not supported"; > + return CameraConfiguration{}; > } > > int PipelineHandlerIPU3::configureStreams(Camera *camera, > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote: > On 2019-04-09 21:25:47 +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, as the viewfinder stream can > > optionally be used for capturing still images, but the main output > > stream cannot be used as viewfinder or for video recording purposes. Is this a limitation of the device, or of the pipeline handler ? > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++------- > > 1 file changed, 79 insertions(+), 26 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 75ffdc56d157..70a92783076f 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -227,36 +227,89 @@ CameraConfiguration > > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > const std::vector<StreamUsage> &usages) > > { > > - CameraConfiguration configs; > > IPU3CameraData *data = cameraData(camera); > > - StreamConfiguration config = {}; > > + CameraConfiguration configs; I wouldn't use the plural for a single object. How about CameraConfiguration config and StreamConfiguration cfg ? Or CameraConfiguration ccfg and StreamConfiguration scfg ? There's also the option of cameraConfig and streamConfig but that may be a bit long. > > + std::vector<IPU3Stream *> availableStreams = { > > Using a std::set instead of std::vector here would be useful. As you > can't have the same stream in the set twice there is no limitation > moving from a vector to a set. > > > + &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 'output' format set to " << config.width << "x" > > - << config.height << "-0x" << std::hex << std::setfill('0') > > - << std::setw(8) << config.pixelFormat; > > - > > - configs[&data->vfStream_] = config; > > - LOG(IPU3, Debug) > > - << "Stream 'viewfinder' format set to " << config.width << "x" > > - << config.height << "-0x" << std::hex << std::setfill('0') > > - << std::setw(8) << config.pixelFormat; > > + for (const StreamUsage &usage : usages) { > > + std::vector<IPU3Stream *>::iterator found; > > Can be refactored away if availableStreams is a std::set, see example > bellow. > > > + enum StreamUsage::Role r = usage.role(); > > s/enum// And s/r/role/ ? r is a bit confusing, it also commonly refers to a rectangle. > > + StreamConfiguration config = {}; > > + IPU3Stream *stream = nullptr; > > + > > + std::vector<IPU3Stream *>::iterator s = availableStreams.begin(); > > + if (r == StreamUsage::Role::StillCapture) { > > + for (; s < availableStreams.end(); ++s) { > > + /* > > + * We can use the viewfinder stream in case > > + * the 'StillCapture' usage is required > > + * multiple times. > > + */ > > + if (*s == &data->outStream_) { > > + stream = &data->outStream_; > > + found = s; > > + break; > > + } else { > > + stream = &data->vfStream_; > > + found = s; > > + } > > + } > > The for() loop can be replaced with this if availableStreams is std::set > > > if (availableStreams.find(&data->outStream_) != availableStreams.end()) > stream = &data->outStream_; > else > stream = &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 alignment requirements. > > + */ > > + config.width = 2560; > > + config.height = 1920; > > + } else if (r == StreamUsage::Role::Viewfinder || > > + r == StreamUsage::Role::VideoRecording) { > > + for (; s < availableStreams.end(); ++s) { > > + /* > > + * We can't use the 'output' stream for > > + * viewfinder or video capture usages. > > + */ > > + if (*s != &data->vfStream_) > > + continue; > > + > > + stream = &data->vfStream_; > > + found = s; > > + break; > > + } And here if (availableStreams.find(&data->vfStream_) != availableStreams.end()) stream = &data->vfStream_; You may want to rename availableStreams to streams if it helps shortening lines, up to you. > > + > > + config.width = 640; > > + config.height = 480; Should you use the resolution requested by the Viewfinder usage ? > > + } > > + > > + if (stream == nullptr) > > + goto error; > > + > > + availableStreams.erase(found); > > With std::set > > availableStreams.erase(stream); > > > + > > + config.pixelFormat = V4L2_PIX_FMT_NV12; > > + config.bufferCount = IPU3_BUFFER_COUNT; > > + > > + configs[stream] = config; > > + > > + LOG(IPU3, Debug) > > + << "Stream " << stream->name_ << " format set to " > > + << config.width << "x" << config.height > > + << "-0x" << std::hex << std::setfill('0') > > + << std::setw(8) << config.pixelFormat; > > + } > > > > return configs; > > + > > +error: > > + LOG(IPU3, Error) << "Requested stream roles not supported"; > > + return CameraConfiguration{}; > > } > > > > int PipelineHandlerIPU3::configureStreams(Camera *camera,
Hi Laurent, Niklas, On Mon, Apr 15, 2019 at 05:42:44PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote: > > On 2019-04-09 21:25:47 +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, as the viewfinder stream can > > > optionally be used for capturing still images, but the main output > > > stream cannot be used as viewfinder or for video recording purposes. > > Is this a limitation of the device, or of the pipeline handler ? > Is a limitation I introduced in the pipeline handler as I assume viewfinder frame rate cannot be sustained by the still capture output node. It's an assumption though, but I kept it there to demonstrate how to use roles in assigning configurations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++------- > > > 1 file changed, 79 insertions(+), 26 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 75ffdc56d157..70a92783076f 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -227,36 +227,89 @@ CameraConfiguration > > > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > > const std::vector<StreamUsage> &usages) > > > { > > > - CameraConfiguration configs; > > > IPU3CameraData *data = cameraData(camera); > > > - StreamConfiguration config = {}; > > > + CameraConfiguration configs; > > I wouldn't use the plural for a single object. How about > CameraConfiguration config and StreamConfiguration cfg ? Or > CameraConfiguration ccfg and StreamConfiguration scfg ? There's also the > option of cameraConfig and streamConfig but that may be a bit long. > > > > + std::vector<IPU3Stream *> availableStreams = { > > > > Using a std::set instead of std::vector here would be useful. As you > > can't have the same stream in the set twice there is no limitation > > moving from a vector to a set. > > > > > + &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 'output' format set to " << config.width << "x" > > > - << config.height << "-0x" << std::hex << std::setfill('0') > > > - << std::setw(8) << config.pixelFormat; > > > - > > > - configs[&data->vfStream_] = config; > > > - LOG(IPU3, Debug) > > > - << "Stream 'viewfinder' format set to " << config.width << "x" > > > - << config.height << "-0x" << std::hex << std::setfill('0') > > > - << std::setw(8) << config.pixelFormat; > > > + for (const StreamUsage &usage : usages) { > > > + std::vector<IPU3Stream *>::iterator found; > > > > Can be refactored away if availableStreams is a std::set, see example > > bellow. > > > > > + enum StreamUsage::Role r = usage.role(); > > > > s/enum// > > And s/r/role/ ? r is a bit confusing, it also commonly refers to a > rectangle. > > > > + StreamConfiguration config = {}; > > > + IPU3Stream *stream = nullptr; > > > + > > > + std::vector<IPU3Stream *>::iterator s = availableStreams.begin(); > > > + if (r == StreamUsage::Role::StillCapture) { > > > + for (; s < availableStreams.end(); ++s) { > > > + /* > > > + * We can use the viewfinder stream in case > > > + * the 'StillCapture' usage is required > > > + * multiple times. > > > + */ > > > + if (*s == &data->outStream_) { > > > + stream = &data->outStream_; > > > + found = s; > > > + break; > > > + } else { > > > + stream = &data->vfStream_; > > > + found = s; > > > + } > > > + } > > > > The for() loop can be replaced with this if availableStreams is std::set > > > > > > if (availableStreams.find(&data->outStream_) != availableStreams.end()) > > stream = &data->outStream_; > > else > > stream = &data->vfStream_; > > Much better, thanks! Just one note, this is not enough, as we might get asked for 3 viewfinders, so we have to make sure vfStream_ is available as well, and fail eventually if it is not. > > > + > > > + /* > > > + * 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. > > > + */ > > > + config.width = 2560; > > > + config.height = 1920; > > > + } else if (r == StreamUsage::Role::Viewfinder || > > > + r == StreamUsage::Role::VideoRecording) { > > > + for (; s < availableStreams.end(); ++s) { > > > + /* > > > + * We can't use the 'output' stream for > > > + * viewfinder or video capture usages. > > > + */ > > > + if (*s != &data->vfStream_) > > > + continue; > > > + > > > + stream = &data->vfStream_; > > > + found = s; > > > + break; > > > + } > > And here > > if (availableStreams.find(&data->vfStream_) != > availableStreams.end()) > stream = &data->vfStream_; > > You may want to rename availableStreams to streams if it helps > shortening lines, up to you. > > > > + > > > + config.width = 640; > > > + config.height = 480; > > Should you use the resolution requested by the Viewfinder usage ? > Yes indeed! Thanks, with Niklas and your suggestions this looks much much better! > > > + } > > > + > > > + if (stream == nullptr) > > > + goto error; > > > + > > > + availableStreams.erase(found); > > > > With std::set > > > > availableStreams.erase(stream); > > > > > + > > > + config.pixelFormat = V4L2_PIX_FMT_NV12; > > > + config.bufferCount = IPU3_BUFFER_COUNT; > > > + > > > + configs[stream] = config; > > > + > > > + LOG(IPU3, Debug) > > > + << "Stream " << stream->name_ << " format set to " > > > + << config.width << "x" << config.height > > > + << "-0x" << std::hex << std::setfill('0') > > > + << std::setw(8) << config.pixelFormat; > > > + } > > > > > > return configs; > > > + > > > +error: > > > + LOG(IPU3, Error) << "Requested stream roles not supported"; > > > + return CameraConfiguration{}; > > > } > > > > > > 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 75ffdc56d157..70a92783076f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -227,36 +227,89 @@ CameraConfiguration PipelineHandlerIPU3::streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) { - CameraConfiguration configs; IPU3CameraData *data = cameraData(camera); - StreamConfiguration config = {}; + CameraConfiguration configs; + std::vector<IPU3Stream *> availableStreams = { + &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 'output' format set to " << config.width << "x" - << config.height << "-0x" << std::hex << std::setfill('0') - << std::setw(8) << config.pixelFormat; - - configs[&data->vfStream_] = config; - LOG(IPU3, Debug) - << "Stream 'viewfinder' format set to " << config.width << "x" - << config.height << "-0x" << std::hex << std::setfill('0') - << std::setw(8) << config.pixelFormat; + for (const StreamUsage &usage : usages) { + std::vector<IPU3Stream *>::iterator found; + enum StreamUsage::Role r = usage.role(); + StreamConfiguration config = {}; + IPU3Stream *stream = nullptr; + + std::vector<IPU3Stream *>::iterator s = availableStreams.begin(); + if (r == StreamUsage::Role::StillCapture) { + for (; s < availableStreams.end(); ++s) { + /* + * We can use the viewfinder stream in case + * the 'StillCapture' usage is required + * multiple times. + */ + if (*s == &data->outStream_) { + stream = &data->outStream_; + found = s; + break; + } else { + stream = &data->vfStream_; + found = s; + } + } + + /* + * 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. + */ + config.width = 2560; + config.height = 1920; + } else if (r == StreamUsage::Role::Viewfinder || + r == StreamUsage::Role::VideoRecording) { + for (; s < availableStreams.end(); ++s) { + /* + * We can't use the 'output' stream for + * viewfinder or video capture usages. + */ + if (*s != &data->vfStream_) + continue; + + stream = &data->vfStream_; + found = s; + break; + } + + config.width = 640; + config.height = 480; + } + + if (stream == nullptr) + goto error; + + availableStreams.erase(found); + + config.pixelFormat = V4L2_PIX_FMT_NV12; + config.bufferCount = IPU3_BUFFER_COUNT; + + configs[stream] = config; + + LOG(IPU3, Debug) + << "Stream " << stream->name_ << " format set to " + << config.width << "x" << config.height + << "-0x" << std::hex << std::setfill('0') + << std::setw(8) << config.pixelFormat; + } return configs; + +error: + LOG(IPU3, Error) << "Requested stream roles not supported"; + return CameraConfiguration{}; } 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, as the viewfinder stream can optionally be used for capturing still images, but the main output stream cannot be used as viewfinder or for video recording purposes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++------- 1 file changed, 79 insertions(+), 26 deletions(-)