AW: [PATCH] gstreamer:Implement caps parsing for video/x-bayer
Johannes Kirchmair - SKIDATA
johannes.kirchmair at skidata.com
Thu Oct 31 15:02:18 CET 2024
Hi,
> -----Ursprüngliche Nachricht-----
> Von: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> Gesendet: Donnerstag, 31. Oktober 2024 14:35
> An: mailinglist1 at johanneskirchmair.de; libcamera-devel at lists.libcamera.org
> Cc: Johannes Kirchmair - SKIDATA <johannes.kirchmair at skidata.com>;
> pavel at ucw.cz
> Betreff: Re: [PATCH] gstreamer:Implement caps parsing for video/x-bayer
>
> Caution: This is an external email, please take care when clicking links or
> opening attachments
>
> Hi,
>
> comment on the subject, you are missing a space after ":". Instead of saying
> "caps parsing", I'd prefer to call this caps negotiation.
>
> more comments below...
>
> Le jeudi 31 octobre 2024 à 09:16 +0100, mailinglist1 at johanneskirchmair.de a
> écrit :
> > From: Johannes Kirchmair <johannes.kirchmair at skidata.com>
> >
> > The parsing of video/x-bayer sources from string makes is possible to
>
> makes *it* ?
>
> > use cameras providing e.g SGRBG8 streams via gst-launch.
> >
> > Like:
> > gst-launch-1.0 libcamerasrc camera-name=<cam> !
> > video/x-bayer,format=grbg
> >
> > Without this change the gstreamer plugin complains about "Unsupported
> > media type: video/x-bayer".
> >
> > Signed-off-by: Johannes Kirchmair <johannes.kirchmair at skidata.com>
> > ---
> > src/gstreamer/gstlibcamera-utils.cpp | 90
> > ++++++++++++++--------------
> > 1 file changed, 46 insertions(+), 44 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > b/src/gstreamer/gstlibcamera-utils.cpp
> > index 79f71246..472367f2 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -254,54 +254,53 @@ gst_format_to_pixel_format(GstVideoFormat
> gst_format)
> > return PixelFormat{};
> > }
> >
> > +static struct {
> > + PixelFormat format;
> > + const gchar *name;
> > +} bayer_formats[]{
> > + { formats::SBGGR8, "bggr" },
> > + { formats::SGBRG8, "gbrg" },
> > + { formats::SGRBG8, "grbg" },
> > + { formats::SRGGB8, "rggb" },
> > + { formats::SBGGR10, "bggr10le" },
> > + { formats::SGBRG10, "gbrg10le" },
> > + { formats::SGRBG10, "grbg10le" },
> > + { formats::SRGGB10, "rggb10le" },
> > + { formats::SBGGR12, "bggr12le" },
> > + { formats::SGBRG12, "gbrg12le" },
> > + { formats::SGRBG12, "grbg12le" },
> > + { formats::SRGGB12, "rggb12le" },
> > + { formats::SBGGR14, "bggr14le" },
> > + { formats::SGBRG14, "gbrg14le" },
> > + { formats::SGRBG14, "grbg14le" },
> > + { formats::SRGGB14, "rggb14le" },
> > + { formats::SBGGR16, "bggr16le" },
> > + { formats::SGBRG16, "gbrg16le" },
> > + { formats::SGRBG16, "grbg16le" },
> > + { formats::SRGGB16, "rggb16le" }, };
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(x[0]))
> > +
> > static const gchar *
> > bayer_format_to_string(int format)
> > {
> > - switch (format) {
> > - case formats::SBGGR8:
> > - return "bggr";
> > - case formats::SGBRG8:
> > - return "gbrg";
> > - case formats::SGRBG8:
> > - return "grbg";
> > - case formats::SRGGB8:
> > - return "rggb";
> > - case formats::SBGGR10:
> > - return "bggr10le";
> > - case formats::SGBRG10:
> > - return "gbrg10le";
> > - case formats::SGRBG10:
> > - return "grbg10le";
> > - case formats::SRGGB10:
> > - return "rggb10le";
> > - case formats::SBGGR12:
> > - return "bggr12le";
> > - case formats::SGBRG12:
> > - return "gbrg12le";
> > - case formats::SGRBG12:
> > - return "grbg12le";
> > - case formats::SRGGB12:
> > - return "rggb12le";
> > - case formats::SBGGR14:
> > - return "bggr14le";
> > - case formats::SGBRG14:
> > - return "gbrg14le";
> > - case formats::SGRBG14:
> > - return "grbg14le";
> > - case formats::SRGGB14:
> > - return "rggb14le";
> > - case formats::SBGGR16:
> > - return "bggr16le";
> > - case formats::SGBRG16:
> > - return "gbrg16le";
> > - case formats::SGRBG16:
> > - return "grbg16le";
> > - case formats::SRGGB16:
> > - return "rggb16le";
> > + for (unsigned int i = 0; i < ARRAY_SIZE(bayer_formats); i++) {
> > + if ((uint32_t)bayer_formats[i].format ==
> > + (uint32_t)format)
>
> I'm curious why the choice of uint32 cast from enum and int, instead of typing
> the format argument as PixelFormat ?
My knowledge of the formats is not very good and thought the format argument was int for a good reason.
Didn’t want to break something :-D
I had a look at pixel_format.h:29.
It provides a uint32_t() operator, that’s why I preferred the uint32_t.
But if you say it would be better to have the argument being PixelFormat as well, I will change it happily.
>
> > + return bayer_formats[i].name;
>
> c++17 have this nice feature that let you do:
>
> for (const auto &format : bayer_formats) {
> ....
> }
Didn't really do c++ since 2010 so this is, more or less, my best guess on proper c++ :-D
For that reason, I am happy to have a proper review of it.
Will submit v3 soon.
Best regards
Johannes
>
> You can then drop ARRAY_SIZE. You map want to stay concistent with the
> format_map naming and call this bayer_map.
>
> > }
> > return NULL;
>
> Use nullptr, this one was missed during original review.
>
> > }
> >
> > +static PixelFormat bayer_format_from_string(const gchar *name) {
> > + for (unsigned int i = 0; i < ARRAY_SIZE(bayer_formats); i++) {
>
> Same.
>
> > + if (strcmp(bayer_formats[i].name, name) == 0)
> > + return bayer_formats[i].format;
> > + }
> > + return PixelFormat{};
> > +}
> > +
> > static GstStructure *
> > bare_structure_from_format(const PixelFormat &format) { @@ -407,9
> > +406,8 @@ gst_libcamera_stream_configuration_to_caps(const
> StreamConfiguration &stream_cfg
> > return caps;
> > }
> >
> > -void
> > -gst_libcamera_configure_stream_from_caps(StreamConfiguration
> &stream_cfg,
> > - GstCaps *caps)
> > +void gst_libcamera_configure_stream_from_caps(StreamConfiguration
> &stream_cfg,
> > + GstCaps *caps)
>
> We have chosen to follow GStreamer coding style for this one, please refrain
> from creating a mix and match through this file.
>
> > {
> > GstVideoFormat gst_format =
> pixel_format_to_gst_format(stream_cfg.pixelFormat);
> > guint i;
> > @@ -469,11 +467,15 @@
> gst_libcamera_configure_stream_from_caps(StreamConfiguration
> &stream_cfg,
> > gst_structure_fixate_field_string(s, "format", format);
> > }
> >
> > + printf("in the function!!!!!!");
>
> Please cleanup your temporary debug traces before submitting patches.
>
> > /* Then configure the stream with the result. */
> > if (gst_structure_has_name(s, "video/x-raw")) {
> > const gchar *format = gst_structure_get_string(s, "format");
> > gst_format = gst_video_format_from_string(format);
> > stream_cfg.pixelFormat =
> > gst_format_to_pixel_format(gst_format);
> > + } else if (gst_structure_has_name(s, "video/x-bayer")) {
> > + const gchar *format = gst_structure_get_string(s, "format");
> > + stream_cfg.pixelFormat =
> > + bayer_format_from_string(format);
> > } else if (gst_structure_has_name(s, "image/jpeg")) {
> > stream_cfg.pixelFormat = formats::MJPEG;
> > } else {
>
> Your patch otherwise looks right to me, we clearly missed this while adding
> Bayer support. Please send a V2 with the above fixes.
>
> regards
> Nicolas
More information about the libcamera-devel
mailing list