<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>