[libcamera-devel,RFC,07/12] libcamera: buffer: Add dedicated container for buffer information

2267 diff mbox series

The Buffer object will be split in two, one containing the memory and
one containing the information recorded from when the buffer is
dequeued. Add a container for the later in preparation for the split.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/buffer.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Hi Niklas,

Thank you for the patch.

On Mon, Oct 28, 2019 at 03:25:20AM +0100, Niklas Söderlund wrote:
> The Buffer object will be split in two, one containing the memory and
> one containing the information recorded from when the buffer is
> dequeued. Add a container for the later in preparation for the split.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/buffer.h | 34 ++++++++++++++++++++++++++++++++++

This is missing documentation (hence the RFC status I suppose). I assume
you're aware of it :-)

>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 54c757ef7db8b5f6..c626f669040b3c04 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -105,6 +105,40 @@ private:
>  	Stream *stream_;
>  };
>  
> +class BufferInfo

An alternative name could be BufferMetaData. I wanted to mention it as
that's what I envisioned when we discussed this topic a few weeks ago.
We need a really good naming scheme for the buffer-related API, and
BufferInfo may be best, let's see how it fits in the big picture.

> +{
> +public:
> +	enum Status {
> +		BufferSuccess,
> +		BufferError,
> +		BufferCancelled,
> +	};
> +
> +	struct PlaneInfo {
> +		unsigned int bytesused;
> +	};

Do you foresee more plane-specific fields in the future, or should be
drop this structures and replace planes_ with a vector of bytesused ? I
think I'm leaning a bit towards keeping it as-is, but I may change my
mind while reviewing the rest of the series.

> +
> +	BufferInfo(Status status, unsigned int sequence, uint64_t timestamp,
> +		   const std::vector<PlaneInfo> &planes)
> +		: status_(status), sequence_(sequence), timestamp_(timestamp),
> +		  planes_(planes)
> +	{
> +	}
> +
> +	Status status() const { return status_; }
> +	unsigned int sequence() const { return sequence_; }
> +	unsigned int timestamp() const { return timestamp_; }

The return type doesn't match the timestamp_ member type.

> +
> +	unsigned int planes() const { return planes_.size(); }
> +	const PlaneInfo &plane(unsigned int plane) const { return planes_.at(plane); }

How about replacing those two methods with

	const std::vector<PlaneInfo> &planes() const { return planes_; }

? The planes() method above is a bit ill-named, as it returns the number
of planes, and I think returning the vector will result in more flexible
code for the caller.

> +
> +private:
> +	Status status_;
> +	unsigned int sequence_;
> +	uint64_t timestamp_;

Should this be a std::chrono type instead ?

> +	std::vector<PlaneInfo> planes_;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_BUFFER_H__ */

Hi Laurent,

Thanks for your feedback.

On 2019-11-18 21:27:42 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Oct 28, 2019 at 03:25:20AM +0100, Niklas Söderlund wrote:
> > The Buffer object will be split in two, one containing the memory and
> > one containing the information recorded from when the buffer is
> > dequeued. Add a container for the later in preparation for the split.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/buffer.h | 34 ++++++++++++++++++++++++++++++++++
> 
> This is missing documentation (hence the RFC status I suppose). I assume
> you're aware of it :-)

:-)

> 
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 54c757ef7db8b5f6..c626f669040b3c04 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -105,6 +105,40 @@ private:
> >  	Stream *stream_;
> >  };
> >  
> > +class BufferInfo
> 
> An alternative name could be BufferMetaData. I wanted to mention it as
> that's what I envisioned when we discussed this topic a few weeks ago.
> We need a really good naming scheme for the buffer-related API, and
> BufferInfo may be best, let's see how it fits in the big picture.

I'm not opposed to naming this BufferMetaData. But will keep it as 
BufferInfo until we make a decision based on the big picture.

> 
> > +{
> > +public:
> > +	enum Status {
> > +		BufferSuccess,
> > +		BufferError,
> > +		BufferCancelled,
> > +	};
> > +
> > +	struct PlaneInfo {
> > +		unsigned int bytesused;
> > +	};
> 
> Do you foresee more plane-specific fields in the future, or should be
> drop this structures and replace planes_ with a vector of bytesused ? I
> think I'm leaning a bit towards keeping it as-is, but I may change my
> mind while reviewing the rest of the series.
> 
> > +
> > +	BufferInfo(Status status, unsigned int sequence, uint64_t timestamp,
> > +		   const std::vector<PlaneInfo> &planes)
> > +		: status_(status), sequence_(sequence), timestamp_(timestamp),
> > +		  planes_(planes)
> > +	{
> > +	}
> > +
> > +	Status status() const { return status_; }
> > +	unsigned int sequence() const { return sequence_; }
> > +	unsigned int timestamp() const { return timestamp_; }
> 
> The return type doesn't match the timestamp_ member type.

Thanks.

> 
> > +
> > +	unsigned int planes() const { return planes_.size(); }
> > +	const PlaneInfo &plane(unsigned int plane) const { return planes_.at(plane); }
> 
> How about replacing those two methods with
> 
> 	const std::vector<PlaneInfo> &planes() const { return planes_; }
> 
> ? The planes() method above is a bit ill-named, as it returns the number
> of planes, and I think returning the vector will result in more flexible
> code for the caller.

I like it will do so in next version.


> 
> > +
> > +private:
> > +	Status status_;
> > +	unsigned int sequence_;
> > +	uint64_t timestamp_;
> 
> Should this be a std::chrono type instead ?

I'm not opposed to this, but maybe we can do this on top if we think 
it's the right thing? I have added this to my list of buffer stuff to 
do.

> 
> > +	std::vector<PlaneInfo> planes_;
> > +};
> > +
> >  } /* namespace libcamera */
> >  
> >  #endif /* __LIBCAMERA_BUFFER_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart