[libcamera-devel] [PATCH v2 2/6] libcamera: Use stream roles directly instead of StreamUsage
Jacopo Mondi
jacopo at jmondi.org
Tue May 21 12:12:06 CEST 2019
Hi Laurent,
On Sun, May 19, 2019 at 06:00:43PM +0300, Laurent Pinchart wrote:
> In order to prepare for an API overhall of the camera configuration
> generation, remove the StreamUsage class and replace its uses by stream
> roles. The size hints can't be specified anymore, and will be replaced
> with an API on the StreamConfiguration to negotiate configuration
> parameters with cameras.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/camera.h | 8 +--
> include/libcamera/stream.h | 44 ++----------
> src/cam/main.cpp | 13 ++--
> src/libcamera/camera.cpp | 16 ++---
> src/libcamera/include/pipeline_handler.h | 6 +-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++----
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +-
> src/libcamera/pipeline/uvcvideo.cpp | 5 +-
> src/libcamera/pipeline/vimc.cpp | 5 +-
> src/libcamera/pipeline_handler.cpp | 10 +--
> src/libcamera/stream.cpp | 89 ++++--------------------
> src/qcam/main_window.cpp | 2 +-
> test/camera/capture.cpp | 2 +-
> test/camera/configuration_default.cpp | 6 +-
> test/camera/configuration_set.cpp | 2 +-
> test/camera/statemachine.cpp | 2 +-
> 16 files changed, 69 insertions(+), 173 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 306739b7014a..42ba5201eabc 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -14,16 +14,13 @@
>
> #include <libcamera/request.h>
> #include <libcamera/signal.h>
> +#include <libcamera/stream.h>
>
> namespace libcamera {
>
> class Buffer;
> class PipelineHandler;
> class Request;
> -class Stream;
> -class StreamUsage;
I guess this would just need a StreamUsage/StreamRoles but I'm fine
with dropping forward declarations and include stream.h
> -
> -struct StreamConfiguration;
>
> class CameraConfiguration
> {
> @@ -74,8 +71,7 @@ public:
> int release();
>
> const std::set<Stream *> &streams() const;
> - CameraConfiguration
> - generateConfiguration(const std::vector<StreamUsage> &usage);
> + CameraConfiguration generateConfiguration(const StreamRoles &roles);
> int configure(const CameraConfiguration &config);
>
> int allocateBuffers();
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 0c986310939b..59bdf217eb31 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -8,6 +8,7 @@
> #define __LIBCAMERA_STREAM_H__
>
> #include <string>
> +#include <vector>
>
> #include <libcamera/buffer.h>
> #include <libcamera/geometry.h>
> @@ -25,48 +26,17 @@ struct StreamConfiguration {
> std::string toString() const;
> };
>
> -class StreamUsage
> -{
> -public:
> - enum Role {
> - StillCapture,
> - VideoRecording,
> - Viewfinder,
> - };
> -
> - Role role() const { return role_; }
> - const Size &size() const { return size_; }
> -
> -protected:
> - explicit StreamUsage(Role role);
> - StreamUsage(Role role, const Size &size);
> -
> -private:
> - Role role_;
> - Size size_;
> +enum StreamRole {
> + StillCapture,
> + VideoRecording,
> + Viewfinder,
> };
>
> +using StreamRoles = std::vector<StreamRole>;
> +
This is indeed nicer to write, it's true that for a user of it,
dealing with an std::vector directly would make it clear which API to
use without having to look at the type def here.
> class Stream
> {
> public:
> - class StillCapture : public StreamUsage
> - {
> - public:
> - StillCapture();
> - };
> -
> - class VideoRecording : public StreamUsage
> - {
> - public:
> - VideoRecording();
> - };
> -
> - class Viewfinder : public StreamUsage
> - {
> - public:
> - Viewfinder(int width, int height);
> - };
> -
> Stream();
> BufferPool &bufferPool() { return bufferPool_; }
> const StreamConfiguration &configuration() const { return configuration_; }
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 6a2508dd3bd9..d603228c0116 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -87,13 +87,13 @@ static int parseOptions(int argc, char *argv[])
>
> static int prepareCameraConfig(CameraConfiguration *config)
> {
> - std::vector<StreamUsage> roles;
> + StreamRoles roles;
>
> streamInfo.clear();
>
> /* If no configuration is provided assume a single video stream. */
> if (!options.isSet(OptStream)) {
> - *config = camera->generateConfiguration({ Stream::VideoRecording() });
> + *config = camera->generateConfiguration({ StreamRole::VideoRecording });
> streamInfo[config->front()] = "stream0";
> return 0;
> }
> @@ -106,14 +106,13 @@ static int prepareCameraConfig(CameraConfiguration *config)
> KeyValueParser::Options conf = value.toKeyValues();
>
> if (!conf.isSet("role")) {
> - roles.push_back(Stream::VideoRecording());
> + roles.push_back(StreamRole::VideoRecording);
> } else if (conf["role"].toString() == "viewfinder") {
> - roles.push_back(Stream::Viewfinder(conf["width"],
> - conf["height"]));
> + roles.push_back(StreamRole::Viewfinder);
> } else if (conf["role"].toString() == "video") {
> - roles.push_back(Stream::VideoRecording());
> + roles.push_back(StreamRole::VideoRecording);
> } else if (conf["role"].toString() == "still") {
> - roles.push_back(Stream::StillCapture());
> + roles.push_back(StreamRole::StillCapture);
> } else {
> std::cerr << "Unknown stream role "
> << conf["role"].toString() << std::endl;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 359174a41823..a3921f91f1c9 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -542,23 +542,23 @@ const std::set<Stream *> &Camera::streams() const
> }
>
> /**
> - * \brief Generate a default camera configuration according to stream usages
> - * \param[in] usages A list of stream usages
> + * \brief Generate a default camera configuration according to stream roles
> + * \param[in] roles A list of stream roles
> *
> - * Generate a camera configuration for a set of desired usages. The caller
> - * specifies a list of stream usages and the camera returns a configuration
> + * Generate a camera configuration for a set of desired stream roles. The caller
> + * specifies a list of stream roles and the camera returns a configuration
> * containing suitable streams and their suggested default configurations.
> *
> - * \return A valid CameraConfiguration if the requested usages can be satisfied,
> + * \return A valid CameraConfiguration if the requested roles can be satisfied,
> * or a invalid one otherwise
> */
> CameraConfiguration
> -Camera::generateConfiguration(const std::vector<StreamUsage> &usages)
> +Camera::generateConfiguration(const StreamRoles &roles)
> {
> - if (disconnected_ || !usages.size() || usages.size() > streams_.size())
> + if (disconnected_ || !roles.size() || roles.size() > streams_.size())
> return CameraConfiguration();
>
> - CameraConfiguration config = pipe_->generateConfiguration(this, usages);
> + CameraConfiguration config = pipe_->generateConfiguration(this, roles);
>
> std::ostringstream msg("streams configuration:", std::ios_base::ate);
> unsigned int index = 0;
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 9cc11a8e192e..3352cb0e5bc9 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -14,6 +14,8 @@
> #include <string>
> #include <vector>
>
> +#include <libcamera/stream.h>
> +
> namespace libcamera {
>
> class Buffer;
> @@ -26,8 +28,6 @@ class DeviceMatch;
> class MediaDevice;
> class PipelineHandler;
> class Request;
> -class Stream;
> -class StreamUsage;
Same comment as above. Or would you need to forward declare both StreamRoles
and StreamRole?
>
> class CameraData
> {
> @@ -61,7 +61,7 @@ public:
> void unlock();
>
> virtual CameraConfiguration
> - generateConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> + generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> virtual int configure(Camera *camera, const CameraConfiguration &config) = 0;
>
> virtual int allocateBuffers(Camera *camera,
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ba0c708f9e1e..d234a8ac5289 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -151,8 +151,7 @@ public:
> PipelineHandlerIPU3(CameraManager *manager);
>
> CameraConfiguration
> - generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages) override;
> + generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> int configure(Camera *camera,
> const CameraConfiguration &config) override;
>
> @@ -211,7 +210,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>
> CameraConfiguration
> PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages)
> + const StreamRoles &roles)
> {
> IPU3CameraData *data = cameraData(camera);
> CameraConfiguration config = {};
> @@ -220,13 +219,12 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> &data->vfStream_,
> };
>
> - for (const StreamUsage &usage : usages) {
> + for (const StreamRole role : roles) {
> StreamConfiguration cfg = {};
> - StreamUsage::Role role = usage.role();
> IPU3Stream *stream = nullptr;
>
> switch (role) {
> - case StreamUsage::Role::StillCapture:
> + case StreamRole::StillCapture:
> /*
> * Pick the output stream by default as the Viewfinder
> * and VideoRecording roles are not allowed on
> @@ -256,11 +254,11 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>
> break;
>
> - case StreamUsage::Role::Viewfinder:
> - case StreamUsage::Role::VideoRecording: {
> + case StreamRole::Viewfinder:
> + case StreamRole::VideoRecording: {
> /*
> * We can't use the 'output' stream for viewfinder or
> - * video capture usages.
> + * video capture roles.
> *
> * \todo This is an artificial limitation until we
> * figure out the exact capabilities of the hardware.
> @@ -275,15 +273,13 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> stream = &data->vfStream_;
>
> /*
> - * Align the requested viewfinder size to the
> - * maximum available sensor resolution and to the
> - * IPU3 alignment constraints.
> + * 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(usage.size().width,
> - res.width);
> - unsigned int height = std::min(usage.size().height,
> - res.height);
> + unsigned int width = std::min(1280U, res.width);
> + unsigned int height = std::min(720U, res.height);
Why 1280x720 ?
> cfg.size = { width & ~7, height & ~3 };
>
> break;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4d02f9604ad9..4bd8c5101a96 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -35,7 +35,7 @@ public:
> ~PipelineHandlerRkISP1();
>
> CameraConfiguration generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages) override;
> + const StreamRoles &roles) override;
> int configure(Camera *camera,
> const CameraConfiguration &config) override;
>
> @@ -107,7 +107,7 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> */
>
> CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages)
> + const StreamRoles &roles)
> {
> RkISP1CameraData *data = cameraData(camera);
> CameraConfiguration config;
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 118b97457d2a..d2e1f7d4e5b2 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -26,8 +26,7 @@ public:
> PipelineHandlerUVC(CameraManager *manager);
>
> CameraConfiguration
> - generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages) override;
> + generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> int configure(Camera *camera,
> const CameraConfiguration &config) override;
>
> @@ -77,7 +76,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>
> CameraConfiguration
> PipelineHandlerUVC::generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages)
> + const StreamRoles &roles)
> {
> UVCCameraData *data = cameraData(camera);
> CameraConfiguration config;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 74959581a7ef..17e2491e5c27 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -26,8 +26,7 @@ public:
> PipelineHandlerVimc(CameraManager *manager);
>
> CameraConfiguration
> - generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages) override;
> + generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> int configure(Camera *camera,
> const CameraConfiguration &config) override;
>
> @@ -77,7 +76,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>
> CameraConfiguration
> PipelineHandlerVimc::generateConfiguration(Camera *camera,
> - const std::vector<StreamUsage> &usages)
> + const StreamRoles &roles)
> {
> VimcCameraData *data = cameraData(camera);
> CameraConfiguration config;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index b9ac64328f1d..81c11149c9fe 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -221,18 +221,18 @@ void PipelineHandler::unlock()
> * \fn PipelineHandler::generateConfiguration()
> * \brief Generate a camera configuration for a specified camera
> * \param[in] camera The camera to generate a default configuration for
> - * \param[in] usages A list of stream usages
> + * \param[in] roles A list of stream roles
> *
> - * Generate a default configuration for the \a camera for a specified group of
> - * use-cases. The caller shall populate the \a usages array with the use-cases
> - * it wishes to fetch the default configuration for. The returned configuration
> + * Generate a default configuration for the \a camera for a specified list of
> + * stream roles. The caller shall populate the \a roles with the use-cases it
> + * wishes to fetch the default configuration for. The returned configuration
> * can then be examined by the caller to learn about the selected streams and
> * their default parameters.
> *
> * The intended companion to this is \a configure() which can be used to change
> * the group of streams parameters.
> *
> - * \return A valid CameraConfiguration if the requested usages can be satisfied,
> + * \return A valid CameraConfiguration if the requested roles can be satisfied,
> * or a invalid configuration otherwise
> */
>
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index af259510b19c..fe4c4ecf4150 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -75,61 +75,31 @@ std::string StreamConfiguration::toString() const
> }
>
> /**
> - * \class StreamUsage
> - * \brief Stream usage information
> - *
> - * The StreamUsage class describes how an application intends to use a stream.
> - * Usages are specified by applications and passed to cameras, that then select
> - * the most appropriate streams and their default configurations.
> - */
> -
> -/**
> - * \enum StreamUsage::Role
> + * \enum StreamRole
> * \brief Identify the role a stream is intended to play
> - * \var StreamUsage::StillCapture
> + *
> + * The StreamRole describes how an application intends to use a stream. Roles
> + * are specified by applications and passed to cameras, that then select the
s/select/selects ?
Minors apart, and the resolution selected on IPU3 clarified:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> + * most appropriate streams and their default configurations.
> + *
> + * \var StillCapture
> * The stream is intended to capture high-resolution, high-quality still images
> * with low frame rate. The captured frames may be exposed with flash.
> - * \var StreamUsage::VideoRecording
> + * \var VideoRecording
> * The stream is intended to capture video for the purpose of recording or
> * streaming. The video stream may produce a high frame rate and may be
> * enhanced with video stabilization.
> - * \var StreamUsage::Viewfinder
> + * \var Viewfinder
> * The stream is intended to capture video for the purpose of display on the
> - * local screen. The StreamUsage includes the desired resolution. Trade-offs
> - * between quality and usage of system resources are acceptable.
> + * local screen. Trade-offs between quality and usage of system resources are
> + * acceptable.
> */
>
> /**
> - * \fn StreamUsage::role()
> - * \brief Retrieve the stream role
> - * \return The stream role
> + * \typedef StreamRoles
> + * \brief A vector of StreamRole
> */
>
> -/**
> - * \fn StreamUsage::size()
> - * \brief Retrieve desired size
> - * \return The desired size
> - */
> -
> -/**
> - * \brief Create a stream usage
> - * \param[in] role Stream role
> - */
> -StreamUsage::StreamUsage(Role role)
> - : role_(role)
> -{
> -}
> -
> -/**
> - * \brief Create a stream usage with a desired size
> - * \param[in] role Stream role
> - * \param[in] size The desired size
> - */
> -StreamUsage::StreamUsage(Role role, const Size &size)
> - : role_(role), size_(size)
> -{
> -}
> -
> /**
> * \class Stream
> * \brief Video stream for a camera
> @@ -148,39 +118,6 @@ StreamUsage::StreamUsage(Role role, const Size &size)
> * optimal stream for the task.
> */
>
> -/**
> - * \class Stream::StillCapture
> - * \brief Describe a still capture usage
> - */
> -Stream::StillCapture::StillCapture()
> - : StreamUsage(Role::StillCapture)
> -{
> -}
> -
> -/**
> - * \class Stream::VideoRecording
> - * \brief Describe a video recording usage
> - */
> -Stream::VideoRecording::VideoRecording()
> - : StreamUsage(Role::VideoRecording)
> -{
> -}
> -
> -/**
> - * \class Stream::Viewfinder
> - * \brief Describe a viewfinder usage
> - */
> -
> -/**
> - * \brief Create a viewfinder usage with a desired dimension
> - * \param[in] width The desired viewfinder width
> - * \param[in] height The desired viewfinder height
> - */
> -Stream::Viewfinder::Viewfinder(int width, int height)
> - : StreamUsage(Role::Viewfinder, Size(width, height))
> -{
> -}
> -
> /**
> * \brief Construct a stream with default parameters
> */
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index c91b82727ec6..a984aaca764f 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -97,7 +97,7 @@ int MainWindow::startCapture()
> {
> int ret;
>
> - config_ = camera_->generateConfiguration({ Stream::VideoRecording() });
> + config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> Stream *stream = config_.front();
> ret = camera_->configure(config_);
> if (ret < 0) {
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index bc3a4d6cb9f2..e7e6438203b9 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -43,7 +43,7 @@ protected:
> int run()
> {
> CameraConfiguration config =
> - camera_->generateConfiguration({ Stream::VideoRecording() });
> + camera_->generateConfiguration({ StreamRole::VideoRecording });
> Stream *stream = config.front();
> StreamConfiguration *cfg = &config[stream];
>
> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> index 340b5f58f04c..8c4a03db498a 100644
> --- a/test/camera/configuration_default.cpp
> +++ b/test/camera/configuration_default.cpp
> @@ -21,7 +21,7 @@ protected:
> CameraConfiguration config;
>
> /* Test asking for configuration for a video stream. */
> - config = camera_->generateConfiguration({ Stream::VideoRecording() });
> + config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> if (!config.isValid()) {
> cout << "Default configuration invalid" << endl;
> return TestFail;
> @@ -29,11 +29,11 @@ protected:
>
> /*
> * Test that asking for configuration for an empty array of
> - * stream usages returns an empty list of configurations.
> + * stream roles returns an empty list of configurations.
> */
> config = camera_->generateConfiguration({});
> if (config.isValid()) {
> - cout << "Failed to retrieve configuration for empty usage list"
> + cout << "Failed to retrieve configuration for empty roles list"
> << endl;
> return TestFail;
> }
> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> index 24d5ca6690b7..76d8bc3e40a4 100644
> --- a/test/camera/configuration_set.cpp
> +++ b/test/camera/configuration_set.cpp
> @@ -19,7 +19,7 @@ protected:
> int run()
> {
> CameraConfiguration config =
> - camera_->generateConfiguration({ Stream::VideoRecording() });
> + camera_->generateConfiguration({ StreamRole::VideoRecording });
> StreamConfiguration *cfg = &config[config.front()];
>
> if (!config.isValid()) {
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index bd2e61ff2939..7a74cd85a37a 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -235,7 +235,7 @@ protected:
>
> int run()
> {
> - defconf_ = camera_->generateConfiguration({ Stream::VideoRecording() });
> + defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
>
> if (testAvailable() != TestPass) {
> cout << "State machine in Available state failed" << endl;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190521/8ecdfab1/attachment-0001.sig>
More information about the libcamera-devel
mailing list