| Message ID | 20190125153340.2744-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Fri, Jan 25, 2019 at 04:33:35PM +0100, Niklas Söderlund wrote: > Add a simple StreamConfiguration class to hold configuration data for a > single stream of a Camera. In its current form not many configuration > parameters are supported but it's expected the number of options will > grow over time. > > At this stage the pixel format is represented as a unsigned int to allow s/as a/as an/ > for a easy mapping to the V4L2 API. This might be subject to change in s/a easy/an easy/ (or just s/a easy/easy/) > the future as we finalize how libcamera shall represent pixelformats. s/pixelformats/pixel formats/ > A StreamConfiguration objected needs to be created from the Stream > object it should configure. As the two objects are so closely related I > have at this stage opted to implement them in the same stream.{h,cpp} as > the Stream class. Why does the StreamConfiguration object need to be created with the Stream passed as an argument to the constructor ? > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/stream.h | 20 ++++++++++++ > src/libcamera/stream.cpp | 64 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 415815ba12c65e47..8750797c36dd9b42 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -20,6 +20,26 @@ private: > unsigned int id_; > }; > > +class StreamConfiguration final > +{ > +public: > + StreamConfiguration(class Stream &stream); s/class // > + > + unsigned int id() const { return id_; }; Given that the stream is given to the constructor, should we store the stream reference instead of the id ? > + unsigned int width() const { return width_; }; > + unsigned int height() const { return height_; }; > + unsigned int pixelformat() const { return pixelformat_; }; Open question, pixelformat or pixelFormat ? > + > + void setDimension(unsigned int width, unsigned int height); I'd name this setSize(). I think we'll need a class to represent sizes and rectangles at some point, but that can be done later. > + void setPixelFormat(unsigned int pixelformat); > + > +private: > + unsigned int id_; > + unsigned int width_; > + unsigned int height_; > + unsigned int pixelformat_; With accessors for width_, height_ and pixelformat_ that expose the fields directly, how about just making those three fields public ? The StreamConfiguration is more of a data structure that groups stream parameters without much associated processing than a real object with methods. > +}; > + > } /* namespace libcamera */ > > #endif /* __LIBCAMERA_STREAM_H__ */ > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index 3b44e834ee02b35a..e40756260c5768f3 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -63,4 +63,68 @@ Stream::Stream(unsigned int id) > * \return The stream ID > */ > > +/** > + * \class StreamConfiguration > + * \brief Stream configuration object Maybe "Configuration parameters for a stream" ? > + * > + * The StreamConfiguration class is a model of all information which can be > + * configured for a single video stream. A application should acquire a all > + * Stream object from a camera, select the ones it wish to use, create a > + * StreamConfiguration for each of those streams, set the desired parameter on > + * the objects and feed them back to the camera object to configure the camera. I think we should just describe here the StreamConfiguration class, and explain how it interacts with Stream and Camera in the Camera documentation. > + */ > + > +/** > + * \fn StreamConfiguration::id() > + * \brief Retrieve the streams ID s/streams/stream/ > + * \return The stream ID > + */ > + > +/** > + * \fn StreamConfiguration::width() > + * \brief Retrieve the Stream width s/Stream/stream/ We should describe what the stream width (and height and pixel format below) is. > + * \return The stream width > + */ > + > +/** > + * \fn StreamConfiguration::height() > + * \brief Retrieve the Stream height Same here. > + * \return The stream height > + */ > + > +/** > + * \fn StreamConfiguration::pixelformat() > + * \brief Retrieve the Stream pixelformat And here too. > + * \return The stream pixelformat > + */ > + > +/** > + * \brief Set desired width and height for the stream > + * \param[in] width The desired width of the stream > + * \param[in] height The desired height of the stream > + */ > +void StreamConfiguration::setDimension(unsigned int width, unsigned int height) > +{ > + width_ = width; > + height_ = height; > +} > + > +/** > + * \brief Set desired pixelformat for the stream > + * \param[in] pixelformat The desired pixelformat of the stream > + */ > +void StreamConfiguration::setPixelFormat(unsigned int pixelformat) > +{ > + pixelformat_ = pixelformat; > +} > + > +/** > + * \brief Create a new configuration container for a stream > + * \param[in] stream The stream object the configuration object should act on > + */ > +StreamConfiguration::StreamConfiguration(class Stream &stream) > + : id_(stream.id()), width_(0), height_(0), pixelformat_(0) > +{ > +} Please order functions as in the class definition. > + > } /* namespace libcamera */
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 415815ba12c65e47..8750797c36dd9b42 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -20,6 +20,26 @@ private: unsigned int id_; }; +class StreamConfiguration final +{ +public: + StreamConfiguration(class Stream &stream); + + unsigned int id() const { return id_; }; + unsigned int width() const { return width_; }; + unsigned int height() const { return height_; }; + unsigned int pixelformat() const { return pixelformat_; }; + + void setDimension(unsigned int width, unsigned int height); + void setPixelFormat(unsigned int pixelformat); + +private: + unsigned int id_; + unsigned int width_; + unsigned int height_; + unsigned int pixelformat_; +}; + } /* namespace libcamera */ #endif /* __LIBCAMERA_STREAM_H__ */ diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index 3b44e834ee02b35a..e40756260c5768f3 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -63,4 +63,68 @@ Stream::Stream(unsigned int id) * \return The stream ID */ +/** + * \class StreamConfiguration + * \brief Stream configuration object + * + * The StreamConfiguration class is a model of all information which can be + * configured for a single video stream. A application should acquire a all + * Stream object from a camera, select the ones it wish to use, create a + * StreamConfiguration for each of those streams, set the desired parameter on + * the objects and feed them back to the camera object to configure the camera. + */ + +/** + * \fn StreamConfiguration::id() + * \brief Retrieve the streams ID + * \return The stream ID + */ + +/** + * \fn StreamConfiguration::width() + * \brief Retrieve the Stream width + * \return The stream width + */ + +/** + * \fn StreamConfiguration::height() + * \brief Retrieve the Stream height + * \return The stream height + */ + +/** + * \fn StreamConfiguration::pixelformat() + * \brief Retrieve the Stream pixelformat + * \return The stream pixelformat + */ + +/** + * \brief Set desired width and height for the stream + * \param[in] width The desired width of the stream + * \param[in] height The desired height of the stream + */ +void StreamConfiguration::setDimension(unsigned int width, unsigned int height) +{ + width_ = width; + height_ = height; +} + +/** + * \brief Set desired pixelformat for the stream + * \param[in] pixelformat The desired pixelformat of the stream + */ +void StreamConfiguration::setPixelFormat(unsigned int pixelformat) +{ + pixelformat_ = pixelformat; +} + +/** + * \brief Create a new configuration container for a stream + * \param[in] stream The stream object the configuration object should act on + */ +StreamConfiguration::StreamConfiguration(class Stream &stream) + : id_(stream.id()), width_(0), height_(0), pixelformat_(0) +{ +} + } /* namespace libcamera */
Add a simple StreamConfiguration class to hold configuration data for a single stream of a Camera. In its current form not many configuration parameters are supported but it's expected the number of options will grow over time. At this stage the pixel format is represented as a unsigned int to allow for a easy mapping to the V4L2 API. This might be subject to change in the future as we finalize how libcamera shall represent pixelformats. A StreamConfiguration objected needs to be created from the Stream object it should configure. As the two objects are so closely related I have at this stage opted to implement them in the same stream.{h,cpp} as the Stream class. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/stream.h | 20 ++++++++++++ src/libcamera/stream.cpp | 64 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+)