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

[Niklas Söderlund]

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

-- 
Regards,
Niklas Söderlund