[libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign sizes to roles
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 21 00:28:02 CET 2023
Hi Jacopo,
On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > > Currently each RkISP1 path (main and self) generate a configuration
> > > by bounding the sensor's resolution to their respective maximum output
> > > aspect ratio and size.
> > >
> > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > > .boundedTo(resolution);
> > >
> > > In the case of self path, whose maximum size is 1920x1920, the generated
> > > configuration could get assigned unusual sizes, as the result of the
> > > above operation
> >
> > s/$/./
> >
> > > As an example, with the imx258 sensor whose resolution is 4208x3118, the
> > > resulting size for the Viewfinder use case is an unusual 1920x1423.
> > >
> > > Fix this by assigning to each role a desired output size:
> > > - Use the sensor's resolution for StillCapture and Raw
> > > - Use 1080p for Viewfinder and VideoRecording
> >
> > Will the pipeline handler crop the full sensor resolution to 4096x2304
> > before scaling it down to 1080p, or will it just scale ? In the latter
> > case, the pixel ratio won't be square, which isn't good.
>
> I'm not sure I agree this isn't good. If we struggle to maintain the
> sensor's aspect ratio we get:
> 1) weird and unusual sizes like the above reported 1920x1423
That's not great indeed.
> 2) different results depending on which sensor is connected
That's to be expected, as sensors have different capabilities :-)
> I think a known 1080p size is a much more predictable behavior
> for applications, regardless of the aspect ratio.
Unless the application explicitly requests a non-square pixel ratio,
defaulting to 1920x1080 with non-square pixels *really* bad. There are
multiple options in this case, starting from a 4208x3118 sensor
resolution:
- Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect
ratio
- Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect
ratio
Both will produce square pixels.
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +++++++++++-
> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--
> > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 1 +
> > > 3 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index ebc9bef8688a..60ab7380034c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -164,6 +164,8 @@ public:
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > private:
> > > + static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> > > +
> > > RkISP1CameraData *cameraData(Camera *camera)
> > > {
> > > return static_cast<RkISP1CameraData *>(camera->_d());
> > > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > >
> > > for (const StreamRole role : roles) {
> > > bool useMainPath = mainPathAvailable;
> > > + Size size;
> > >
> > > switch (role) {
> > > case StreamRole::StillCapture:
> > > /* JPEG encoders typically expect sYCC. */
> > > if (!colorSpace)
> > > colorSpace = ColorSpace::Sycc;
> > > +
> > > + size = data->sensor_->resolution();
> > > break;
> > >
> > > case StreamRole::Viewfinder:
> > > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > */
> > > if (!colorSpace)
> > > colorSpace = ColorSpace::Sycc;
> > > +
> > > + size = kRkISP1PreviewSize;
> > > break;
> > >
> > > case StreamRole::VideoRecording:
> > > /* Rec. 709 is a good default for HD video recording. */
> > > if (!colorSpace)
> > > colorSpace = ColorSpace::Rec709;
> > > +
> > > + size = kRkISP1PreviewSize;
> > > break;
> > >
> > > case StreamRole::Raw:
> > > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > }
> > >
> > > colorSpace = ColorSpace::Raw;
> > > + size = data->sensor_->resolution();
> > > break;
> > >
> > > default:
> > > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > }
> > >
> > > StreamConfiguration cfg =
> > > - path->generateConfiguration(data->sensor_.get(), role);
> > > + path->generateConfiguration(data->sensor_.get(), size, role);
> > > if (!cfg.pixelFormat.isValid())
> > > return nullptr;
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > index 5079b268c464..a27ac6fc35cb 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()
> > > }
> > >
> > > StreamConfiguration
> > > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> > > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,
> > > + const Size &size,
> > > + StreamRole role)
> >
> > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> > StreamRole role)
> >
> > > {
> > > const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > > const Size &resolution = sensor->resolution();
> > >
> > > + /* Min and max resolutions to populate the available stream formats. */
> > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > > .boundedTo(resolution);
> > > Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> > >
> > > + /* The desired stream size, bound to the max resolution. */
> > > + Size streamSize = size.boundedTo(maxResolution);
> > > +
> > > /* Create the list of supported stream formats. */
> > > std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > > unsigned int rawBitsPerPixel = 0;
> > > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> > > StreamFormats formats(streamFormats);
> > > StreamConfiguration cfg(formats);
> > > cfg.pixelFormat = format;
> > > - cfg.size = maxResolution;
> > > + cfg.size = streamSize;
> > > cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > >
> > > return cfg;
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > index bdf3f95b95e1..cd77957ee4a6 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > @@ -41,6 +41,7 @@ public:
> > > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> > >
> > > StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> > > + const Size &resolution,
> > > StreamRole role);
> > > CameraConfiguration::Status validate(const CameraSensor *sensor,
> > > StreamConfiguration *cfg);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list