[libcamera-devel,v2,1/6] pipeline: raspberrypi: Refactor stream configuration routine
diff mbox series

Message ID 20210122092537.708375-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Colour denoise
Related show

Commit Message

Naushir Patuck Jan. 22, 2021, 9:25 a.m. UTC 
Refactor the high/low resolution stream format and output selection
routine. This change is in preparation of adding 1/4 resolution output
for fast colour denoise.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
 1 file changed, 16 insertions(+), 41 deletions(-)

Comments

Kieran Bingham Jan. 22, 2021, 11:38 a.m. UTC | #1 
Hi Naush,

On 22/01/2021 09:25, Naushir Patuck wrote:
> Refactor the high/low resolution stream format and output selection
> routine. This change is in preparation of adding 1/4 resolution output
> for fast colour denoise.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
>  1 file changed, 16 insertions(+), 41 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 524cc960dd37..7e92f7669126 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  			continue;
>  		}
>  
> -		if (i == maxIndex) {
> -			/* ISP main output format. */
> -			V4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();
> -			V4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> -			format.size = cfg.size;
> -			format.fourcc = fourcc;
> -
> -			ret = dev->setFormat(&format);
> -			if (ret)
> -				return -EINVAL;
> -
> -			if (format.size != cfg.size || format.fourcc != fourcc) {
> -				LOG(RPI, Error)
> -					<< "Failed to set format on ISP capture0 device: "
> -					<< format.toString();
> -				return -EINVAL;
> -			}
> -
> -			cfg.setStream(&data->isp_[Isp::Output0]);
> -			data->isp_[Isp::Output0].setExternal(true);
> -		}
> +		/* The largest resolution gets routed to the ISP Output 0 node. */
> +		RPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0] :
> +							&data->isp_[Isp::Output1];

Only minor, I think Laurent usually prefers the : below the ? in that style.



>  
> -		/*
> -		 * ISP second output format. This fallthrough means that if a
> -		 * second output stream has not been configured, we simply use
> -		 * the Output0 configuration.
> -		 */
> -		V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
> -		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> +		V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
>  		format.size = cfg.size;
> +		format.fourcc = fourcc;
>  
> -		ret = dev->setFormat(&format);
> -		if (ret) {
> +		ret = stream->dev()->setFormat(&format);
> +		if (ret)
> +			return -EINVAL;
> +
> +		if (format.size != cfg.size || format.fourcc != fourcc) {
>  			LOG(RPI, Error)
> -				<< "Failed to set format on ISP capture1 device: "
> -				<< format.toString();
> -			return ret;
> -		}
> -		/*
> -		 * If we have not yet provided a stream for this config, it
> -		 * means this is to be routed from Output1.
> -		 */
> -		if (!cfg.stream()) {
> -			cfg.setStream(&data->isp_[Isp::Output1]);
> -			data->isp_[Isp::Output1].setExternal(true);
> +				<< "Failed to set format on " << stream->name()
> +				<< " to " << format.toString();

isn't format.toString() going to report what was set by setFormat(&format)?

(i.e. it's not going to be the correct format that requested to be set 'to'.




> +			return -EINVAL;
>  		}
> +
> +		cfg.setStream(stream);
> +		stream->setExternal(true);
>  	}
>  
>  	/* ISP statistics output format. */
>
Naushir Patuck Jan. 22, 2021, 11:57 a.m. UTC | #2 
Hi Kieran,

Thank you for your review feedback:

On Fri, 22 Jan 2021 at 11:38, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi Naush,
>
> On 22/01/2021 09:25, Naushir Patuck wrote:
> > Refactor the high/low resolution stream format and output selection
> > routine. This change is in preparation of adding 1/4 resolution output
> > for fast colour denoise.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
> >  1 file changed, 16 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 524cc960dd37..7e92f7669126 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                       continue;
> >               }
> >
> > -             if (i == maxIndex) {
> > -                     /* ISP main output format. */
> > -                     V4L2VideoDevice *dev =
> data->isp_[Isp::Output0].dev();
> > -                     V4L2PixelFormat fourcc =
> dev->toV4L2PixelFormat(cfg.pixelFormat);
> > -                     format.size = cfg.size;
> > -                     format.fourcc = fourcc;
> > -
> > -                     ret = dev->setFormat(&format);
> > -                     if (ret)
> > -                             return -EINVAL;
> > -
> > -                     if (format.size != cfg.size || format.fourcc !=
> fourcc) {
> > -                             LOG(RPI, Error)
> > -                                     << "Failed to set format on ISP
> capture0 device: "
> > -                                     << format.toString();
> > -                             return -EINVAL;
> > -                     }
> > -
> > -                     cfg.setStream(&data->isp_[Isp::Output0]);
> > -                     data->isp_[Isp::Output0].setExternal(true);
> > -             }
> > +             /* The largest resolution gets routed to the ISP Output 0
> node. */
> > +             RPi::Stream *stream = (i == maxIndex) ?
> &data->isp_[Isp::Output0] :
> > +
>  &data->isp_[Isp::Output1];
>
> Only minor, I think Laurent usually prefers the : below the ? in that
> style.
>

