| Message ID | 20210122092537.708375-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the revised version of this patch. On Fri, 22 Jan 2021 at 09:25, Naushir Patuck <naush@raspberrypi.com> wrote: > > In preparation for fast colour denoise, set the low resolution ISP > output stream (Output1) to a 1/4 resolution of the application requested > stream (Output0). This only happens if the application has not requested > an additional YUV or RGB stream. > > We also constrain this 1/4 resolution to at most 1200 pixels in the > largest dimension to avoid being too taxing on memory usage and system > bandwidth. > > Also switch the default StreamRole::VideoRecording to YUV420 to allow > fast colour denoise to run. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 32 ++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7e92f7669126..a0d3ac7ce49f 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > case StreamRole::VideoRecording: > fmts = data->isp_[Isp::Output0].dev()->formats(); > - pixelFormat = formats::NV12; > + pixelFormat = formats::YUV420; > size = { 1920, 1080 }; > bufferCount = 4; > outCount++; > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > * StreamConfiguration appropriately. > */ > V4L2DeviceFormat format; > + bool output1Set = false; > for (unsigned i = 0; i < config->size(); i++) { > StreamConfiguration &cfg = config->at(i); > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > format.size = cfg.size; > format.fourcc = fourcc; > > + LOG(RPI, Info) << "Setting " << stream->name() << " to a resolution of " > + << format.toString(); > + > ret = stream->dev()->setFormat(&format); > if (ret) > return -EINVAL; > @@ -645,6 +649,32 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > cfg.setStream(stream); > stream->setExternal(true); > + > + if (i != maxIndex) > + output1Set = true; > + } > + > + /* > + * If ISP::Output1 stream has not been requested by the application, we > + * set it up for internal use now. This second stream will be used for > + * fast colour denoise, and must be a quarter resolution of the ISP::Output0 > + * stream. However, also limit the maximum size to 1200 pixels in the > + * larger dimension, just to avoid being wasteful with buffer allocations > + * and memory bandwidth. > + */ > + if (!output1Set) { > + V4L2DeviceFormat output1Format = format; > + constexpr unsigned int maxDimensions = 1200; > + const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size); > + > + output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2); Yes, doesn't it look lovely now! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > + > + LOG(RPI, Info) << "Setting ISP Output1 (internal) to a resolution of " > + << output1Format.toString(); > + > + ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format); > + if (ret) > + return -EINVAL; > } > > /* ISP statistics output format. */ > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On 22/01/2021 09:25, Naushir Patuck wrote: > In preparation for fast colour denoise, set the low resolution ISP > output stream (Output1) to a 1/4 resolution of the application requested > stream (Output0). This only happens if the application has not requested > an additional YUV or RGB stream. > > We also constrain this 1/4 resolution to at most 1200 pixels in the > largest dimension to avoid being too taxing on memory usage and system > bandwidth. > > Also switch the default StreamRole::VideoRecording to YUV420 to allow > fast colour denoise to run. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 32 ++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7e92f7669126..a0d3ac7ce49f 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > case StreamRole::VideoRecording: > fmts = data->isp_[Isp::Output0].dev()->formats(); > - pixelFormat = formats::NV12; > + pixelFormat = formats::YUV420; > size = { 1920, 1080 }; > bufferCount = 4; > outCount++; > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > * StreamConfiguration appropriately. > */ > V4L2DeviceFormat format; > + bool output1Set = false; > for (unsigned i = 0; i < config->size(); i++) { > StreamConfiguration &cfg = config->at(i); > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > format.size = cfg.size; > format.fourcc = fourcc; > > + LOG(RPI, Info) << "Setting " << stream->name() << " to a resolution of " > + << format.toString(); > + > ret = stream->dev()->setFormat(&format); > if (ret) > return -EINVAL; > @@ -645,6 +649,32 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > cfg.setStream(stream); > stream->setExternal(true); > + > + if (i != maxIndex) > + output1Set = true; > + } > + > + /* > + * If ISP::Output1 stream has not been requested by the application, we > + * set it up for internal use now. This second stream will be used for > + * fast colour denoise, and must be a quarter resolution of the ISP::Output0 > + * stream. However, also limit the maximum size to 1200 pixels in the > + * larger dimension, just to avoid being wasteful with buffer allocations > + * and memory bandwidth. > + */ > + if (!output1Set) { > + V4L2DeviceFormat output1Format = format; > + constexpr unsigned int maxDimensions = 1200; > + const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size); > + > + output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2); > + > + LOG(RPI, Info) << "Setting ISP Output1 (internal) to a resolution of " > + << output1Format.toString(); > + > + ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format); > + if (ret) Should there be a print here if this happens? Or is it never really going to happen? Otherwise: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + return -EINVAL; > } > > /* ISP statistics output format. */ >
Hi Kieran, Thank you for your review feedback. On Fri, 22 Jan 2021 at 11:42, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 22/01/2021 09:25, Naushir Patuck wrote: > > In preparation for fast colour denoise, set the low resolution ISP > > output stream (Output1) to a 1/4 resolution of the application requested > > stream (Output0). This only happens if the application has not requested > > an additional YUV or RGB stream. > > > > We also constrain this 1/4 resolution to at most 1200 pixels in the > > largest dimension to avoid being too taxing on memory usage and system > > bandwidth. > > > > Also switch the default StreamRole::VideoRecording to YUV420 to allow > > fast colour denoise to run. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 32 ++++++++++++++++++- > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7e92f7669126..a0d3ac7ce49f 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -496,7 +496,7 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > > case StreamRole::VideoRecording: > > fmts = data->isp_[Isp::Output0].dev()->formats(); > > - pixelFormat = formats::NV12; > > + pixelFormat = formats::YUV420; > > size = { 1920, 1080 }; > > bufferCount = 4; > > outCount++; > > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > * StreamConfiguration appropriately. > > */ > > V4L2DeviceFormat format; > > + bool output1Set = false; > > for (unsigned i = 0; i < config->size(); i++) { > > StreamConfiguration &cfg = config->at(i); > > > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > format.size = cfg.size; > > format.fourcc = fourcc; > > > > + LOG(RPI, Info) << "Setting " << stream->name() << " to a > resolution of " > > + << format.toString(); > > + > > ret = stream->dev()->setFormat(&format); > > if (ret) > > return -EINVAL; > > @@ -645,6 +649,32 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > > > cfg.setStream(stream); > > stream->setExternal(true); > > + > > + if (i != maxIndex) > > + output1Set = true; > > + } > > + > > + /* > > + * If ISP::Output1 stream has not been requested by the > application, we > > + * set it up for internal use now. This second stream will be used > for > > + * fast colour denoise, and must be a quarter resolution of the > ISP::Output0 > > + * stream. However, also limit the maximum size to 1200 pixels in > the > > + * larger dimension, just to avoid being wasteful with buffer > allocations > > + * and memory bandwidth. > > + */ > > + if (!output1Set) { > > + V4L2DeviceFormat output1Format = format; > > + constexpr unsigned int maxDimensions = 1200; > > + const Size limit = Size(maxDimensions, > maxDimensions).boundedToAspectRatio(format.size); > > + > > + output1Format.size = (format.size / > 2).boundedTo(limit).alignedDownTo(2, 2); > > + > > + LOG(RPI, Info) << "Setting ISP Output1 (internal) to a > resolution of " > > + << output1Format.toString(); > > + > > + ret = > data->isp_[Isp::Output1].dev()->setFormat(&output1Format); > > + if (ret) > > Should there be a print here if this happens? Or is it never really > going to happen? > It is possible to have this so I can add a log message (like for the previous case). Regards, Naush > Otherwise: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + return -EINVAL; > > } > > > > /* ISP statistics output format. */ > > > > -- > Regards > -- > Kieran >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 7e92f7669126..a0d3ac7ce49f 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, case StreamRole::VideoRecording: fmts = data->isp_[Isp::Output0].dev()->formats(); - pixelFormat = formats::NV12; + pixelFormat = formats::YUV420; size = { 1920, 1080 }; bufferCount = 4; outCount++; @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) * StreamConfiguration appropriately. */ V4L2DeviceFormat format; + bool output1Set = false; for (unsigned i = 0; i < config->size(); i++) { StreamConfiguration &cfg = config->at(i); @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) format.size = cfg.size; format.fourcc = fourcc; + LOG(RPI, Info) << "Setting " << stream->name() << " to a resolution of " + << format.toString(); + ret = stream->dev()->setFormat(&format); if (ret) return -EINVAL; @@ -645,6 +649,32 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) cfg.setStream(stream); stream->setExternal(true); + + if (i != maxIndex) + output1Set = true; + } + + /* + * If ISP::Output1 stream has not been requested by the application, we + * set it up for internal use now. This second stream will be used for + * fast colour denoise, and must be a quarter resolution of the ISP::Output0 + * stream. However, also limit the maximum size to 1200 pixels in the + * larger dimension, just to avoid being wasteful with buffer allocations + * and memory bandwidth. + */ + if (!output1Set) { + V4L2DeviceFormat output1Format = format; + constexpr unsigned int maxDimensions = 1200; + const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size); + + output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2); + + LOG(RPI, Info) << "Setting ISP Output1 (internal) to a resolution of " + << output1Format.toString(); + + ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format); + if (ret) + return -EINVAL; } /* ISP statistics output format. */
In preparation for fast colour denoise, set the low resolution ISP output stream (Output1) to a 1/4 resolution of the application requested stream (Output0). This only happens if the application has not requested an additional YUV or RGB stream. We also constrain this 1/4 resolution to at most 1200 pixels in the largest dimension to avoid being too taxing on memory usage and system bandwidth. Also switch the default StreamRole::VideoRecording to YUV420 to allow fast colour denoise to run. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)