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

Tue Apr 2 14:02:14 CEST 2019

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 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?

> 
> > +	};
> >
> > +	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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund