[libcamera-devel] libcamera: pipeline: rkisp1: Fix array size of formats

Message ID 20200806141435.3023001-1-niklas.soderlund@ragnatech.se
State Accepted
Commit c29ee8ef73da6b93f6800b5699ba86d9f1976f36
Headers show
Series
  • [libcamera-devel] libcamera: pipeline: rkisp1: Fix array size of formats
Related show

Commit Message

Niklas Söderlund Aug. 6, 2020, 2:14 p.m. UTC
When switching from V4L2 to DRM pixel formats V4L2_PIX_FMT_GREY was
dropped form the list of supported formats but the arrays size was never
decreased, fix this.

Fixes: 448716d8f7518579 ("libcamera: Switch PixelFormat to DRM FourCC values")
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham Aug. 10, 2020, 12:44 p.m. UTC | #1
Hi Niklas,

On 06/08/2020 15:14, Niklas Söderlund wrote:
> When switching from V4L2 to DRM pixel formats V4L2_PIX_FMT_GREY was
> dropped form the list of supported formats but the arrays size was never
> decreased, fix this.
> 
> Fixes: 448716d8f7518579 ("libcamera: Switch PixelFormat to DRM FourCC values")
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index b7609cbc8f363135..32fdaed7c661ae74 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -461,7 +461,7 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
> -	static const std::array<PixelFormat, 8> formats{
> +	static const std::array<PixelFormat, 7> formats{

Ayee, it's a shame we can't have these automatically sized for us by the
compiler...

But indeed, I count 7.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

The Greyscale format is supported in our DRM formats as formats::R8 ...
but I think it looks like more than just the (re)addition of the line to
this table to get that to work, so I guess it could be left as a todo.



>  		formats::YUYV,
>  		formats::YVYU,
>  		formats::VYUY,
>
Jacopo Mondi Aug. 10, 2020, 12:46 p.m. UTC | #2
Hi Niklas,

On Thu, Aug 06, 2020 at 04:14:35PM +0200, Niklas Söderlund wrote:
> When switching from V4L2 to DRM pixel formats V4L2_PIX_FMT_GREY was
> dropped form the list of supported formats but the arrays size was never
> decreased, fix this.
>
> Fixes: 448716d8f7518579 ("libcamera: Switch PixelFormat to DRM FourCC values")
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Good catch. What happens if we over allocate the array ?

Anyway:
Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index b7609cbc8f363135..32fdaed7c661ae74 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -461,7 +461,7 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
> -	static const std::array<PixelFormat, 8> formats{
> +	static const std::array<PixelFormat, 7> formats{
>  		formats::YUYV,
>  		formats::YVYU,
>  		formats::VYUY,
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Aug. 10, 2020, 1:42 p.m. UTC | #3
On Mon, Aug 10, 2020 at 01:44:23PM +0100, Kieran Bingham wrote:
> On 06/08/2020 15:14, Niklas Söderlund wrote:
> > When switching from V4L2 to DRM pixel formats V4L2_PIX_FMT_GREY was
> > dropped form the list of supported formats but the arrays size was never
> > decreased, fix this.
> > 
> > Fixes: 448716d8f7518579 ("libcamera: Switch PixelFormat to DRM FourCC values")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index b7609cbc8f363135..32fdaed7c661ae74 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -461,7 +461,7 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >  
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> > -	static const std::array<PixelFormat, 8> formats{
> > +	static const std::array<PixelFormat, 7> formats{
> 
> Ayee, it's a shame we can't have these automatically sized for us by the
> compiler...

There's std::to_array() ([1]) but only for C++20. There was also
std::make_array() ([2]) that I have implemented in utils:: as an
experiment. It helps a bit, but isn't completely ideal. Maybe we should
try utils::to_array().

[1] https://en.cppreference.com/w/cpp/container/array/to_array
[2] https://en.cppreference.com/w/cpp/experimental/make_array

> But indeed, I count 7.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> The Greyscale format is supported in our DRM formats as formats::R8 ...
> but I think it looks like more than just the (re)addition of the line to
> this table to get that to work, so I guess it could be left as a todo.
> 
> >  		formats::YUYV,
> >  		formats::YVYU,
> >  		formats::VYUY,

Patch

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index b7609cbc8f363135..32fdaed7c661ae74 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -461,7 +461,7 @@  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
 
 CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 {
-	static const std::array<PixelFormat, 8> formats{
+	static const std::array<PixelFormat, 7> formats{
 		formats::YUYV,
 		formats::YVYU,
 		formats::VYUY,