[libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add toMbusCode method
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 10 13:38:23 CET 2023
Hi David,
On Tue, Jan 10, 2023 at 11:59:19AM +0000, David Plowman wrote:
> Hi again
>
> I was thinking about this a bit more. One thing I wanted to clarify
> was what media bus formats really mean. For example, if I have
> MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2
> packing. Does that sound right?
Not per my understanding
The "CSI-2 packaged" Bayer format are described by the 'P' variant of
the canonical raw Bayer formats packaged in 5 bytes according to the
CSI-2 specifications.
In example section "11.4.4 RAW10" specifies
The transmission of 10-bit Raw data is accomplished by packing the
10-bit pixel data to look like 8-bit data format.
Which means the first 4 bytes will contain the 8 MSB and the last
byte contains the 2 LSB of each pixel.
V4L2 defines pixel formats for CSI-2 packaged bayer formats, but not
media bus codes as far as I can tell:
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10p.html
This surprises me as I would be expecting that being the formats
defined by the CSI-2 specs that's actually what it is sent on the wire
but as far as I can tell sensors I've seen do not send RAW10 that way?
Am I confused ?
>
> Under those circumstances it seems reasonable to me that the
> mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P
> for most of the formats there, but with new values (e.g.
> Packing::PADHI_BE) for the ones now under discussion.
>
> I don't think such a change would bother Raspberry Pi (it might well
> "just work", or if not require only very minor tweaks), but someone
> else would have to comment on ipu3 or rkisp1.
>
Please note that 2X8 formats are for parallel busses only, and the
PAD.. only apply to them. I wonder if we do care about them for real..
> Dave also mentions that all the non-BGGR versions are missing (it
> seems a bit disappointing that they weren't added all at the same
> time), but for the moment I suggest we ignore that.
>
> I also note there's a potential problem with the "ALAW8" and "DPCM8"
> formats. I don't know if folks think those can be handled by more
> "packing" formats, or whether we'd rather have a new "modifiers"
> field?
ALAW and DPCM8 could be described as "compressions" more than
packaging ? Reading
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-srggb10alaw8.html
it seems to imply a 10-bit pixel gets compressed to an 8-bit sample,
so that impacts the buffer and line sizes ?
>
> Thoughts?
Yes, and all of them rather confused :)
>
> David
>
> On Fri, 6 Jan 2023 at 17:28, Dave Stevenson
> <dave.stevenson at raspberrypi.com> wrote:
> >
> > Hi David and Jacopo
> >
> > On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > Hi Jacopo
> > >
> > > On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > Hi David
> > > > sorry to revive this, but I'm carrying this series in my tree and I
> > > > just found a bug in this patches
> > > >
> > > >
> > > > On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:
> > > > > From: David Plowman <david.plowman at raspberrypi.com>
> > > > >
> > > > > This makes it easier to perform transformations on Bayer type mbus
> > > > > codes by converting them to a BayerFormat, doing the transform, and
> > > > > then converting them back again.
> > > > >
> > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > ---
> > > > > include/libcamera/internal/bayer_format.h | 1 +
> > > > > src/libcamera/bayer_format.cpp | 11 +++++++++++
> > > > > 2 files changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > > > index 78ba3969913d..3601dccb7228 100644
> > > > > --- a/include/libcamera/internal/bayer_format.h
> > > > > +++ b/include/libcamera/internal/bayer_format.h
> > > > > @@ -47,6 +47,7 @@ public:
> > > > > }
> > > > >
> > > > > static const BayerFormat &fromMbusCode(unsigned int mbusCode);
> > > > > + unsigned int toMbusCode() const;
> > > > > bool isValid() const { return bitDepth != 0; }
> > > > >
> > > > > std::string toString() const;
> > > > > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > > > > index f27cc1662d25..fdbc4af1dcc0 100644
> > > > > --- a/src/libcamera/bayer_format.cpp
> > > > > +++ b/src/libcamera/bayer_format.cpp
> > > > > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)
> > > > > return it->second;
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * \brief Retrieve the media bus code corresponding this this BayerFormat
> > > > > + * \return The corresponding media bus code, or zero if none was found
> > > > > + */
> > > > > +unsigned int BayerFormat::toMbusCode() const
> > > > > +{
> > > > > + auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),
> > > > > + [this](const auto &i) { return i.second == *this; });
> > > > > + return it != mbusCodeToBayer.end() ? it->first : 0;
> > > >
> > > > I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10
> > > > It doesn't register flips, so the bayer pattern should not be changed
> > > >
> > > > The code you added in the next patch for the CameraSensor class goes
> > > > as
> > > >
> > > > for (const auto &format : formats) {
> > > > unsigned int mbusCode = format.first;
> > > > BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > > >
> > > > if (bayerFormat.isValid())
> > > > mbusCode = bayerFormat.transform(transform).toMbusCode();
> > > >
> > > > if (mbusCode)
> > > > formats_[mbusCode] = std::move(format.second);
> > > > }
> > > >
> > > >
> > > > The first
> > > >
> > > > BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > > >
> > > > returns { BayerFormat::BGGR, 10, BayerFormat::Packing::None }
> > > >
> > > > we go throgh a transform() which does nothing as it receives Indentity
> > > > and then call toMbusCode() on it
> > > >
> > > > The above implementation of toMbusCode() walks the mbusCodeToBayer map
> > > > looking for a matching bayer. Look at this table section
> > > >
> > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > > { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
> > > >
> > > > So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and
> > > > MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same
> > > > { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly
> > > > get my original 0x3007 code transformed to 0x3003!
> > > >
> > > > I'm afraid the fact mbus_code-to-BayerFormat relationship is not
> > > > unique is a blocker for this patch
> > >
> > > Oh dear! Thanks for discovering this, I had no idea that these formats existed.
> >
> > Why do those formats exist, and why only the BGGR ordering versions?
> > A partial implementation of something, and presumably on a parallel
> > interface as formats such as YUYV are mandated to use the 1X16
> > versions (not 2X8) on CSI.
> >
> > Are they actually relevant, or legacy cruft?
> >
> > Dave
> >
> > > I wonder if the correct solution is to have 4 extra packing formats,
> > > alongside the existing CSI2 and IPU3 ones. What do you think?
> > >
> > > David
> > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * \fn BayerFormat::isValid()
> > > > > * \brief Return whether a BayerFormat is valid
> > > > > --
> > > > > 2.38.1
> > > > >
More information about the libcamera-devel
mailing list