<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:42, 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>
> In preparation for fast colour denoise, set the low resolution ISP<br>
> output stream (Output1) to a 1/4 resolution of the application requested<br>
> stream (Output0). This only happens if the application has not requested<br>
> an additional YUV or RGB stream.<br>
> <br>
> We also constrain this 1/4 resolution to at most 1200 pixels in the<br>
> largest dimension to avoid being too taxing on memory usage and system<br>
> bandwidth.<br>
> <br>
> Also switch the default StreamRole::VideoRecording to YUV420 to allow<br>
> fast colour denoise to run.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 32 ++++++++++++++++++-<br>
>  1 file changed, 31 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 7e92f7669126..a0d3ac7ce49f 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
>  <br>
>               case StreamRole::VideoRecording:<br>
>                       fmts = data->isp_[Isp::Output0].dev()->formats();<br>
> -                     pixelFormat = formats::NV12;<br>
> +                     pixelFormat = formats::YUV420;<br>
>                       size = { 1920, 1080 };<br>
>                       bufferCount = 4;<br>
>                       outCount++;<br>
> @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>        * StreamConfiguration appropriately.<br>
>        */<br>
>       V4L2DeviceFormat format;<br>
> +     bool output1Set = false;<br>
>       for (unsigned i = 0; i < config->size(); i++) {<br>
>               StreamConfiguration &cfg = config->at(i);<br>
>  <br>
> @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>               format.size = cfg.size;<br>
>               format.fourcc = fourcc;<br>
>  <br>
> +             LOG(RPI, Info) << "Setting " << stream->name() << " to a resolution of "<br>
> +                            << format.toString();<br>
> +<br>
>               ret = stream->dev()->setFormat(&format);<br>
>               if (ret)<br>
>                       return -EINVAL;<br>
> @@ -645,6 +649,32 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
>  <br>
>               cfg.setStream(stream);<br>
>               stream->setExternal(true);<br>
> +<br>
> +             if (i != maxIndex)<br>
> +                     output1Set = true;<br>
> +     }<br>
> +<br>
> +     /*<br>
> +      * If ISP::Output1 stream has not been requested by the application, we<br>
> +      * set it up for internal use now. This second stream will be used for<br>
> +      * fast colour denoise, and must be a quarter resolution of the ISP::Output0<br>
> +      * stream. However, also limit the maximum size to 1200 pixels in the<br>
> +      * larger dimension, just to avoid being wasteful with buffer allocations<br>
> +      * and memory bandwidth.<br>
> +      */<br>
> +     if (!output1Set) {<br>
> +             V4L2DeviceFormat output1Format = format;<br>
> +             constexpr unsigned int maxDimensions = 1200;<br>
> +             const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size);<br>
> +<br>
> +             output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);<br>
> +<br>
> +             LOG(RPI, Info) << "Setting ISP Output1 (internal) to a resolution of "<br>
> +                            << output1Format.toString();<br>
> +<br>
> +             ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format);<br>
> +             if (ret)<br>
<br>
Should there be a print here if this happens? Or is it never really<br>
going to happen?<br></blockquote><div><br></div><div>It is possible to have this so I can add a log message (like for the previous case).</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>
Otherwise:<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> +                     return -EINVAL;<br>
>       }<br>
>  <br>
>       /* ISP statistics output format. */<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>