[libcamera-devel] [PATCH v5 03/23] libcamera: StreamConfiguration: Add frameSize field

[paul.elder at ideasonboard.com]

Hi Kieran,

On Thu, Jul 09, 2020 at 03:42:22PM +0100, Kieran Bingham wrote:
> Hi Paul,
> 
> On 09/07/2020 14:28, Paul Elder wrote:
> > In addition to the stride field, we want the pipeline handler to be able
> > to declare the frame size for the configuration. Add a frameSize field
> > to StreamConfiguration for this purpose.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > ---
> > No change in v5
> > 
> > No change in v4
> > 
> > New in v3
> > ---
> >  include/libcamera/stream.h |  1 +
> >  src/libcamera/stream.cpp   | 17 ++++++++++++++---
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 1a68bd2..f502b35 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -43,6 +43,7 @@ struct StreamConfiguration {
> >  	PixelFormat pixelFormat;
> >  	Size size;
> >  	unsigned int stride;
> > +	unsigned int frameSize;
> >  
> >  	unsigned int bufferCount;
> >  
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index 6df5882..6d6e279 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -279,7 +279,8 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
> >   * handlers provide StreamFormats.
> >   */
> >  StreamConfiguration::StreamConfiguration()
> > -	: pixelFormat(0), stride(0), bufferCount(0), stream_(nullptr)
> > +	: pixelFormat(0), stride(0), frameSize(0), bufferCount(0),
> > +	  stream_(nullptr)
> >  {
> >  }
> >  
> > @@ -287,8 +288,8 @@ StreamConfiguration::StreamConfiguration()
> >   * \brief Construct a configuration with stream formats
> >   */
> >  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> > -	: pixelFormat(0), stride(0), bufferCount(0), stream_(nullptr),
> > -	  formats_(formats)
> > +	: pixelFormat(0), stride(0), frameSize(0), bufferCount(0),
> > +	  stream_(nullptr), formats_(formats)
> >  {
> >  }
> >  
> > @@ -315,6 +316,16 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> >   * the camera is configured.
> >   */
> >  
> > +/**
> > + * \var StreamConfiguration::frameSize
> > + * \brief Frame size for the stream, in bytes
> > + *
> > + * The frameSize value reports the number of bytes necessary to contain one
> > + * frame of an image buffer for this stream. The value is valid after
> > + * successfully validating the configuration with a call to
> > + * CameraConfiguration::validate().
> 
> How will we handle multi-planar formats? Will this always be the total
> of all planes?

Yeah, this is the total of all planes.

> Should we state if this total includes bytes required for
> all image planes explicitly?

I'll add it for clarity.

> In fact, will there be a planeSize function too?

There doesn't seem to be demand for it at the moment. It's not that
difficult for pipeline handlers to do height * stride(format, plane
index).

> Still, that can be later either way.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> Though, should this be squashed with "libcamera: PixelFormatInfo: Add
> functions stride and frameSize" ? Otherwise this value becomes available
> at this point but always zero.

Even if it's squashed, the values will be zero until the pipeline
handlers fill it, so I think it's fine not to.

> It's only used first in that patch, so I'd at least put this patch
> directly before so that the closely-related patches are adjacent.

But this would be good, yes.

> > + */
> > +
> >  /**
> >   * \var StreamConfiguration::bufferCount
> >   * \brief Requested number of buffers to allocate for the stream
> > 


Thanks,

Paul