Message ID | 20210120083449.642418-3-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Naush Thanks for this patch. Just one small tidiness thing... On Wed, 20 Jan 2021 at 08:34, 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 | 42 ++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index e03bcb036f9f..282c4ba75d89 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,42 @@ 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; > + > + output1Format.size = format.size / 2; > + if (output1Format.size.width > maxDimensions) { > + output1Format.size.height = maxDimensions * output1Format.size.height / > + output1Format.size.width; > + output1Format.size.height &= ~1; > + output1Format.size.width = maxDimensions; > + } else if (output1Format.size.height > maxDimensions) { > + output1Format.size.width = maxDimensions * output1Format.size.width / > + output1Format.size.height; > + output1Format.size.width &= ~1; > + output1Format.size.height = maxDimensions; > + } I wonder if we can tidy this up with those new(ish) Size methods? Maybe along the lines of const Size maxSize = Size(maxDimensions , maxDimensions).boundedToAspectRatio(format.size); output1Format.size = (format.size / 2).boundedTo(maxSize).alignedDownTo(2, 2); Thanks! David > + > + LOG(RPI, Info) << "Setting ISP Output 1 (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 David, Thank you for your review feedback. On Wed, 20 Jan 2021 at 09:53, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch. Just one small tidiness thing... > > On Wed, 20 Jan 2021 at 08:34, 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 | 42 ++++++++++++++++++- > > 1 file changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index e03bcb036f9f..282c4ba75d89 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,42 @@ 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; > > + > > + output1Format.size = format.size / 2; > > + if (output1Format.size.width > maxDimensions) { > > + output1Format.size.height = maxDimensions * > output1Format.size.height / > > + > output1Format.size.width; > > + output1Format.size.height &= ~1; > > + output1Format.size.width = maxDimensions; > > + } else if (output1Format.size.height > maxDimensions) { > > + output1Format.size.width = maxDimensions * > output1Format.size.width / > > + > output1Format.size.height; > > + output1Format.size.width &= ~1; > > + output1Format.size.height = maxDimensions; > > + } > > I wonder if we can tidy this up with those new(ish) Size methods? > Maybe along the lines of > > const Size maxSize = Size(maxDimensions , > maxDimensions).boundedToAspectRatio(format.size); > output1Format.size = (format.size / 2).boundedTo(maxSize).alignedDownTo(2, > 2); > Ooh, that is much nicer! Have to admit, I didn't much explore the Size class to see if this could be done. I will make the suggested change on the next revision. Regards, Naush > > Thanks! > David > > > + > > + LOG(RPI, Info) << "Setting ISP Output 1 (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 >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index e03bcb036f9f..282c4ba75d89 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,42 @@ 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; + + output1Format.size = format.size / 2; + if (output1Format.size.width > maxDimensions) { + output1Format.size.height = maxDimensions * output1Format.size.height / + output1Format.size.width; + output1Format.size.height &= ~1; + output1Format.size.width = maxDimensions; + } else if (output1Format.size.height > maxDimensions) { + output1Format.size.width = maxDimensions * output1Format.size.width / + output1Format.size.height; + output1Format.size.width &= ~1; + output1Format.size.height = maxDimensions; + } + + LOG(RPI, Info) << "Setting ISP Output 1 (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 | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)