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

[Laurent Pinchart]

Hi Kieran,

On Thu, May 28, 2020 at 10:13:21AM +0100, Kieran Bingham wrote:
> 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>

I've now merged the patch. Do you plan to submit a new version of your
vimc pipeline handler fix ?

> > - 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" ...
> 
> > 
> > 	/*
> > 	 * 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,

Laurent Pinchart