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

[Kieran Bingham]

Hi Laurent,

On 24/03/2020 13:12, 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):


In fact, VIMC camera support is currently broken anyway at the moment,
(as per "[RFC PATCH] libcamera: pipeline: vimc: Skip unsupported
formats") and I believe applying this patch doesn't actually break
anything further.

It does however help progress development of the VIMC pipeline handler
for supporting extra formats when they are available, and as such I
think it should be merged already ;-)

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



> 
> - 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

I wonder if the issue you describe there is a symptom of / fixed by:
"[PATCH] qcam: viewfinder: Use correct DRM/QImage mappings" ...

--
Kieran


> 
> 	/*
> 	 * 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 },
>  };
>  
>  } /* 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
--
Kieran