<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review feedback:</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 22 Jan 2021 at 11:38, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 22/01/2021 09:25, Naushir Patuck wrote:<br>
> Refactor the high/low resolution stream format and output selection<br>
> routine. This change is in preparation of adding 1/4 resolution output<br>
> for fast colour denoise.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------<br>
>  1 file changed, 16 insertions(+), 41 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 524cc960dd37..7e92f7669126 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>                       continue;<br>
>               }<br>
>  <br>
> -             if (i == maxIndex) {<br>
> -                     /* ISP main output format. */<br>
> -                     V4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();<br>
> -                     V4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);<br>
> -                     format.size = cfg.size;<br>
> -                     format.fourcc = fourcc;<br>
> -<br>
> -                     ret = dev->setFormat(&format);<br>
> -                     if (ret)<br>
> -                             return -EINVAL;<br>
> -<br>
> -                     if (format.size != cfg.size || format.fourcc != fourcc) {<br>
> -                             LOG(RPI, Error)<br>
> -                                     << "Failed to set format on ISP capture0 device: "<br>
> -                                     << format.toString();<br>
> -                             return -EINVAL;<br>
> -                     }<br>
> -<br>
> -                     cfg.setStream(&data->isp_[Isp::Output0]);<br>
> -                     data->isp_[Isp::Output0].setExternal(true);<br>
> -             }<br>
> +             /* The largest resolution gets routed to the ISP Output 0 node. */<br>
> +             RPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0] :<br>
> +                                                     &data->isp_[Isp::Output1];<br>
<br>
Only minor, I think Laurent usually prefers the : below the ? in that style.<br></blockquote><div><br></div><div>Ack.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
>  <br>
> -             /*<br>
> -              * ISP second output format. This fallthrough means that if a<br>
> -              * second output stream has not been configured, we simply use<br>
> -              * the Output0 configuration.<br>
> -              */<br>
> -             V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();<br>
> -             format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);<br>
> +             V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);<br>
>               format.size = cfg.size;<br>
> +             format.fourcc = fourcc;<br>
>  <br>
> -             ret = dev->setFormat(&format);<br>
> -             if (ret) {<br>
> +             ret = stream->dev()->setFormat(&format);<br>
> +             if (ret)<br>
> +                     return -EINVAL;<br>
> +<br>
> +             if (format.size != cfg.size || format.fourcc != fourcc) {<br>
>                       LOG(RPI, Error)<br>
> -                             << "Failed to set format on ISP capture1 device: "<br>
> -                             << format.toString();<br>
> -                     return ret;<br>
> -             }<br>
> -             /*<br>
> -              * If we have not yet provided a stream for this config, it<br>
> -              * means this is to be routed from Output1.<br>
> -              */<br>
> -             if (!cfg.stream()) {<br>
> -                     cfg.setStream(&data->isp_[Isp::Output1]);<br>
> -                     data->isp_[Isp::Output1].setExternal(true);<br>
> +                             << "Failed to set format on " << stream->name()<br>
> +                             << " to " << format.toString();<br>
<br>
isn't format.toString() going to report what was set by setFormat(&format)?<br>
<br>
(i.e. it's not going to be the correct format that requested to be set 'to'.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
<br>
> +                     return -EINVAL;<br>
>               }<br>
> +<br>
> +             cfg.setStream(stream);<br>
> +             stream->setExternal(true);<br>
>       }<br>
>  <br>
>       /* ISP statistics output format. */<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>