[libcamera-devel] [PATCH 20/30] libcamera: stream: Add prototypes for new interface

[Jacopo Mondi]

Hi Niklas,

On Wed, Nov 27, 2019 at 12:36:10AM +0100, Niklas Söderlund wrote:
> The buffer allocation rework will remove most of the methods in the
> Stream class. This change adds the prototypes for the FrameBuffer
> interface with stub default implementations.
>
> Once the new buffer allocation work is completed the prototypes added in
> this change will be turned into pure virtual functions preventing
> the base Stream class from being instantiated.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/stream.h | 11 ++++++++
>  src/libcamera/stream.cpp   | 55 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
>
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index a404eccf34d9c93b..395148e45d342d92 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_STREAM_H__
>  #define __LIBCAMERA_STREAM_H__
>
> +#include <errno.h>
>  #include <map>
>  #include <memory>
>  #include <string>
> @@ -74,6 +75,7 @@ class Stream
>  {
>  public:
>  	Stream();
> +	virtual ~Stream(){};

You can add this when you'll make the methods virtual... anyway, it's
fine

>
>  	std::unique_ptr<Buffer> createBuffer(unsigned int index);
>  	std::unique_ptr<Buffer> createBuffer(const std::array<int, 3> &fds);
> @@ -86,6 +88,15 @@ public:
>  protected:
>  	friend class Camera;
>
> +	virtual int allocateBuffers(const StreamConfiguration &config,
> +				    std::vector<FrameBuffer *> *buffers)
> +	{
> +		return -EINVAL;
> +	}
> +	virtual void releaseBuffers() { return; }
> +	virtual int start() { return -EINVAL; }
> +	virtual void stop() { return; }
> +
>  	int mapBuffer(const Buffer *buffer);
>  	void unmapBuffer(const Buffer *buffer);
>
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index e70a1e307ecaa5ba..ba3f571b08cb0c41 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -520,6 +520,61 @@ std::unique_ptr<Buffer> Stream::createBuffer(const std::array<int, 3> &fds)
>   * \return The memory type used by the stream
>   */
>
> +/**
> + * \fn Stream::allocateBuffers()
> + * \brief Allocate buffers from the stream

from or for ?

> + * \param[in] config A configuration describing the buffer(s) to be allocated
> + * \param[out] buffers Array of buffers successfully allocated

vector or array ?

"succesfully allocate buffers"

As the vector is emtpy if allocation fails, do you need "successfully" ?

> + *
> + * Allocate buffers matching exactly what is described in \a config and return
> + * then in \a buffers. If buffers matching \a config can't be allocated an error
> + * shall be returned and no buffers returned in \a buffers.

I would

Allocate buffers for the Stream using the provided configuration. If
the allocated buffer parameters cannot match the requested
configuration an error is returned and the \a buffers vector is empty.

> + *
> + * This is a helper which may be used by libcamera helper classes to allocate
an helper

> + * buffers from the stream itself. The allocated buffers may then be treated
> + * in the same way as if they where externally allocated.

Not sure I got what you mean (and helper is repeated twice in 2 lines)

> + *
> + * The only intended caller is buffer allocator helpers and the function must
> + * be implemeted by all subclasses of Stream.

This last part is implied by the fact the operation is a pure virtual
one. Same for all the other comments.

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn Stream::releaseBuffers()
> + * \brief Relase buffers allocated from the stram

for or from ?

> + *
> + * This is a helper which release buffers allocated using allocateBuffers(). The
an helper

> + * only intended caller is buffer allocator helpers.

s/helper//

> + *
> + * The only intended caller is buffer allocator helpers and the function must
> + * be implemeted by all subclasses of Stream.

implied

> + */
> +
> +/**
> + * \fn Stream::start()
> + * \brief Prepare a stream for capture
> + *
> + * The subclss shall prepare the stream for capture and if needed allocate
> + * resources to allow for that. No buffers may be allocated from the stream

s/may/can

> + * using allocateBuffers() after a stream have been started.

s/have/has

> + *
> + * The only intended caller is the camera base class and the function must be
> + * implemeted by all subclasses of Stream.

implied

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn Stream::stop()
> + * \brief Stop a stream after capture
> + *
> + * The subclass shall free all resources allocated in start().

I would remove the use of "subclass" from these two methods
description.

> + *
> + * The only intended caller is the camera base class and the function must be
> + * implemeted by all subclasses of Stream.
> + */
> +
>  /**
>   * \brief Map a Buffer to a buffer memory index
>   * \param[in] buffer The buffer to map to a buffer memory index
> --
> 2.24.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/20191202/3538d0e3/attachment.sig>