| Message ID | 20190403011221.12711-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote: > In preparation of reworking how a default configuration is retrieved > from a camera add stream roles. The roles will be used by applications > to describe how it intends to use a camera and replace the Stream IDs s/it intends/they intend/ > role when retrieving default configuration from the camera using > streamConfiguration(). > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/stream.h | 41 ++++++++++++++++++ > src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 129 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 970c479627fab064..adcf20d336347dad 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -21,9 +21,50 @@ struct StreamConfiguration { > unsigned int bufferCount; > }; > > +class StreamRole > +{ > +public: > + enum Role { > + StillCapture, > + VideoRecording, > + Viewfinder, > + }; > + > + Role role() const { return role_; } > + int width() const { return width_; } > + int height() const { return height_; } > + > +protected: > + StreamRole(Role role); > + StreamRole(Role role, int width, int height); > + > +private: > + Role role_; > + int width_; > + int height_; How about using the new Size structure ? > +}; > + > class Stream final > { > public: > + class StillCapture : public StreamRole > + { > + public: > + StillCapture(); > + }; > + > + class VideoRecording : public StreamRole > + { > + public: > + VideoRecording(); > + }; > + > + class Viewfinder : public StreamRole > + { > + public: > + Viewfinder(int width, int height); > + }; > + > Stream(); > BufferPool &bufferPool() { return bufferPool_; } > const StreamConfiguration &configuration() const { return configuration_; } > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index c4943c91b2e6ce13..f4be5d265e872842 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -60,6 +60,61 @@ namespace libcamera { > * \brief Requested number of buffers to allocate for the stream > */ > > +/** > + * \class StreamRole > + * \brief Stream role information > + * > + * The StreamRole class carries information about stream usage hints from the > + * application to the camera. The camera shall take the usage hints into account > + * when select which stream to use for the desired operation. I'd like to drop the word "hint" from most of the documentation. How about The StreamRole class describes how a stream is intended to be used. Stream roles are specified by applications and passed to cameras, that then select the most appropriate streams and their default configurations. > + */ > + > +/** > + * \enum StreamRole::Role > + * \brief List of different stream roles Using the word role to name both the class and the enum makes documentation a bit more difficult to write. You also need to document the enum values by the way. How about something like the following ? /** * \enum StreamRole::Role * Identify the role a stream is intended to play * \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 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 Viewfinder * The stream is intended to capture video for the purpose of display on * the local screen. The StreamRole includes the desired resolution. * Trade-offs between quality and usage of system resources are * acceptable. */ > + */ > + > +/** > + * \fn StreamRole::role() > + * \brief Retrieve the stream role > + * > + * \return The stream role hint s/ hint// > + */ > + > +/** > + * \fn StreamRole::width() > + * \brief Retrieve desired width > + * > + * \return The desired width if defined, -1 otherwise > + */ > + > +/** > + * \fn StreamRole::height() > + * \brief Retrieve desired height > + * > + * \return The desired height if defined, -1 otherwise > + */ After merging Jacopo's ImgU patch series I think we could use the Size class, and modify it to use -1 as a marker of invalid dimensions. > + > +/** > + * \brief Create a stream role > + * \param[in] role Stream role hint I would like to drop the word hint, but "Stream role" sounds weird. I wonder if we should rename the StreamRole class to StreamUsage in order to have two different words, usage and role. Feel free to propose an alternative for "hint" that wouldn't require such a rename :-) > + */ > +StreamRole::StreamRole(Role role) > + : role_(role), width_(-1), height_(-1) > +{ > +} > + > +/** > + * \brief Create a stream role with dimension hints > + * \param[in] role Stream role hint > + * \param[in] width Desired width > + * \param[in] height Desired height > + */ > +StreamRole::StreamRole(Role role, int width, int height) > + : role_(role), width_(width), height_(height) > +{ > +} > + > /** > * \class Stream > * \brief Video stream for a camera > @@ -78,6 +133,39 @@ namespace libcamera { > * optimal stream for the task. > */ > > +/** > + * \class Stream::StillCapture > + * \brief Describes a still capture role > + */ > +Stream::StillCapture::StillCapture() > + : StreamRole(Role::StillCapture) > +{ > +} > + > +/** > + * \class Stream::VideoRecording > + * \brief Describes a video recording role > + */ > +Stream::VideoRecording::VideoRecording() > + : StreamRole(Role::VideoRecording) > +{ > +} > + > +/** > + * \class Stream::Viewfinder > + * \brief Describes a viewfinder role > + */ > + > +/** > + * \brief Create a viewfinder role with dimension hints > + * \param[in] width Desired viewfinder width > + * \param[in] height Desired viewfinder height > + */ > +Stream::Viewfinder::Viewfinder(int width, int height) > + : StreamRole(Role::Viewfinder, width, height) > +{ > +} > + > /** > * \brief Construct a stream with default parameters > */
HI Niklas, On Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote: > > In preparation of reworking how a default configuration is retrieved > > from a camera add stream roles. The roles will be used by applications > > to describe how it intends to use a camera and replace the Stream IDs > > s/it intends/they intend/ > > > role when retrieving default configuration from the camera using s/Stream IDs role/Stream IDs/ ? > > streamConfiguration(). > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/stream.h | 41 ++++++++++++++++++ > > src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 129 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 970c479627fab064..adcf20d336347dad 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -21,9 +21,50 @@ struct StreamConfiguration { > > unsigned int bufferCount; > > }; > > > > +class StreamRole > > +{ > > +public: > > + enum Role { > > + StillCapture, > > + VideoRecording, > > + Viewfinder, > > + }; > > + > > + Role role() const { return role_; } > > + int width() const { return width_; } > > + int height() const { return height_; } > > + > > +protected: > > + StreamRole(Role role); This is protected so it might not matter, but constructors with 1 parameteres should be marked explicit. > > + StreamRole(Role role, int width, int height); > > + > > +private: > > + Role role_; > > + int width_; > > + int height_; > > How about using the new Size structure ? > > > +}; > > + > > class Stream final > > { > > public: > > + class StillCapture : public StreamRole > > + { > > + public: > > + StillCapture(); > > + }; > > + > > + class VideoRecording : public StreamRole > > + { > > + public: > > + VideoRecording(); > > + }; > > + > > + class Viewfinder : public StreamRole > > + { > > + public: > > + Viewfinder(int width, int height); > > + }; > > + > > Stream(); > > BufferPool &bufferPool() { return bufferPool_; } > > const StreamConfiguration &configuration() const { return configuration_; } > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index c4943c91b2e6ce13..f4be5d265e872842 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -60,6 +60,61 @@ namespace libcamera { > > * \brief Requested number of buffers to allocate for the stream > > */ > > > > +/** > > + * \class StreamRole > > + * \brief Stream role information > > + * > > + * The StreamRole class carries information about stream usage hints from the > > + * application to the camera. The camera shall take the usage hints into account > > + * when select which stream to use for the desired operation. > > I'd like to drop the word "hint" from most of the documentation. How > about > > The StreamRole class describes how a stream is intended to be used. or "how an application intends to use a stream" > Stream roles are specified by applications and passed to cameras, that > then select the most appropriate streams and their default > configurations. > > > + */ > > + > > +/** > > + * \enum StreamRole::Role > > + * \brief List of different stream roles > > Using the word role to name both the class and the enum makes > documentation a bit more difficult to write. You also need to document > the enum values by the way. How about something like the following ? > > /** > * \enum StreamRole::Role > * Identify the role a stream is intended to play We are using both third and first person verbs in the documentation. Ie. "Identify" here, "Describes" below. Also, \brief ? > * \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 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 Viewfinder > * The stream is intended to capture video for the purpose of display on > * the local screen. The StreamRole includes the desired resolution. s/The/This ? > * Trade-offs between quality and usage of system resources are > * acceptable. > */ > > > + */ > > + > > +/** > > + * \fn StreamRole::role() > > + * \brief Retrieve the stream role > > + * > > + * \return The stream role hint > > s/ hint// > > > + */ > > + > > +/** > > + * \fn StreamRole::width() > > + * \brief Retrieve desired width the desired? I would s/desired/requested > > + * > > + * \return The desired width if defined, -1 otherwise ", -1 otherwise" seems like an error condition, while it is the default value of an uninitialized width. > > + */ > > + > > +/** > > + * \fn StreamRole::height() > > + * \brief Retrieve desired height > > + * > > + * \return The desired height if defined, -1 otherwise > > + */ > > After merging Jacopo's ImgU patch series I think we could use the Size > class, and modify it to use -1 as a marker of invalid dimensions. > Also, yes. Anway, 0 should not be a valid size, and should be used as default, isn't it ? > > + > > +/** > > + * \brief Create a stream role > > + * \param[in] role Stream role hint > > I would like to drop the word hint, but "Stream role" sounds weird. I > wonder if we should rename the StreamRole class to StreamUsage in order > to have two different words, usage and role. Feel free to propose an > alternative for "hint" that wouldn't require such a rename :-) +1 for StreamUsage Thanks j > > > + */ > > +StreamRole::StreamRole(Role role) > > + : role_(role), width_(-1), height_(-1) > > +{ > > +} > > + > > +/** > > + * \brief Create a stream role with dimension hints > > + * \param[in] role Stream role hint > > + * \param[in] width Desired width > > + * \param[in] height Desired height > > + */ > > +StreamRole::StreamRole(Role role, int width, int height) > > + : role_(role), width_(width), height_(height) > > +{ > > +} > > + > > /** > > * \class Stream > > * \brief Video stream for a camera > > @@ -78,6 +133,39 @@ namespace libcamera { > > * optimal stream for the task. > > */ > > > > +/** > > + * \class Stream::StillCapture > > + * \brief Describes a still capture role > > + */ > > +Stream::StillCapture::StillCapture() > > + : StreamRole(Role::StillCapture) > > +{ > > +} > > + > > +/** > > + * \class Stream::VideoRecording > > + * \brief Describes a video recording role > > + */ > > +Stream::VideoRecording::VideoRecording() > > + : StreamRole(Role::VideoRecording) > > +{ > > +} > > + > > +/** > > + * \class Stream::Viewfinder > > + * \brief Describes a viewfinder role > > + */ > > + > > +/** > > + * \brief Create a viewfinder role with dimension hints > > + * \param[in] width Desired viewfinder width > > + * \param[in] height Desired viewfinder height > > + */ > > +Stream::Viewfinder::Viewfinder(int width, int height) > > + : StreamRole(Role::Viewfinder, width, height) > > +{ > > +} > > + > > /** > > * \brief Construct a stream with default parameters > > */ > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Wed, Apr 03, 2019 at 03:06:14PM +0200, Jacopo Mondi wrote: > On Wed, Apr 03, 2019 at 09:56:52AM +0300, Laurent Pinchart wrote: > > On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote: > >> In preparation of reworking how a default configuration is retrieved > >> from a camera add stream roles. The roles will be used by applications > >> to describe how it intends to use a camera and replace the Stream IDs > > > > s/it intends/they intend/ > > > >> role when retrieving default configuration from the camera using > > s/Stream IDs role/Stream IDs/ ? > > >> streamConfiguration(). > >> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> include/libcamera/stream.h | 41 ++++++++++++++++++ > >> src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 129 insertions(+) > >> > >> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > >> index 970c479627fab064..adcf20d336347dad 100644 > >> --- a/include/libcamera/stream.h > >> +++ b/include/libcamera/stream.h > >> @@ -21,9 +21,50 @@ struct StreamConfiguration { > >> unsigned int bufferCount; > >> }; > >> > >> +class StreamRole > >> +{ > >> +public: > >> + enum Role { > >> + StillCapture, > >> + VideoRecording, > >> + Viewfinder, > >> + }; > >> + > >> + Role role() const { return role_; } > >> + int width() const { return width_; } > >> + int height() const { return height_; } > >> + > >> +protected: > >> + StreamRole(Role role); > > This is protected so it might not matter, but constructors with 1 > parameteres should be marked explicit. Good point. > >> + StreamRole(Role role, int width, int height); > >> + > >> +private: > >> + Role role_; > >> + int width_; > >> + int height_; > > > > How about using the new Size structure ? > > > >> +}; > >> + > >> class Stream final > >> { > >> public: > >> + class StillCapture : public StreamRole > >> + { > >> + public: > >> + StillCapture(); > >> + }; > >> + > >> + class VideoRecording : public StreamRole > >> + { > >> + public: > >> + VideoRecording(); > >> + }; > >> + > >> + class Viewfinder : public StreamRole > >> + { > >> + public: > >> + Viewfinder(int width, int height); > >> + }; > >> + > >> Stream(); > >> BufferPool &bufferPool() { return bufferPool_; } > >> const StreamConfiguration &configuration() const { return configuration_; } > >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > >> index c4943c91b2e6ce13..f4be5d265e872842 100644 > >> --- a/src/libcamera/stream.cpp > >> +++ b/src/libcamera/stream.cpp > >> @@ -60,6 +60,61 @@ namespace libcamera { > >> * \brief Requested number of buffers to allocate for the stream > >> */ > >> > >> +/** > >> + * \class StreamRole > >> + * \brief Stream role information > >> + * > >> + * The StreamRole class carries information about stream usage hints from the > >> + * application to the camera. The camera shall take the usage hints into account > >> + * when select which stream to use for the desired operation. > > > > I'd like to drop the word "hint" from most of the documentation. How > > about > > > > The StreamRole class describes how a stream is intended to be used. > > or "how an application intends to use a stream" That's better indeed. The active form usually feels lighter to read. > > Stream roles are specified by applications and passed to cameras, that > > then select the most appropriate streams and their default > > configurations. > > > >> + */ > >> + > >> +/** > >> + * \enum StreamRole::Role > >> + * \brief List of different stream roles > > > > Using the word role to name both the class and the enum makes > > documentation a bit more difficult to write. You also need to document > > the enum values by the way. How about something like the following ? > > > > /** > > * \enum StreamRole::Role > > * Identify the role a stream is intended to play > > We are using both third and first person verbs in the documentation. > Ie. "Identify" here, "Describes" below. I think the library prefers the first person, so let's fix it below. > > Also, \brief ? > > > * \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 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 Viewfinder > > * The stream is intended to capture video for the purpose of display on > > * the local screen. The StreamRole includes the desired resolution. > > s/The/This ? I mean that for the Viewfinder role, the StreamRole instance includes the desired resolution. > > * Trade-offs between quality and usage of system resources are > > * acceptable. > > */ > > > >> + */ > >> + > >> +/** > >> + * \fn StreamRole::role() > >> + * \brief Retrieve the stream role > >> + * > >> + * \return The stream role hint > > > > s/ hint// > > > >> + */ > >> + > >> +/** > >> + * \fn StreamRole::width() > >> + * \brief Retrieve desired width > > the desired? > I would s/desired/requested > > >> + * > >> + * \return The desired width if defined, -1 otherwise > > ", -1 otherwise" seems like an error condition, while it is the > default value of an uninitialized width. > > >> + */ > >> + > >> +/** > >> + * \fn StreamRole::height() > >> + * \brief Retrieve desired height > >> + * > >> + * \return The desired height if defined, -1 otherwise > >> + */ > > > > After merging Jacopo's ImgU patch series I think we could use the Size > > class, and modify it to use -1 as a marker of invalid dimensions. > > Also, yes. Anway, 0 should not be a valid size, and should be used as > default, isn't it ? I would go for -1 to mark an invalid size in general. For an image 0 isn't valid either, but for some other usages it may. 0x0 marks an empty sizes, while -1x-1 would mark an invalid size. > >> + > >> +/** > >> + * \brief Create a stream role > >> + * \param[in] role Stream role hint > > > > I would like to drop the word hint, but "Stream role" sounds weird. I > > wonder if we should rename the StreamRole class to StreamUsage in order > > to have two different words, usage and role. Feel free to propose an > > alternative for "hint" that wouldn't require such a rename :-) > > +1 for StreamUsage > > >> + */ > >> +StreamRole::StreamRole(Role role) > >> + : role_(role), width_(-1), height_(-1) > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Create a stream role with dimension hints > >> + * \param[in] role Stream role hint > >> + * \param[in] width Desired width > >> + * \param[in] height Desired height > >> + */ > >> +StreamRole::StreamRole(Role role, int width, int height) > >> + : role_(role), width_(width), height_(height) > >> +{ > >> +} > >> + > >> /** > >> * \class Stream > >> * \brief Video stream for a camera > >> @@ -78,6 +133,39 @@ namespace libcamera { > >> * optimal stream for the task. > >> */ > >> > >> +/** > >> + * \class Stream::StillCapture > >> + * \brief Describes a still capture role > >> + */ > >> +Stream::StillCapture::StillCapture() > >> + : StreamRole(Role::StillCapture) > >> +{ > >> +} > >> + > >> +/** > >> + * \class Stream::VideoRecording > >> + * \brief Describes a video recording role > >> + */ > >> +Stream::VideoRecording::VideoRecording() > >> + : StreamRole(Role::VideoRecording) > >> +{ > >> +} > >> + > >> +/** > >> + * \class Stream::Viewfinder > >> + * \brief Describes a viewfinder role > >> + */ > >> + > >> +/** > >> + * \brief Create a viewfinder role with dimension hints > >> + * \param[in] width Desired viewfinder width > >> + * \param[in] height Desired viewfinder height > >> + */ > >> +Stream::Viewfinder::Viewfinder(int width, int height) > >> + : StreamRole(Role::Viewfinder, width, height) > >> +{ > >> +} > >> + > >> /** > >> * \brief Construct a stream with default parameters > >> */
Hi Laurent, Thanks for your feedback. On 2019-04-03 09:56:52 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Wed, Apr 03, 2019 at 03:12:20AM +0200, Niklas Söderlund wrote: > > In preparation of reworking how a default configuration is retrieved > > from a camera add stream roles. The roles will be used by applications > > to describe how it intends to use a camera and replace the Stream IDs > > s/it intends/they intend/ > > > role when retrieving default configuration from the camera using > > streamConfiguration(). > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/stream.h | 41 ++++++++++++++++++ > > src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 129 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 970c479627fab064..adcf20d336347dad 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -21,9 +21,50 @@ struct StreamConfiguration { > > unsigned int bufferCount; > > }; > > > > +class StreamRole > > +{ > > +public: > > + enum Role { > > + StillCapture, > > + VideoRecording, > > + Viewfinder, > > + }; > > + > > + Role role() const { return role_; } > > + int width() const { return width_; } > > + int height() const { return height_; } > > + > > +protected: > > + StreamRole(Role role); > > + StreamRole(Role role, int width, int height); > > + > > +private: > > + Role role_; > > + int width_; > > + int height_; > > How about using the new Size structure ? That is a possibility. Unfortunately the new Size structure is a private datatype not exposed to applications. This might change in the future but for now I will keep it as is. > > > +}; > > + > > class Stream final > > { > > public: > > + class StillCapture : public StreamRole > > + { > > + public: > > + StillCapture(); > > + }; > > + > > + class VideoRecording : public StreamRole > > + { > > + public: > > + VideoRecording(); > > + }; > > + > > + class Viewfinder : public StreamRole > > + { > > + public: > > + Viewfinder(int width, int height); > > + }; > > + > > Stream(); > > BufferPool &bufferPool() { return bufferPool_; } > > const StreamConfiguration &configuration() const { return configuration_; } > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index c4943c91b2e6ce13..f4be5d265e872842 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -60,6 +60,61 @@ namespace libcamera { > > * \brief Requested number of buffers to allocate for the stream > > */ > > > > +/** > > + * \class StreamRole > > + * \brief Stream role information > > + * > > + * The StreamRole class carries information about stream usage hints from the > > + * application to the camera. The camera shall take the usage hints into account > > + * when select which stream to use for the desired operation. > > I'd like to drop the word "hint" from most of the documentation. How > about > > The StreamRole class describes how a stream is intended to be used. > Stream roles are specified by applications and passed to cameras, that > then select the most appropriate streams and their default > configurations. > > > + */ > > + > > +/** > > + * \enum StreamRole::Role > > + * \brief List of different stream roles > > Using the word role to name both the class and the enum makes > documentation a bit more difficult to write. You also need to document > the enum values by the way. How about something like the following ? > > /** > * \enum StreamRole::Role > * Identify the role a stream is intended to play > * \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 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 Viewfinder > * The stream is intended to capture video for the purpose of display on > * the local screen. The StreamRole includes the desired resolution. > * Trade-offs between quality and usage of system resources are > * acceptable. > */ > > > + */ > > + > > +/** > > + * \fn StreamRole::role() > > + * \brief Retrieve the stream role > > + * > > + * \return The stream role hint > > s/ hint// > > > + */ > > + > > +/** > > + * \fn StreamRole::width() > > + * \brief Retrieve desired width > > + * > > + * \return The desired width if defined, -1 otherwise > > + */ > > + > > +/** > > + * \fn StreamRole::height() > > + * \brief Retrieve desired height > > + * > > + * \return The desired height if defined, -1 otherwise > > + */ > > After merging Jacopo's ImgU patch series I think we could use the Size > class, and modify it to use -1 as a marker of invalid dimensions. > > > + > > +/** > > + * \brief Create a stream role > > + * \param[in] role Stream role hint > > I would like to drop the word hint, but "Stream role" sounds weird. I > wonder if we should rename the StreamRole class to StreamUsage in order > to have two different words, usage and role. Feel free to propose an > alternative for "hint" that wouldn't require such a rename :-) > > > + */ > > +StreamRole::StreamRole(Role role) > > + : role_(role), width_(-1), height_(-1) > > +{ > > +} > > + > > +/** > > + * \brief Create a stream role with dimension hints > > + * \param[in] role Stream role hint > > + * \param[in] width Desired width > > + * \param[in] height Desired height > > + */ > > +StreamRole::StreamRole(Role role, int width, int height) > > + : role_(role), width_(width), height_(height) > > +{ > > +} > > + > > /** > > * \class Stream > > * \brief Video stream for a camera > > @@ -78,6 +133,39 @@ namespace libcamera { > > * optimal stream for the task. > > */ > > > > +/** > > + * \class Stream::StillCapture > > + * \brief Describes a still capture role > > + */ > > +Stream::StillCapture::StillCapture() > > + : StreamRole(Role::StillCapture) > > +{ > > +} > > + > > +/** > > + * \class Stream::VideoRecording > > + * \brief Describes a video recording role > > + */ > > +Stream::VideoRecording::VideoRecording() > > + : StreamRole(Role::VideoRecording) > > +{ > > +} > > + > > +/** > > + * \class Stream::Viewfinder > > + * \brief Describes a viewfinder role > > + */ > > + > > +/** > > + * \brief Create a viewfinder role with dimension hints > > + * \param[in] width Desired viewfinder width > > + * \param[in] height Desired viewfinder height > > + */ > > +Stream::Viewfinder::Viewfinder(int width, int height) > > + : StreamRole(Role::Viewfinder, width, height) > > +{ > > +} > > + > > /** > > * \brief Construct a stream with default parameters > > */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 970c479627fab064..adcf20d336347dad 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -21,9 +21,50 @@ struct StreamConfiguration { unsigned int bufferCount; }; +class StreamRole +{ +public: + enum Role { + StillCapture, + VideoRecording, + Viewfinder, + }; + + Role role() const { return role_; } + int width() const { return width_; } + int height() const { return height_; } + +protected: + StreamRole(Role role); + StreamRole(Role role, int width, int height); + +private: + Role role_; + int width_; + int height_; +}; + class Stream final { public: + class StillCapture : public StreamRole + { + public: + StillCapture(); + }; + + class VideoRecording : public StreamRole + { + public: + VideoRecording(); + }; + + class Viewfinder : public StreamRole + { + public: + Viewfinder(int width, int height); + }; + Stream(); BufferPool &bufferPool() { return bufferPool_; } const StreamConfiguration &configuration() const { return configuration_; } diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index c4943c91b2e6ce13..f4be5d265e872842 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -60,6 +60,61 @@ namespace libcamera { * \brief Requested number of buffers to allocate for the stream */ +/** + * \class StreamRole + * \brief Stream role information + * + * The StreamRole class carries information about stream usage hints from the + * application to the camera. The camera shall take the usage hints into account + * when select which stream to use for the desired operation. + */ + +/** + * \enum StreamRole::Role + * \brief List of different stream roles + */ + +/** + * \fn StreamRole::role() + * \brief Retrieve the stream role + * + * \return The stream role hint + */ + +/** + * \fn StreamRole::width() + * \brief Retrieve desired width + * + * \return The desired width if defined, -1 otherwise + */ + +/** + * \fn StreamRole::height() + * \brief Retrieve desired height + * + * \return The desired height if defined, -1 otherwise + */ + +/** + * \brief Create a stream role + * \param[in] role Stream role hint + */ +StreamRole::StreamRole(Role role) + : role_(role), width_(-1), height_(-1) +{ +} + +/** + * \brief Create a stream role with dimension hints + * \param[in] role Stream role hint + * \param[in] width Desired width + * \param[in] height Desired height + */ +StreamRole::StreamRole(Role role, int width, int height) + : role_(role), width_(width), height_(height) +{ +} + /** * \class Stream * \brief Video stream for a camera @@ -78,6 +133,39 @@ namespace libcamera { * optimal stream for the task. */ +/** + * \class Stream::StillCapture + * \brief Describes a still capture role + */ +Stream::StillCapture::StillCapture() + : StreamRole(Role::StillCapture) +{ +} + +/** + * \class Stream::VideoRecording + * \brief Describes a video recording role + */ +Stream::VideoRecording::VideoRecording() + : StreamRole(Role::VideoRecording) +{ +} + +/** + * \class Stream::Viewfinder + * \brief Describes a viewfinder role + */ + +/** + * \brief Create a viewfinder role with dimension hints + * \param[in] width Desired viewfinder width + * \param[in] height Desired viewfinder height + */ +Stream::Viewfinder::Viewfinder(int width, int height) + : StreamRole(Role::Viewfinder, width, height) +{ +} + /** * \brief Construct a stream with default parameters */
In preparation of reworking how a default configuration is retrieved from a camera add stream roles. The roles will be used by applications to describe how it intends to use a camera and replace the Stream IDs role when retrieving default configuration from the camera using streamConfiguration(). Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/stream.h | 41 ++++++++++++++++++ src/libcamera/stream.cpp | 88 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+)