Ack.


>
>
>
> >
> > -             /*
> > -              * ISP second output format. This fallthrough means that
> if a
> > -              * second output stream has not been configured, we simply
> use
> > -              * the Output0 configuration.
> > -              */
> > -             V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
> > -             format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > +             V4L2PixelFormat fourcc =
> stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
> >               format.size = cfg.size;
> > +             format.fourcc = fourcc;
> >
> > -             ret = dev->setFormat(&format);
> > -             if (ret) {
> > +             ret = stream->dev()->setFormat(&format);
> > +             if (ret)
> > +                     return -EINVAL;
> > +
> > +             if (format.size != cfg.size || format.fourcc != fourcc) {
> >                       LOG(RPI, Error)
> > -                             << "Failed to set format on ISP capture1
> device: "
> > -                             << format.toString();
> > -                     return ret;
> > -             }
> > -             /*
> > -              * If we have not yet provided a stream for this config, it
> > -              * means this is to be routed from Output1.
> > -              */
> > -             if (!cfg.stream()) {
> > -                     cfg.setStream(&data->isp_[Isp::Output1]);
> > -                     data->isp_[Isp::Output1].setExternal(true);
> > +                             << "Failed to set format on " <<
> stream->name()
> > +                             << " to " << format.toString();
>
> isn't format.toString() going to report what was set by setFormat(&format)?
>
> (i.e. it's not going to be the correct format that requested to be set
> 'to'.
>

Yes, you are right.  I ought to change the message text to "Failed to get
requested format, returned: " ... as I log the requested format earlier.

Regards,
Naush


>
>
>
>
> > +                     return -EINVAL;
> >               }
> > +
> > +             cfg.setStream(stream);
> > +             stream->setExternal(true);
> >       }
> >
> >       /* ISP statistics output format. */
> >
>
> --
> Regards
> --
> Kieran
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 524cc960dd37..7e92f7669126 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -624,52 +624,27 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 			continue;
 		}
 
-		if (i == maxIndex) {
-			/* ISP main output format. */
-			V4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();
-			V4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
-			format.size = cfg.size;
-			format.fourcc = fourcc;
-
-			ret = dev->setFormat(&format);
-			if (ret)
-				return -EINVAL;
-
-			if (format.size != cfg.size || format.fourcc != fourcc) {
-				LOG(RPI, Error)
-					<< "Failed to set format on ISP capture0 device: "
-					<< format.toString();
-				return -EINVAL;
-			}
-
-			cfg.setStream(&data->isp_[Isp::Output0]);
-			data->isp_[Isp::Output0].setExternal(true);
-		}
+		/* The largest resolution gets routed to the ISP Output 0 node. */
+		RPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0] :
+							&data->isp_[Isp::Output1];
 
-		/*
-		 * ISP second output format. This fallthrough means that if a
-		 * second output stream has not been configured, we simply use
-		 * the Output0 configuration.
-		 */
-		V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
-		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
+		V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
 		format.size = cfg.size;
+		format.fourcc = fourcc;
 
-		ret = dev->setFormat(&format);
-		if (ret) {
+		ret = stream->dev()->setFormat(&format);
+		if (ret)
+			return -EINVAL;
+
+		if (format.size != cfg.size || format.fourcc != fourcc) {
 			LOG(RPI, Error)
-				<< "Failed to set format on ISP capture1 device: "
-				<< format.toString();
-			return ret;
-		}
-		/*
-		 * If we have not yet provided a stream for this config, it
-		 * means this is to be routed from Output1.
-		 */
-		if (!cfg.stream()) {
-			cfg.setStream(&data->isp_[Isp::Output1]);
-			data->isp_[Isp::Output1].setExternal(true);
+				<< "Failed to set format on " << stream->name()
+				<< " to " << format.toString();
+			return -EINVAL;
 		}
+
+		cfg.setStream(stream);
+		stream->setExternal(true);
 	}
 
 	/* ISP statistics output format. */