[libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use appropriate media bus format

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-03-24 15:12:47 +0200, Laurent Pinchart wrote:
> Pick the correct media bus format based on the video pixel format on the
> capture node.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/vimc.cpp | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> This patch, while being correct (I think :-)), shouldn't be integrated,
> as it breaks VIMC camera support with Qt < 5.14.0. On 5.14.0, the
> viewfinder reports the following supported native formats (in that
> order):
> 
> - DRM_FORMAT_ABGR8888
> - DRM_FORMAT_ARGB8888
> - DRM_FORMAT_BGR888
> - DRM_FORMAT_RGB888
> 
> The first two formats are not supported by the VIMC pipeline handler,
> the last two are. The third format, DRM_FORMAT_BGR888, is thus picked,
> which corresponds to MEDIA_BUS_FMT_RGB888_1X24, the default today.
> 
> On Qt < 5.14.0, the third format isn't supported, so the fourth format,
> DRM_FORMAT_RGB888, will be picked. This results in a different pipeline
> configuration, with the debayering subdev source pad being configured
> with MEDIA_BUS_FMT_BGR888_1X24. The kernel driver is meant to support
> that, but in drivers/media/platform/vimc/vimc-debayer.c the
> vimc_deb_set_fmt() function handles the source pad format with
> 
> 	/*
> 	 * Do not change the format of the source pad,
> 	 * it is propagated from the sink
> 	 */
> 	if (VIMC_IS_SRC(fmt->pad)) {
> 		fmt->format = *sink_fmt;
> 		/* TODO: Add support for other formats */
> 		fmt->format.code = vdeb->src_code;
> 	}
> 
> This needs to be fixed on the kernel side.
> 
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index b04a9726efa5..ace58381fe92 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -6,8 +6,8 @@
>   */
>  
>  #include <algorithm>
> -#include <array>
>  #include <iomanip>
> +#include <map>
>  #include <tuple>
>  
>  #include <linux/media-bus-format.h>
> @@ -103,10 +103,10 @@ private:
>  
>  namespace {
>  
> -static const std::array<PixelFormat, 3> pixelformats{
> -	PixelFormat(DRM_FORMAT_RGB888),
> -	PixelFormat(DRM_FORMAT_BGR888),
> -	PixelFormat(DRM_FORMAT_BGRA8888),
> +static const std::map<PixelFormat, uint32_t> pixelformats{
> +	{ PixelFormat(DRM_FORMAT_RGB888), MEDIA_BUS_FMT_BGR888_1X24 },
> +	{ PixelFormat(DRM_FORMAT_BGR888), MEDIA_BUS_FMT_RGB888_1X24 },
> +	{ PixelFormat(DRM_FORMAT_BGRA8888), MEDIA_BUS_FMT_ARGB8888_1X32 },

This looks odd BGRA8888 mapping to ARGB8888, should that not be 
DRM_FORMAT_ABGR8888? Also should the 4th bus format listed in the commit 
message be added to this map?

>  };
>  
>  } /* namespace */
> @@ -132,8 +132,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>  	StreamConfiguration &cfg = config_[0];
>  
>  	/* Adjust the pixel format. */
> -	if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==
> -	    pixelformats.end()) {
> +	if (pixelformats.find(cfg.pixelFormat) == pixelformats.end()) {
>  		LOG(VIMC, Debug) << "Adjusting format to RGB24";
>  		cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
>  		status = Adjusted;
> @@ -174,12 +173,12 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
> -	for (PixelFormat pixelformat : pixelformats) {
> +	for (const auto &pixelformat : pixelformats) {
>  		/* The scaler hardcodes a x3 scale-up ratio. */
>  		std::vector<SizeRange> sizes{
>  			SizeRange{ { 48, 48 }, { 4096, 2160 } }
>  		};
> -		formats[pixelformat] = sizes;
> +		formats[pixelformat.first] = sizes;
>  	}
>  
>  	StreamConfiguration cfg(formats);
> @@ -214,7 +213,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  	if (ret)
>  		return ret;
>  
> -	subformat.mbus_code = MEDIA_BUS_FMT_RGB888_1X24;
> +	subformat.mbus_code = pixelformats.find(cfg.pixelFormat)->second;
>  	ret = data->debayer_->setFormat(1, &subformat);
>  	if (ret)
>  		return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund