| Message ID | 20190402005332.25018-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Apr 02, 2019 at 02:53:31AM +0200, Niklas Söderlund wrote: > In preparation of reworking how a default configuration is retrieved > from a camera add stream usage hints. The hints 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 | 39 ++++++++++++++++++ > src/libcamera/stream.cpp | 84 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 123 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 3e8e83a2ff245879..445b80de66217934 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -35,7 +35,46 @@ private: > StreamConfiguration configuration_; > }; > > +class StreamHint > +{ > +public: > + enum Type { > + Viewfinder, > + Video, > + Still, I would use all capitals, and lengthen 'video' and 'still' as STILL_CAPTURE and VIDEO_CAPTURE > + }; > > + Type type() const { return type_; } const Type & > + > +protected: > + StreamHint(Type type); > + StreamHint(Type type, StreamConfiguration hints); &hints > + > +private: > + Type type_; > + StreamConfiguration hints_; I would call the configuration descriptor of an hint 'config_' not 'hints_'. > +}; > + > +class Viewfinder : public StreamHint > +{ > +public: > + Viewfinder(unsigned int width, unsigned int height); > + > +private: > + static StreamConfiguration initHints(unsigned int width, unsigned int height); > +}; > + > +class Video : public StreamHint > +{ > +public: > + Video(); > +}; > + > +class Still : public StreamHint > +{ > +public: > + Still(); > +}; I would question if these hierarchy brings any benefit or it just makes more complex adding new use cases. As I immagined this, use case hints might just be an enumeration with a configuration associated, ie. I don't think only viewfinder should have associated sizes, but it should still be possible to specify a max sizes for the other streams to make sure the selected streams could provide that resolution. Furthermore the base class has a configuration field, but it is only accessible for the viewfinder use case, why that? > > } /* namespace libcamera */ > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -102,4 +102,88 @@ Stream::Stream() > * \return The active configuration of the stream > */ > > +/** > + * \class StreamHint > + * \brief Stream usage hint information > + * > + * The StreamHint class carries information about stream usage hints from the > + * application to a specific pipeline handler implementation. The pipeline > + * handler shall take the usage hints into account when select which stream > + * to use for the desired operation. I would mention Camera not pipeline handlers here, as the interface for applications using hints will be the Camera class. > + */ > + > +/** > + * \enum StreamHint::Type > + * \brief List of types of usage hints > + */ > + > +/** > + * \fn StreamHint::type() > + * \brief Retrieve the usage hint type Don't we usually insert a blank line between \brief and \return? > + * \return The hint type > + */ > + > +/** > + * \brief Create a stream hint > + * \param[in] type Stream hint type > + */ > +StreamHint::StreamHint(Type type) > + : type_(type), hints_() > +{ > +} > + > +/** > + * \brief Create a stream hint with parameters > + * \param[in] type Stream hint type > + * \param[in] hints Stream hint parameters > + */ > +StreamHint::StreamHint(Type type, StreamConfiguration hints) > + : type_(type), hints_(hints) > +{ > +} > + > +/** > + * \class Viewfinder > + * \brief Stream usage hint for viewfinder > + */ > + > +/** > + * \brief Create a viewfinder stream hint > + * \param[in] width Desired view finder width viewfinder > + * \param[in] height Desired view finder height > + */ > +Viewfinder::Viewfinder(unsigned int width, unsigned int height) > + : StreamHint(Type::Viewfinder, initHints(width, height)) > +{ > +} > + > +StreamConfiguration Viewfinder::initHints(unsigned int width, > + unsigned int height) > +{ > + StreamConfiguration hints = {}; > + > + hints.width = width; > + hints.height = height; > + > + return hints; > +} Is this necessary? Couldn't you just pass width and height to the base class constructor and let it assign hints_.widht and hints_.height without going through copies? Or you think they would be elided by RVO everywhere here? > + > +/** > + * \class Video > + * \brief Stream usage hint for video > + */ > +Video::Video() > + : StreamHint(Type::Video) > +{ > +} > + > +/** > + * \class Still > + * \brief Stream usage hint for still images > + */ > +Still::Still() > + : StreamHint(Type::Still) > +{ > +} > + > } /* namespace libcamera */ > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2019-04-02 09:59:27 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Apr 02, 2019 at 02:53:31AM +0200, Niklas Söderlund wrote: > > In preparation of reworking how a default configuration is retrieved > > from a camera add stream usage hints. The hints 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 | 39 ++++++++++++++++++ > > src/libcamera/stream.cpp | 84 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 123 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 3e8e83a2ff245879..445b80de66217934 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -35,7 +35,46 @@ private: > > StreamConfiguration configuration_; > > }; > > > > +class StreamHint > > +{ > > +public: > > + enum Type { > > + Viewfinder, > > + Video, > > + Still, > > I would use all capitals, and lengthen 'video' and 'still' as > STILL_CAPTURE and VIDEO_CAPTURE I don't agree I think it's better as is :-) As this is bike shedding territory I will yield to popular opinion. What to the rest of you think? > > > + }; > > > > + Type type() const { return type_; } > > const Type & Thanks. > > > + > > +protected: > > + StreamHint(Type type); > > + StreamHint(Type type, StreamConfiguration hints); > > &hints Thanks, will make this a const &. > > > + > > +private: > > + Type type_; > > + StreamConfiguration hints_; > > I would call the configuration descriptor of an hint 'config_' not > 'hints_'. I tried that at first but it gave me a bad feeling as it's in fact not a configuration it's a hint of the configuration the user wish to use. I will keep this as hints_ for now. I'm not even sure the type should be StreamConfiguration or if we should inline the variables we judge to be useful as hints. As we have no pipelines yet to experiment with making use of the hints I picked something to store the user provided hints in. Also note that there is currently no way for a pipeline handler to get hold of the content of hints_. This is by design to force me to do something here when we join this work with the stream properties :-) > > > +}; > > + > > +class Viewfinder : public StreamHint > > +{ > > +public: > > + Viewfinder(unsigned int width, unsigned int height); > > + > > +private: > > + static StreamConfiguration initHints(unsigned int width, unsigned int height); > > +}; > > + > > +class Video : public StreamHint > > +{ > > +public: > > + Video(); > > +}; > > + > > +class Still : public StreamHint > > +{ > > +public: > > + Still(); > > +}; > > I would question if these hierarchy brings any benefit or it just > makes more complex adding new use cases. I don't think we will add use cases all that much. The main reason I went with this design is to make it easier for applications to use the interface. foo->streamConfiguration({ Viewfinder(640,480), Still() }); Is quiet nice to use instead of having to create raw StreamHint objects and configure them. It also gives us a bit more of control, with this design we can force any hint for a Viewfinder to supply a hint for the resolution while if we allowed raw StreamHint creation pipeline handers would need to be prepared to handle them both with and without that information. I expect his area will change a lot as we move forward, both based on needs from applications and implementations in pipeline handlers. In this RFC I have only parameterized the viewfinder hint as it's quiet clear dimensions would be part of its hints. I expect Video and Still to be parameterized in the near future. > > As I immagined this, use case hints might just be an enumeration with > a configuration associated, ie. I don't think only viewfinder should > have associated sizes, but it should still be possible to specify a > max sizes for the other streams to make sure the selected streams > could provide that resolution. Furthermore the base class has a > configuration field, but it is only accessible for the viewfinder use > case, why that? > > > > > } /* namespace libcamera */ > > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > > index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -102,4 +102,88 @@ Stream::Stream() > > * \return The active configuration of the stream > > */ > > > > +/** > > + * \class StreamHint > > + * \brief Stream usage hint information > > + * > > + * The StreamHint class carries information about stream usage hints from the > > + * application to a specific pipeline handler implementation. The pipeline > > + * handler shall take the usage hints into account when select which stream > > + * to use for the desired operation. > > I would mention Camera not pipeline handlers here, as the interface > for applications using hints will be the Camera class. The Camera don't make use of the hints it only forwards them to the pipeline handler. I think it's correct to describe that here, that the expectation is that the pipeline handler is responsible to make good use of the hints. > > > > + */ > > + > > +/** > > + * \enum StreamHint::Type > > + * \brief List of types of usage hints > > + */ > > + > > +/** > > + * \fn StreamHint::type() > > + * \brief Retrieve the usage hint type > > Don't we usually insert a blank line between \brief and \return? We do, will fix for next version. Thanks. > > > + * \return The hint type > > + */ > > + > > +/** > > + * \brief Create a stream hint > > + * \param[in] type Stream hint type > > + */ > > +StreamHint::StreamHint(Type type) > > + : type_(type), hints_() > > +{ > > +} > > + > > +/** > > + * \brief Create a stream hint with parameters > > + * \param[in] type Stream hint type > > + * \param[in] hints Stream hint parameters > > + */ > > +StreamHint::StreamHint(Type type, StreamConfiguration hints) > > + : type_(type), hints_(hints) > > +{ > > +} > > + > > +/** > > + * \class Viewfinder > > + * \brief Stream usage hint for viewfinder > > + */ > > + > > +/** > > + * \brief Create a viewfinder stream hint > > + * \param[in] width Desired view finder width > > viewfinder Thanks. > > > + * \param[in] height Desired view finder height > > + */ > > +Viewfinder::Viewfinder(unsigned int width, unsigned int height) > > + : StreamHint(Type::Viewfinder, initHints(width, height)) > > +{ > > +} > > + > > +StreamConfiguration Viewfinder::initHints(unsigned int width, > > + unsigned int height) > > +{ > > + StreamConfiguration hints = {}; > > + > > + hints.width = width; > > + hints.height = height; > > + > > + return hints; > > +} > > Is this necessary? Couldn't you just pass width and height to the base > class constructor and let it assign hints_.widht and hints_.height > without going through copies? Or you think they would be elided by > RVO everywhere here? I'm not concerned about the var being copied anywhere. My rational for using this is as we move forward I think these static initializer functions to be moved to StreamHints and used by other stream usage hint implementations. > > > + > > +/** > > + * \class Video > > + * \brief Stream usage hint for video > > + */ > > +Video::Video() > > + : StreamHint(Type::Video) > > +{ > > +} > > + > > +/** > > + * \class Still > > + * \brief Stream usage hint for still images > > + */ > > +Still::Still() > > + : StreamHint(Type::Still) > > +{ > > +} > > + > > } /* namespace libcamera */ > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Tue, Apr 02, 2019 at 02:53:31AM +0200, Niklas Söderlund wrote: > In preparation of reworking how a default configuration is retrieved > from a camera add stream usage hints. The hints 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 | 39 ++++++++++++++++++ > src/libcamera/stream.cpp | 84 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 123 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 3e8e83a2ff245879..445b80de66217934 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -35,7 +35,46 @@ private: > StreamConfiguration configuration_; > }; > > +class StreamHint Should we name this StreamUsage instead ? I'm not sure the word "hint" is a really good fit for the API, as these are more than hints. In any case it would be usage hints, so I think StreamUsage is better than StreamHint, and StreamUsageHint seems a bit too long. > +{ > +public: > + enum Type { Maybe Role instead or Type ? > + Viewfinder, > + Video, > + Still, You could go for VideoRecording and StillCapture, to be a bit more descriptive. > + }; > > + Type type() const { return type_; } > + > +protected: > + StreamHint(Type type); > + StreamHint(Type type, StreamConfiguration hints); Doesn't this abuse the StreamConfiguration class a bit, shouldn't we use a size (or possibly format) class ? > + > +private: > + Type type_; > + StreamConfiguration hints_; > +}; > + > +class Viewfinder : public StreamHint > +{ > +public: > + Viewfinder(unsigned int width, unsigned int height); > + > +private: > + static StreamConfiguration initHints(unsigned int width, unsigned int height); A bit overkill in my opinion, I don't think we need this helper. > +}; > + > +class Video : public StreamHint > +{ > +public: > + Video(); > +}; > + > +class Still : public StreamHint > +{ > +public: > + Still(); > +}; Those are very generic names, and may lead to namespace clashes. You could isolate them in a separate namespace (possibly moving them to the Stream class), or rename them to StreamUsageViewfinder, StreamUsageVideoRecording and StreamUsageStillCapture. I like this overall, but I wonder if it's really worth subclassing StreamHint. > > } /* namespace libcamera */ > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -102,4 +102,88 @@ Stream::Stream() > * \return The active configuration of the stream > */ > > +/** > + * \class StreamHint > + * \brief Stream usage hint information > + * > + * The StreamHint class carries information about stream usage hints from the > + * application to a specific pipeline handler implementation. The pipeline > + * handler shall take the usage hints into account when select which stream > + * to use for the desired operation. > + */ > + > +/** > + * \enum StreamHint::Type > + * \brief List of types of usage hints > + */ > + > +/** > + * \fn StreamHint::type() > + * \brief Retrieve the usage hint type > + * \return The hint type > + */ > + > +/** > + * \brief Create a stream hint > + * \param[in] type Stream hint type > + */ > +StreamHint::StreamHint(Type type) > + : type_(type), hints_() > +{ > +} > + > +/** > + * \brief Create a stream hint with parameters > + * \param[in] type Stream hint type > + * \param[in] hints Stream hint parameters > + */ > +StreamHint::StreamHint(Type type, StreamConfiguration hints) > + : type_(type), hints_(hints) > +{ > +} > + > +/** > + * \class Viewfinder > + * \brief Stream usage hint for viewfinder > + */ > + > +/** > + * \brief Create a viewfinder stream hint > + * \param[in] width Desired view finder width > + * \param[in] height Desired view finder height > + */ > +Viewfinder::Viewfinder(unsigned int width, unsigned int height) > + : StreamHint(Type::Viewfinder, initHints(width, height)) > +{ > +} > + > +StreamConfiguration Viewfinder::initHints(unsigned int width, > + unsigned int height) > +{ > + StreamConfiguration hints = {}; > + > + hints.width = width; > + hints.height = height; > + > + return hints; > +} > + > +/** > + * \class Video > + * \brief Stream usage hint for video > + */ > +Video::Video() > + : StreamHint(Type::Video) > +{ > +} > + > +/** > + * \class Still > + * \brief Stream usage hint for still images > + */ > +Still::Still() > + : StreamHint(Type::Still) > +{ > +} > + > } /* namespace libcamera */
Hello, On Tue, Apr 02, 2019 at 02:02:14PM +0200, Niklas Söderlund wrote: > On 2019-04-02 09:59:27 +0200, Jacopo Mondi wrote: > > On Tue, Apr 02, 2019 at 02:53:31AM +0200, Niklas Söderlund wrote: > >> In preparation of reworking how a default configuration is retrieved > >> from a camera add stream usage hints. The hints 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 | 39 ++++++++++++++++++ > >> src/libcamera/stream.cpp | 84 ++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 123 insertions(+) > >> > >> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > >> index 3e8e83a2ff245879..445b80de66217934 100644 > >> --- a/include/libcamera/stream.h > >> +++ b/include/libcamera/stream.h > >> @@ -35,7 +35,46 @@ private: > >> StreamConfiguration configuration_; > >> }; > >> > >> +class StreamHint > >> +{ > >> +public: > >> + enum Type { > >> + Viewfinder, > >> + Video, > >> + Still, > > > > I would use all capitals, and lengthen 'video' and 'still' as > > STILL_CAPTURE and VIDEO_CAPTURE > > I don't agree I think it's better as is :-) As this is bike shedding > territory I will yield to popular opinion. What to the rest of you > think? All our enums use CamelCase. > > > >> + }; > >> > >> + Type type() const { return type_; } > > > > const Type & > > Thanks. A bit overkill as Type is just an integer... > >> + > >> +protected: > >> + StreamHint(Type type); > >> + StreamHint(Type type, StreamConfiguration hints); > > > > &hints > > Thanks, will make this a const &. Not overkill at all :-) > >> + > >> +private: > >> + Type type_; > >> + StreamConfiguration hints_; > > > > I would call the configuration descriptor of an hint 'config_' not > > 'hints_'. > > I tried that at first but it gave me a bad feeling as it's in fact not a > configuration it's a hint of the configuration the user wish to use. I > will keep this as hints_ for now. > > I'm not even sure the type should be StreamConfiguration or if we should > inline the variables we judge to be useful as hints. As we have no > pipelines yet to experiment with making use of the hints I picked > something to store the user provided hints in. Also note that there is > currently no way for a pipeline handler to get hold of the content of > hints_. This is by design to force me to do something here when we join > this work with the stream properties :-) > > >> +}; > >> + > >> +class Viewfinder : public StreamHint > >> +{ > >> +public: > >> + Viewfinder(unsigned int width, unsigned int height); > >> + > >> +private: > >> + static StreamConfiguration initHints(unsigned int width, unsigned int height); > >> +}; > >> + > >> +class Video : public StreamHint > >> +{ > >> +public: > >> + Video(); > >> +}; > >> + > >> +class Still : public StreamHint > >> +{ > >> +public: > >> + Still(); > >> +}; > > > > I would question if these hierarchy brings any benefit or it just > > makes more complex adding new use cases. > > I don't think we will add use cases all that much. The main reason I > went with this design is to make it easier for applications to use the > interface. > > foo->streamConfiguration({ Viewfinder(640,480), Still() }); I pondered the same as Jacopo, but reading this feels quite nice... > Is quiet nice to use instead of having to create raw StreamHint objects > and configure them. It also gives us a bit more of control, with this > design we can force any hint for a Viewfinder to supply a hint for the > resolution while if we allowed raw StreamHint creation pipeline handers > would need to be prepared to handle them both with and without that > information. You're right, in the end the API is less error-prone. We may end up reconsidering this though, if the stream usages require more flexibility than what could easily be handled through parameters to the constructors. > I expect his area will change a lot as we move forward, both based on > needs from applications and implementations in pipeline handlers. In > this RFC I have only parameterized the viewfinder hint as it's quiet > clear dimensions would be part of its hints. I expect Video and Still to > be parameterized in the near future. > > > As I immagined this, use case hints might just be an enumeration with > > a configuration associated, ie. I don't think only viewfinder should > > have associated sizes, but it should still be possible to specify a > > max sizes for the other streams to make sure the selected streams > > could provide that resolution. Furthermore the base class has a > > configuration field, but it is only accessible for the viewfinder use > > case, why that? > > > >> > >> } /* namespace libcamera */ > >> > >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > >> index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644 > >> --- a/src/libcamera/stream.cpp > >> +++ b/src/libcamera/stream.cpp > >> @@ -102,4 +102,88 @@ Stream::Stream() > >> * \return The active configuration of the stream > >> */ > >> > >> +/** > >> + * \class StreamHint > >> + * \brief Stream usage hint information > >> + * > >> + * The StreamHint class carries information about stream usage hints from the > >> + * application to a specific pipeline handler implementation. The pipeline > >> + * handler shall take the usage hints into account when select which stream > >> + * to use for the desired operation. > > > > I would mention Camera not pipeline handlers here, as the interface > > for applications using hints will be the Camera class. > > The Camera don't make use of the hints it only forwards them to the > pipeline handler. I think it's correct to describe that here, that the > expectation is that the pipeline handler is responsible to make good use > of the hints. Note that usage hints are public API, while pipeline handlers are purely internal. I'm with Jacopo on this one, I wouldn't mention pipeline handlers in the documentation of public classes. > >> + */ > >> + > >> +/** > >> + * \enum StreamHint::Type > >> + * \brief List of types of usage hints > >> + */ > >> + > >> +/** > >> + * \fn StreamHint::type() > >> + * \brief Retrieve the usage hint type > > > > Don't we usually insert a blank line between \brief and \return? > > We do, will fix for next version. Thanks. > > >> + * \return The hint type > >> + */ > >> + > >> +/** > >> + * \brief Create a stream hint > >> + * \param[in] type Stream hint type > >> + */ > >> +StreamHint::StreamHint(Type type) > >> + : type_(type), hints_() > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Create a stream hint with parameters > >> + * \param[in] type Stream hint type > >> + * \param[in] hints Stream hint parameters > >> + */ > >> +StreamHint::StreamHint(Type type, StreamConfiguration hints) > >> + : type_(type), hints_(hints) > >> +{ > >> +} > >> + > >> +/** > >> + * \class Viewfinder > >> + * \brief Stream usage hint for viewfinder > >> + */ > >> + > >> +/** > >> + * \brief Create a viewfinder stream hint > >> + * \param[in] width Desired view finder width > > > > viewfinder > > Thanks. > > >> + * \param[in] height Desired view finder height > >> + */ > >> +Viewfinder::Viewfinder(unsigned int width, unsigned int height) > >> + : StreamHint(Type::Viewfinder, initHints(width, height)) > >> +{ > >> +} > >> + > >> +StreamConfiguration Viewfinder::initHints(unsigned int width, > >> + unsigned int height) > >> +{ > >> + StreamConfiguration hints = {}; > >> + > >> + hints.width = width; > >> + hints.height = height; > >> + > >> + return hints; > >> +} > > > > Is this necessary? Couldn't you just pass width and height to the base > > class constructor and let it assign hints_.widht and hints_.height > > without going through copies? Or you think they would be elided by > > RVO everywhere here? > > I'm not concerned about the var being copied anywhere. My rational for > using this is as we move forward I think these static initializer > functions to be moved to StreamHints and used by other stream usage > hint implementations. > > >> + > >> +/** > >> + * \class Video > >> + * \brief Stream usage hint for video > >> + */ > >> +Video::Video() > >> + : StreamHint(Type::Video) > >> +{ > >> +} > >> + > >> +/** > >> + * \class Still > >> + * \brief Stream usage hint for still images > >> + */ > >> +Still::Still() > >> + : StreamHint(Type::Still) > >> +{ > >> +} > >> + > >> } /* namespace libcamera */
Hi Laurent, Thanks for your feedback. On 2019-04-02 17:35:47 +0300, Laurent Pinchart wrote: > > +}; > > + > > +class Video : public StreamHint > > +{ > > +public: > > + Video(); > > +}; > > + > > +class Still : public StreamHint > > +{ > > +public: > > + Still(); > > +}; > > Those are very generic names, and may lead to namespace clashes. You > could isolate them in a separate namespace (possibly moving them to the > Stream class), or rename them to StreamUsageViewfinder, > StreamUsageVideoRecording and StreamUsageStillCapture. I wrestled a lot with how to best handle this, for next version I went with moving the subclasses inside Stream class as it seems create then nicest interface for applications to use. Lets see how it works out.
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 3e8e83a2ff245879..445b80de66217934 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -35,7 +35,46 @@ private: StreamConfiguration configuration_; }; +class StreamHint +{ +public: + enum Type { + Viewfinder, + Video, + Still, + }; + Type type() const { return type_; } + +protected: + StreamHint(Type type); + StreamHint(Type type, StreamConfiguration hints); + +private: + Type type_; + StreamConfiguration hints_; +}; + +class Viewfinder : public StreamHint +{ +public: + Viewfinder(unsigned int width, unsigned int height); + +private: + static StreamConfiguration initHints(unsigned int width, unsigned int height); +}; + +class Video : public StreamHint +{ +public: + Video(); +}; + +class Still : public StreamHint +{ +public: + Still(); +}; } /* namespace libcamera */ diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index c4943c91b2e6ce13..32f26a1f8e6adb2c 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -102,4 +102,88 @@ Stream::Stream() * \return The active configuration of the stream */ +/** + * \class StreamHint + * \brief Stream usage hint information + * + * The StreamHint class carries information about stream usage hints from the + * application to a specific pipeline handler implementation. The pipeline + * handler shall take the usage hints into account when select which stream + * to use for the desired operation. + */ + +/** + * \enum StreamHint::Type + * \brief List of types of usage hints + */ + +/** + * \fn StreamHint::type() + * \brief Retrieve the usage hint type + * \return The hint type + */ + +/** + * \brief Create a stream hint + * \param[in] type Stream hint type + */ +StreamHint::StreamHint(Type type) + : type_(type), hints_() +{ +} + +/** + * \brief Create a stream hint with parameters + * \param[in] type Stream hint type + * \param[in] hints Stream hint parameters + */ +StreamHint::StreamHint(Type type, StreamConfiguration hints) + : type_(type), hints_(hints) +{ +} + +/** + * \class Viewfinder + * \brief Stream usage hint for viewfinder + */ + +/** + * \brief Create a viewfinder stream hint + * \param[in] width Desired view finder width + * \param[in] height Desired view finder height + */ +Viewfinder::Viewfinder(unsigned int width, unsigned int height) + : StreamHint(Type::Viewfinder, initHints(width, height)) +{ +} + +StreamConfiguration Viewfinder::initHints(unsigned int width, + unsigned int height) +{ + StreamConfiguration hints = {}; + + hints.width = width; + hints.height = height; + + return hints; +} + +/** + * \class Video + * \brief Stream usage hint for video + */ +Video::Video() + : StreamHint(Type::Video) +{ +} + +/** + * \class Still + * \brief Stream usage hint for still images + */ +Still::Still() + : StreamHint(Type::Still) +{ +} + } /* namespace libcamera */
In preparation of reworking how a default configuration is retrieved from a camera add stream usage hints. The hints 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 | 39 ++++++++++++++++++ src/libcamera/stream.cpp | 84 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+)