[libcamera-devel] [RFC 4/5] libcamera: stream: Add basic stream usage hints definitions

[Laurent Pinchart]

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 at 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 */

-- 
Regards,

Laurent Pinchart