gstreamer:Implement caps parsing for video/x-bayer

Message ID	20241031081645.302543-1-mailinglist1@johanneskirchmair.de
State	Superseded
Headers	show Return-Path: <libcamera-devel-bounces@lists.libcamera.org> From: mailinglist1@johanneskirchmair.de To: libcamera-devel@lists.libcamera.org Cc: johannes.kirchmair@skidata.com, nicolas.dufresne@collabora.com, pavel@ucw.cz Subject: [PATCH] gstreamer:Implement caps parsing for video/x-bayer Date: Thu, 31 Oct 2024 09:16:45 +0100 Message-Id: <20241031081645.302543-1-mailinglist1@johanneskirchmair.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" <libcamera-devel-bounces@lists.libcamera.org>
Series	gstreamer:Implement caps parsing for video/x-bayer
Related	show gstreamer:Implement caps parsing for video/x-bayer

mailinglist1@johanneskirchmair.de Oct. 31, 2024, 8:16 a.m. UTC

From: Johannes Kirchmair <johannes.kirchmair@skidata.com>

The parsing of video/x-bayer sources from string makes is possible to
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@skidata.com>
---
 src/gstreamer/gstlibcamera-utils.cpp | 90 ++++++++++++++--------------
 1 file changed, 46 insertions(+), 44 deletions(-)

Kieran Bingham Oct. 31, 2024, 8:40 a.m. UTC | #1

Hi Johannes,

Quoting mailinglist1@johanneskirchmair.de (2024-10-31 08:16:45)
> From: Johannes Kirchmair <johannes.kirchmair@skidata.com>
> 
> The parsing of video/x-bayer sources from string makes is possible to
> 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@skidata.com>

Thanks, this sounds like a good improvement!


> ---
>  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" },
> +};

I see we already have format_map[] defining a large part of this table.

But aside from additions to bayer2rgb and rgb2bayer, I don't see
anything defining a specific GST_VIDEO_ type for bayer formats so it
seems like this might be the cleanest way to handle this for now...

Have you tested that this interacts correctly with the bayer2rgb
element?

> +
> +#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)
> +                       return bayer_formats[i].name;
>         }
>         return NULL;
>  }
>  
> +static PixelFormat bayer_format_from_string(const gchar *name)

Looking at the existing coding style throughout this file, i think this
should be:

static PixelFormat
bayer_format_from_string(const gchar *name)

...


> +{
> +       for (unsigned int i = 0; i < ARRAY_SIZE(bayer_formats); i++) {
> +               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)

And this shouldn't be changed.


>  {
>         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!!!!!!");

And I think we need to remove this too ...

--
Regards

Kieran


>         /* 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 {
> -- 
> 2.34.1
>

Johannes Kirchmair - SKIDATA Oct. 31, 2024, 9:06 a.m. UTC | #2

Hi Kieran,

> -----Ursprüngliche Nachricht-----
> Von: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Gesendet: Donnerstag, 31. Oktober 2024 09:41
> An: libcamera-devel@lists.libcamera.org; mailinglist1@johanneskirchmair.de
> Cc: Johannes Kirchmair - SKIDATA <johannes.kirchmair@skidata.com>;
> nicolas.dufresne@collabora.com; pavel@ucw.cz
> Betreff: Re: [PATCH] gstreamer:Implement caps parsing for video/x-bayer
> 
> [Sie erhalten nicht häufig E-Mails von kieran.bingham@ideasonboard.com.
> Weitere Informationen, warum dies wichtig ist, finden Sie unter
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This is an external email, please take care when clicking links or
> opening attachments
> 
> Hi Johannes,
> 
> Quoting mailinglist1@johanneskirchmair.de (2024-10-31 08:16:45)
> > From: Johannes Kirchmair <johannes.kirchmair@skidata.com>
> >
> > The parsing of video/x-bayer sources from string makes is possible to
> > 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@skidata.com>
> 
> Thanks, this sounds like a good improvement!
> 
> 
> > ---
> >  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" }, };
> 
> I see we already have format_map[] defining a large part of this table.
> 
> But aside from additions to bayer2rgb and rgb2bayer, I don't see anything
> defining a specific GST_VIDEO_ type for bayer formats so it seems like this
> might be the cleanest way to handle this for now...
> 
> Have you tested that this interacts correctly with the bayer2rgb element?
Yes, I tested it with the bayer2rgb element but had video/x-bayer in between for some stream settings.

> 
> > +
> > +#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)
> > +                       return bayer_formats[i].name;
> >         }
> >         return NULL;
> >  }
> >
> > +static PixelFormat bayer_format_from_string(const gchar *name)
> 
> Looking at the existing coding style throughout this file, i think this should be:
> 
> static PixelFormat
> bayer_format_from_string(const gchar *name)
> 
> ...
> 
> 
> > +{
> > +       for (unsigned int i = 0; i < ARRAY_SIZE(bayer_formats); i++) {
> > +               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)
> 
> And this shouldn't be changed.
> 
> 
> >  {
> >         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!!!!!!");
> 
> And I think we need to remove this too ...
Ups :-/

I will send v2 soon.
Regards Johannes
> 
> --
> Regards
> 
> Kieran
> 
> 
> >         /* 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 {
> > --
> > 2.34.1
> >

Nicolas Dufresne Oct. 31, 2024, 1:34 p.m. UTC | #3

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@johanneskirchmair.de a
écrit :
> From: Johannes Kirchmair <johannes.kirchmair@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@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 ?

> +			return bayer_formats[i].name;

c++17 have this nice feature that let you do:

	for (const auto &format : bayer_formats) {
		....
	}

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

Johannes Kirchmair - SKIDATA Oct. 31, 2024, 2:02 p.m. UTC | #4

Hi,

> -----Ursprüngliche Nachricht-----
> Von: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Gesendet: Donnerstag, 31. Oktober 2024 14:35
> An: mailinglist1@johanneskirchmair.de; libcamera-devel@lists.libcamera.org
> Cc: Johannes Kirchmair - SKIDATA <johannes.kirchmair@skidata.com>;
> pavel@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@johanneskirchmair.de a
> écrit :
> > From: Johannes Kirchmair <johannes.kirchmair@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@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

gstreamer:Implement caps parsing for video/x-bayer
diff mbox series

Commit Message

Comments

Patch
diff mbox series

gstreamer:Implement caps parsing for video/x-bayer 21779 diff mbox series

Commit Message

Comments

Patch 21779 diff mbox series

gstreamer:Implement caps parsing for video/x-bayer
diff mbox series

Patch
diff mbox series