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

[Jacopo Mondi]

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

> +	};
>
> +	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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190402/d8bafe16/attachment-0001.sig>