[libcamera-devel,v5,11/13] libcamera: rpi: Change default stream formats
diff mbox series

Message ID 20230921165550.50956-12-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Introduce SensorConfiguration
Related show

Commit Message

Jacopo Mondi Sept. 21, 2023, 4:55 p.m. UTC 
From: Naushir Patuck <naush@raspberrypi.com>

Switch to XRGB8888 as a default Viewfinder role output format, and
YUV420 as a default output format for everything else.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Sept. 26, 2023, 10:51 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Thu, Sep 21, 2023 at 06:55:48PM +0200, Jacopo Mondi via libcamera-devel wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
> 
> Switch to XRGB8888 as a default Viewfinder role output format, and
> YUV420 as a default output format for everything else.

The commit message should explain *why* those formats are better.

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index c560e48c12fb..f8e8e13dc837 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -268,7 +268,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  
>  		if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
>  			/* If we cannot find a native format, use a default one. */
> -			cfgPixFmt = formats::NV12;
> +			cfgPixFmt = formats::YUV420;
>  			status = Adjusted;
>  		}
>  
> @@ -431,7 +431,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
>  
>  		case StreamRole::StillCapture:
>  			fmts = data->ispFormats();
> -			pixelFormat = formats::NV12;
> +			pixelFormat = formats::YUV420;
>  			/*
>  			 * Still image codecs usually expect the sYCC color space.
>  			 * Even RGB codecs will be fine as the RGB we get with the
> @@ -465,7 +465,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
>  
>  		case StreamRole::Viewfinder:
>  			fmts = data->ispFormats();
> -			pixelFormat = formats::ARGB8888;
> +			pixelFormat = formats::XRGB8888;
>  			colorSpace = ColorSpace::Sycc;
>  			size = { 800, 600 };
>  			bufferCount = 4;
Naushir Patuck Sept. 27, 2023, 7:30 a.m. UTC | #2 
Hi Laurent,

On Tue, 26 Sept 2023 at 23:51, Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Sep 21, 2023 at 06:55:48PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > From: Naushir Patuck <naush@raspberrypi.com>
> >
> > Switch to XRGB8888 as a default Viewfinder role output format, and
> > YUV420 as a default output format for everything else.
>
> The commit message should explain *why* those formats are better.

RGB24/32 formats are typically better supported by display pipelines
than YUV420.  Happy to add this line to the commit message, or can
this be done while applying?

Regards,
Naush

>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index c560e48c12fb..f8e8e13dc837 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -268,7 +268,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >
> >               if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
> >                       /* If we cannot find a native format, use a default one. */
> > -                     cfgPixFmt = formats::NV12;
> > +                     cfgPixFmt = formats::YUV420;
> >                       status = Adjusted;
> >               }
> >
> > @@ -431,7 +431,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> >
> >               case StreamRole::StillCapture:
> >                       fmts = data->ispFormats();
> > -                     pixelFormat = formats::NV12;
> > +                     pixelFormat = formats::YUV420;
> >                       /*
> >                        * Still image codecs usually expect the sYCC color space.
> >                        * Even RGB codecs will be fine as the RGB we get with the
> > @@ -465,7 +465,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> >
> >               case StreamRole::Viewfinder:
> >                       fmts = data->ispFormats();
> > -                     pixelFormat = formats::ARGB8888;
> > +                     pixelFormat = formats::XRGB8888;
> >                       colorSpace = ColorSpace::Sycc;
> >                       size = { 800, 600 };
> >                       bufferCount = 4;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Sept. 27, 2023, 7:48 a.m. UTC | #3 
On Wed, Sep 27, 2023 at 08:30:05AM +0100, Naushir Patuck wrote:
> On Tue, 26 Sept 2023 at 23:51, Laurent Pinchart via libcamera-devel wrote:
> > On Thu, Sep 21, 2023 at 06:55:48PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > From: Naushir Patuck <naush@raspberrypi.com>
> > >
> > > Switch to XRGB8888 as a default Viewfinder role output format, and
> > > YUV420 as a default output format for everything else.
> >
> > The commit message should explain *why* those formats are better.
> 
> RGB24/32 formats are typically better supported by display pipelines
> than YUV420.

This patch switches from YUV 4:2:0 semi-planar to planar, and from ARGB
to XRGB, so I'm not sure how that's related :-)

> Happy to add this line to the commit message, or can
> this be done while applying?

If you reply with an updated commit message, I can handle it when
applying.

> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index c560e48c12fb..f8e8e13dc837 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -268,7 +268,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >
> > >               if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
> > >                       /* If we cannot find a native format, use a default one. */
> > > -                     cfgPixFmt = formats::NV12;
> > > +                     cfgPixFmt = formats::YUV420;
> > >                       status = Adjusted;
> > >               }
> > >
> > > @@ -431,7 +431,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > >
> > >               case StreamRole::StillCapture:
> > >                       fmts = data->ispFormats();
> > > -                     pixelFormat = formats::NV12;
> > > +                     pixelFormat = formats::YUV420;
> > >                       /*
> > >                        * Still image codecs usually expect the sYCC color space.
> > >                        * Even RGB codecs will be fine as the RGB we get with the
> > > @@ -465,7 +465,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > >
> > >               case StreamRole::Viewfinder:
> > >                       fmts = data->ispFormats();
> > > -                     pixelFormat = formats::ARGB8888;
> > > +                     pixelFormat = formats::XRGB8888;
> > >                       colorSpace = ColorSpace::Sycc;
> > >                       size = { 800, 600 };
> > >                       bufferCount = 4;
Naushir Patuck Sept. 27, 2023, 8:59 a.m. UTC | #4 
Hi Laurent,

On Wed, 27 Sept 2023 at 08:48, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Sep 27, 2023 at 08:30:05AM +0100, Naushir Patuck wrote:
> > On Tue, 26 Sept 2023 at 23:51, Laurent Pinchart via libcamera-devel wrote:
> > > On Thu, Sep 21, 2023 at 06:55:48PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > From: Naushir Patuck <naush@raspberrypi.com>
> > > >
> > > > Switch to XRGB8888 as a default Viewfinder role output format, and
> > > > YUV420 as a default output format for everything else.
> > >
> > > The commit message should explain *why* those formats are better.
> >
> > RGB24/32 formats are typically better supported by display pipelines
> > than YUV420.
>
> This patch switches from YUV 4:2:0 semi-planar to planar, and from ARGB
> to XRGB, so I'm not sure how that's related :-)

Sorry, I phrased that completely wrong!  Here's a (hopefully) clearer
commit message:

Switch to XRGB8888 as a default Viewfinder role output format, this is
a more correct description of the ISP hardware output, and what is
accepted by the Raspberry Pi hardware.

Switch to YUV420 as a default output format for everything else, as
this format is best supported by encoding (e.g. H.264, JPEG) sinks on
the Raspberry Pi platform.

>
> > Happy to add this line to the commit message, or can
> > this be done while applying?
>
> If you reply with an updated commit message, I can handle it when
> applying.
>
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index c560e48c12fb..f8e8e13dc837 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -268,7 +268,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > >
> > > >               if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
> > > >                       /* If we cannot find a native format, use a default one. */
> > > > -                     cfgPixFmt = formats::NV12;
> > > > +                     cfgPixFmt = formats::YUV420;
> > > >                       status = Adjusted;
> > > >               }
> > > >
> > > > @@ -431,7 +431,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > > >
> > > >               case StreamRole::StillCapture:
> > > >                       fmts = data->ispFormats();
> > > > -                     pixelFormat = formats::NV12;
> > > > +                     pixelFormat = formats::YUV420;
> > > >                       /*
> > > >                        * Still image codecs usually expect the sYCC color space.
> > > >                        * Even RGB codecs will be fine as the RGB we get with the
> > > > @@ -465,7 +465,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > > >
> > > >               case StreamRole::Viewfinder:
> > > >                       fmts = data->ispFormats();
> > > > -                     pixelFormat = formats::ARGB8888;
> > > > +                     pixelFormat = formats::XRGB8888;
> > > >                       colorSpace = ColorSpace::Sycc;
> > > >                       size = { 800, 600 };
> > > >                       bufferCount = 4;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Sept. 27, 2023, 12:02 p.m. UTC | #5 
Hi Naush,

On Wed, Sep 27, 2023 at 09:59:36AM +0100, Naushir Patuck wrote:
> On Wed, 27 Sept 2023 at 08:48, Laurent Pinchart wrote:
> > On Wed, Sep 27, 2023 at 08:30:05AM +0100, Naushir Patuck wrote:
> > > On Tue, 26 Sept 2023 at 23:51, Laurent Pinchart via libcamera-devel wrote:
> > > > On Thu, Sep 21, 2023 at 06:55:48PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > > From: Naushir Patuck <naush@raspberrypi.com>
> > > > >
> > > > > Switch to XRGB8888 as a default Viewfinder role output format, and
> > > > > YUV420 as a default output format for everything else.
> > > >
> > > > The commit message should explain *why* those formats are better.
> > >
> > > RGB24/32 formats are typically better supported by display pipelines
> > > than YUV420.
> >
> > This patch switches from YUV 4:2:0 semi-planar to planar, and from ARGB
> > to XRGB, so I'm not sure how that's related :-)
> 
> Sorry, I phrased that completely wrong!  Here's a (hopefully) clearer
> commit message:
> 
> Switch to XRGB8888 as a default Viewfinder role output format, this is
> a more correct description of the ISP hardware output, and what is
> accepted by the Raspberry Pi hardware.
> 
> Switch to YUV420 as a default output format for everything else, as
> this format is best supported by encoding (e.g. H.264, JPEG) sinks on
> the Raspberry Pi platform.

Thank you. I've updated the commit message and pushed the series.

> > > Happy to add this line to the commit message, or can
> > > this be done while applying?
> >
> > If you reply with an updated commit message, I can handle it when
> > applying.
> >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > ---
> > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > index c560e48c12fb..f8e8e13dc837 100644
> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > @@ -268,7 +268,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > > > >
> > > > >               if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
> > > > >                       /* If we cannot find a native format, use a default one. */
> > > > > -                     cfgPixFmt = formats::NV12;
> > > > > +                     cfgPixFmt = formats::YUV420;
> > > > >                       status = Adjusted;
> > > > >               }
> > > > >
> > > > > @@ -431,7 +431,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > > > >
> > > > >               case StreamRole::StillCapture:
> > > > >                       fmts = data->ispFormats();
> > > > > -                     pixelFormat = formats::NV12;
> > > > > +                     pixelFormat = formats::YUV420;
> > > > >                       /*
> > > > >                        * Still image codecs usually expect the sYCC color space.
> > > > >                        * Even RGB codecs will be fine as the RGB we get with the
> > > > > @@ -465,7 +465,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > > > >
> > > > >               case StreamRole::Viewfinder:
> > > > >                       fmts = data->ispFormats();
> > > > > -                     pixelFormat = formats::ARGB8888;
> > > > > +                     pixelFormat = formats::XRGB8888;
> > > > >                       colorSpace = ColorSpace::Sycc;
> > > > >                       size = { 800, 600 };
> > > > >                       bufferCount = 4;

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index c560e48c12fb..f8e8e13dc837 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -268,7 +268,7 @@  CameraConfiguration::Status RPiCameraConfiguration::validate()
 
 		if (fmts.find(out.dev->toV4L2PixelFormat(cfgPixFmt)) == fmts.end()) {
 			/* If we cannot find a native format, use a default one. */
-			cfgPixFmt = formats::NV12;
+			cfgPixFmt = formats::YUV420;
 			status = Adjusted;
 		}
 
@@ -431,7 +431,7 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
 
 		case StreamRole::StillCapture:
 			fmts = data->ispFormats();
-			pixelFormat = formats::NV12;
+			pixelFormat = formats::YUV420;
 			/*
 			 * Still image codecs usually expect the sYCC color space.
 			 * Even RGB codecs will be fine as the RGB we get with the
@@ -465,7 +465,7 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
 
 		case StreamRole::Viewfinder:
 			fmts = data->ispFormats();
-			pixelFormat = formats::ARGB8888;
+			pixelFormat = formats::XRGB8888;
 			colorSpace = ColorSpace::Sycc;
 			size = { 800, 600 };
 			bufferCount = 4;