[libcamera-devel] [RFC PATCH] libcamera: pipeline: vimc: Skip unsupported formats

[Kieran Bingham]

Hi Kaaira,

On 26/05/2020 14:25, Kaaira Gupta wrote:
> On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> On 22/05/2020 21:37, Laurent Pinchart wrote:
>>> Hi Kieran,
>>>
>>> Thank you for the patch.
>>>
>>> On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote:
>>>> Older kernels do not support all 'reported' formats. Skip them on those
>>>> kernels.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> ---
>>>> Marking this as RFC, as I don't actually yet know what the correct
>>>> kernel version is to condition against.
>>>
>>> I think it will be v5.9 if we're lucky :-)
>>
>>
>> I think some progress has been made for 5.7, but I don't know the
>> current state. Kaaira might know more here.
> 
> Hi! Support for other formats has been added to vimc, but it is a bit
> buggy right now as it swaps red and blue channels in the final image. I
> have sent a patch to fix that, as soon as it gets accepted, libcamera
> can support them as well for newer kernel versions.


I've just managed to test my jpeg encoder with BGR888 and RGB888 and
both of those came out with good images.

Then I ran QCam using the virtme configuration you sent me - and indeed
I see the colour inversions. But I suspect that could be due to qcam now.

On mainline (v5.7-rc) I can successfully capture BGR888 and RGB888
(RG24|BG24) but I can not configure a BGRA8888 BA24 pipeline yet.


>>>> Currently, the QCam application can not stream the VIMC Camera for users
>>>> of QT Version less that 5.14 as that is when the BGR888 format becomes a
>>>> 'preferred' native format.
>>>>
>>>> Users of QT < 5.14 will find that qcam will chose a 'preferred' format
>>>> which is reported as supported by the kernel, when in fact it isn't.
>>>>
>>>> Ensure that those formats are not reported as supported by the vimc
>>>> pipeline handler on older kernels.
>>>>
>>>>  src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>>>> index 68d65bc785b0..78fb0ece21f8 100644
>>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>>>> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>>>>  	const StreamRoles &roles)
>>>>  {
>>>>  	CameraConfiguration *config = new VimcCameraConfiguration();
>>>> +	VimcCameraData *data = cameraData(camera);
>>>>  
>>>>  	if (roles.empty())
>>>>  		return config;
>>>> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>>>>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>>>>  
>>>>  	for (const auto &pixelformat : pixelformats) {
>>>> +		/*
>>>> +		 * \todo: Update this when the kernel correctly supports other
>>>> +		 * reported formats.
>>>> +		 */
>>>> +		if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) {
>>>
>>> I wouldn't hardcode any kernel version until we know when the issue will
>>> be fixed. I would remove the unsupported formats from the pixelformats
>>> map now, and deal with versioning later.
>>
>> Ok, well that will depend upon the current mainline state.
>>
>>
>>>> +			if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) {
>>>> +				LOG(VIMC, Info)
>>>> +					<< "Skipping unsupported pixel format"
>>>> +					<< pixelformat.first.toString();
>>>> +				continue;
>>>> +			}
>>>> +		}
>>>> +
>>>
>>> You also need to patch the VimcCameraConfiguration::validate() and
>>
>> I started at validate in fact, but couldn't (quickly) get the CameraData
>>
>>> PipelineHandlerVimc::configure() functions, as they both use the
>>> pixelformats map. One option would be to use
>>> StreamConfiguration::formats().pixelformats() in both locations, so only
>>> one location would need to deal with kernel versions.
>>
>> That makes sense in validate(), but not configure() (which should have
>> already been validated()).
>>
>> (Context, this is based upon a branch with your patch applied to provide
>> the PixelFormat -> MBUS code mapping)
>>
>>>
>>>>  		/* The scaler hardcodes a x3 scale-up ratio. */
>>>>  		std::vector<SizeRange> sizes{
>>>>  			SizeRange{ { 48, 48 }, { 4096, 2160 } }
>>>
>>
>> -- 
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran