[libcamera-devel] [PATCH] libcamera: pipeline: vimc: Use appropriate media bus format
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Apr 8 11:12:35 CEST 2020
Hi Laurent,
On 2020-04-08 03:39:59 +0300, Laurent Pinchart wrote:
> 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.
I see, thanks for pointing it out.
>
> > 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 ?
Yes, sorry for saying bus format instead :-)
> The fourth one is DRM_FORMAT_RGB888,
> which is listed here, but maybe you meant the missing one from the four
> ?
Yes I was referring to the missing one from the list, not the 4th one.
> 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.
Sorry I was confused. I assumed vimc would would support it as the
commit message stated 'viewfinder reports' and that was missing was
support for it in the pipeline handler. Thus I asked if support for it
should have been added. I now see what you mean, sorry for not verifying
this in the vimc sources before sending my review.
>
> > > };
> > >
> > > } /* 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list