[libcamera-devel] [PATCH] libcamera: pipeline: raspberrypi: Add StreamFormats to StreamConfiguration

[Kieran Bingham]

Hi Naush, Jacopo,

On 18/06/2020 09:41, Jacopo Mondi wrote:
> Hi Naush,
>    sorry for the delay, this fell through the cracks
> 
> On Wed, Jun 10, 2020 at 03:26:40PM +0100, Naushir Patuck wrote:
>> In generateConfiguration(), add the device node specific formats to the
>> StreamConfiguration for each StreamRole requested.
>>
>> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>> ---
>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 52 +++++++++++++------
>>  1 file changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index e16a9c7f..03a1e641 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -518,41 +518,45 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>>  	RPiCameraData *data = cameraData(camera);
>>  	CameraConfiguration *config = new RPiCameraConfiguration(data);
>>  	V4L2DeviceFormat sensorFormat;
>> +	unsigned int bufferCount;
>> +	PixelFormat pixelFormat;
>>  	V4L2PixFmtMap fmts;
>> +	Size size;
>>
>>  	if (roles.empty())
>>  		return config;
>>
>>  	for (const StreamRole role : roles) {
>> -		StreamConfiguration cfg{};
>> -
>>  		switch (role) {
>>  		case StreamRole::StillCaptureRaw:
>> -			cfg.size = data->sensor_->resolution();
>> +			size = data->sensor_->resolution();
>>  			fmts = data->unicam_[Unicam::Image].dev()->formats();
>> -			sensorFormat = findBestMode(fmts, cfg.size);
>> -			cfg.pixelFormat = sensorFormat.fourcc.toPixelFormat();
>> -			ASSERT(cfg.pixelFormat.isValid());
>> -			cfg.bufferCount = 1;
>> +			sensorFormat = findBestMode(fmts, size);
>> +			pixelFormat = sensorFormat.fourcc.toPixelFormat();
>> +			ASSERT(pixelFormat.isValid());
>> +			bufferCount = 1;
>>  			break;
>>
>>  		case StreamRole::StillCapture:
>> -			cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>> +			fmts = data->isp_[Isp::Output0].dev()->formats();
>> +			pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>>  			/* Return the largest sensor resolution. */
>> -			cfg.size = data->sensor_->resolution();
>> -			cfg.bufferCount = 1;
>> +			size = data->sensor_->resolution();
>> +			bufferCount = 1;
>>  			break;
>>
>>  		case StreamRole::VideoRecording:
>> -			cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>> -			cfg.size = { 1920, 1080 };
>> -			cfg.bufferCount = 4;
>> +			fmts = data->isp_[Isp::Output0].dev()->formats();
>> +			pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>> +			size = { 1920, 1080 };
>> +			bufferCount = 4;
>>  			break;
>>
>>  		case StreamRole::Viewfinder:
>> -			cfg.pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);
>> -			cfg.size = { 800, 600 };
>> -			cfg.bufferCount = 4;
>> +			fmts = data->isp_[Isp::Output0].dev()->formats();
>> +			pixelFormat = PixelFormat(DRM_FORMAT_ARGB8888);
>> +			size = { 800, 600 };
>> +			bufferCount = 4;
>>  			break;
>>
>>  		default:
>> @@ -561,6 +565,22 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>>  			break;
>>  		}
>>
>> +		/* Translate the V4L2PixelFormat to PixelFormat. */
>> +		std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
>> +		std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.begin()),
>> +			       [&](const decltype(fmts)::value_type &format) {
>> +					return decltype(deviceFormats)::value_type{
>> +						format.first.toPixelFormat(),
>> +						format.second
>> +					};
>> +			       });
> 
> Really took me a while to parse this, as I was not expecting to see
> std::inserter() but just deviceFormats.begin() there, but then I
> learned about inserter, and indeed if I remove std::inserter() I get
> an error due to the fact the copy operator of std::pair is deleted.

I think that's the same as the conversion routine in the UVC pipeline
handler (which I think came from Laurent).

Not for this patch (I think this can go in as is) but we should really
make a helper for that conversion as it's likely to be used in multiple
places, and it is quite hard to parse on it's own ;-(.

However, I think that overlaps with changes that you (Jacopo) were
working on with ImageFormats anyway.

Eitherway, this patch will help support more formats and enumeration on
the RPi pipeline handler:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 
>> +
>> +		/* Add the stream format based on the device node used for the use case. */
>> +		StreamFormats formats(deviceFormats);
>> +		StreamConfiguration cfg(formats);
>> +		cfg.size = size;
>> +		cfg.pixelFormat = pixelFormat;
>> +		cfg.bufferCount = bufferCount;
> 
> Patch looks good to me
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>   j
> 
>>  		config->addConfiguration(cfg);
>>  	}
>>
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran