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

[Laurent Pinchart]

Hi Niklas,

On Wed, Apr 08, 2020 at 12:47:08AM +0200, Niklas Söderlund wrote:
> 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?

The vimc drivers maps MEDIA_BUS_FMT_ARGB8888_1X32 to
V4L2_PIX_FMT_ARGB32, which corresponds to DRM_FORMAT_BGRA8888. The
mapping may be considered awkward, but it matches the driver
implementation.

> Also should the 4th bus format listed in the commit message be added
> to this map?

Now I'm confused. The commit message lists DRM pixel formats, not bus
formats. Is that what you meant ? The fourth one is DRM_FORMAT_RGB888,
which is listed here, but maybe you meant the missing one from the four
? That would be DRM_FORMAT_ABGR8888, which corresponds to
V4L2_PIX_FMT_RGBA32, which is not supported by vimc. I've thus left it
out from this patch.

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