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

8718 diff mbox series

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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? Should we state if this total includes bytes required for
all image planes explicitly?

In fact, will there be a planeSize function too?

Still, that can be later either way.

Reviewed-by: Kieran Bingham <kieran.bingham@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.

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.



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

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@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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