| Message ID | 20200129033210.278800-3-nicolas@ndufresne.ca |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, Thank you for the patch. On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote: > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > This transform the basic information found in StreamFormats to GstCaps. s/transform/transformts/ > This can be handy to reply to early caps query or inside a device > provider. Note that we ignored generated range as they are harmful to > caps negotiation. We also don't simplify the caps for readability > reasons, so some of the discrete value may be included in a range. > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 18 +++++ > src/gstreamer/meson.build | 1 + > 3 files changed, 121 insertions(+) > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp > create mode 100644 src/gstreamer/gstlibcamera-utils.h > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > new file mode 100644 > index 0000000..2540be0 > --- /dev/null > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -0,0 +1,102 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Collabora Ltd. > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + * > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function > + */ > + > +#include "gstlibcamera-utils.h" Could you add a blank line here, as per the coding style ? > +#include <linux/drm_fourcc.h> > + > +using namespace libcamera; > + > +static struct { > + GstVideoFormat gst_format; > + guint drm_fourcc; > +} format_map[] = { Open question, should we use camelCase instead of snake_case ? I see you're mixing the two, after a quick glance with snake_case used for C-style functions and camelCase for C++ code. I don't mind that, especially given that the GStreamer API is snake_case. Should we add a README.md to the directory that documents this exception to the coding style ? > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ Can we try to avoid commented-out code ? Is this because GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ? > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, > +}; > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) > + > +static inline GstVideoFormat The compiler should be smart enough to inline functions where needed, you don't have to instruct it to do so specifically. > +drm_to_gst_format(guint drm_fourcc) > +{ > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) > + if (format_map[i].drm_fourcc == drm_fourcc) > + return format_map[i].gst_format; If you want to make this more C++, you can define format_map as an std::array, drop FORMAT_MAP_SIZE, and write for (const auto &format : format_map) { if (format.drm_fourcc == drm_fourcc) return format.gst_format; } > + return GST_VIDEO_FORMAT_UNKNOWN; > +} > + > +static GstStructure * > +bare_structure_from_fourcc(guint fourcc) > +{ > + GstVideoFormat gst_format = drm_to_gst_format(fourcc); > + > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) > + return NULL; C++ uses nullptr instead of NULL. This comment applies to the whole series. > + > + if (gst_format != GST_VIDEO_FORMAT_ENCODED) > + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING, > + gst_video_format_to_string(gst_format), NULL); > + > + switch (fourcc) { > + case DRM_FORMAT_MJPEG: > + return gst_structure_new_empty("image/jpeg"); > + default: > + break; Maybe return nullptr; here and dropping the return at the end of the function ? > + } > + > + return NULL; > +} > + > +GstCaps * > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) > +{ > + GstCaps *caps = gst_caps_new_empty(); > + > + for (unsigned int fourcc : formats.pixelformats()) { > + g_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(fourcc); g_autoptr is a really bad name when used in C++ code and the auto keyword :-( Nothing we can do about it though, but I think you should use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use unique_ptr extensively to convey ownership information. > + > + if (!bare_s) { > + GST_WARNING("Unsupported DRM format %" GST_FOURCC_FORMAT, > + GST_FOURCC_ARGS(fourcc)); > + continue; > + } > + > + for (const Size &size : formats.sizes(fourcc)) { > + GstStructure *s = gst_structure_copy(bare_s); > + gst_structure_set(s, > + "width", G_TYPE_INT, size.width, > + "height", G_TYPE_INT, size.height, > + NULL); > + gst_caps_append_structure(caps, s); Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get the raw pointer, and add bare_s.release() here to avoid the automatic delete. This would avoid the need to call gst_structure_copy(). And now that I've written this, I realise that unique_ptr will use std::default_delete by default, while you need a specific cleanup function. You could still use a unique_ptr with a custom deleter, but it may not be worth it. Shame g_autptr doesn't support the equivalent of std::unique_ptr::release() :-( > + } > + > + const SizeRange &range = formats.range(fourcc); > + if (range.hStep && range.vStep) { > + GstStructure *s = gst_structure_copy(bare_s); > + > + gst_structure_set(s, > + "width", GST_TYPE_INT_RANGE, range.min.width, range.max.width, range.hStep, > + "height", GST_TYPE_INT_RANGE, range.min.height, range.max.height, range.vStep, > + NULL); It would be nice if the gstreamer element could use the same line wrapping rules as the rest of the code. > + gst_caps_append_structure(caps, s); > + } > + } > + > + return caps; > +} > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > new file mode 100644 > index 0000000..4545512 > --- /dev/null > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2020, Collabora Ltd. > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + * > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions > + */ > + > +#include <gst/gst.h> > +#include <gst/video/video.h> > + > +#include <libcamera/stream.h> > + These can go after the header guard too. This comment applies to the rest of the series. > +#ifndef __GST_LIBCAMERA_UTILS_H_ Missing #define __GST_LIBCAMERA_UTILS_H_ The rest of the code base has two _ at the end of the header guards, how about doing the same for all patches in this series ? > + > +GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > + > +#endif /* __GST_LIBCAMERA_UTILS_H_ */ > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > index 32d4233..39a34e7 100644 > --- a/src/gstreamer/meson.build > +++ b/src/gstreamer/meson.build > @@ -1,5 +1,6 @@ > libcamera_gst_sources = [ > 'gstlibcamera.c', > + 'gstlibcamera-utils.cpp', Nitpicking, - comes before ., so this line should go first. > 'gstlibcamerasrc.cpp', > ] >
Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart < laurent.pinchart@ideasonboard.com> a écrit : > Hi Nicolas, > > Thank you for the patch. > > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote: > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > This transform the basic information found in StreamFormats to GstCaps. > > s/transform/transformts/ > > > This can be handy to reply to early caps query or inside a device > > provider. Note that we ignored generated range as they are harmful to > > caps negotiation. We also don't simplify the caps for readability > > reasons, so some of the discrete value may be included in a range. > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++ > > src/gstreamer/gstlibcamera-utils.h | 18 +++++ > > src/gstreamer/meson.build | 1 + > > 3 files changed, 121 insertions(+) > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp > > create mode 100644 src/gstreamer/gstlibcamera-utils.h > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > > new file mode 100644 > > index 0000000..2540be0 > > --- /dev/null > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -0,0 +1,102 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2020, Collabora Ltd. > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > + * > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function > > + */ > > + > > +#include "gstlibcamera-utils.h" > > Could you add a blank line here, as per the coding style ? > > > +#include <linux/drm_fourcc.h> > > + > > +using namespace libcamera; > > + > > +static struct { > > + GstVideoFormat gst_format; > > + guint drm_fourcc; > > +} format_map[] = { > > Open question, should we use camelCase instead of snake_case ? I see > you're mixing the two, after a quick glance with snake_case used for > C-style functions and camelCase for C++ code. I don't mind that, > especially given that the GStreamer API is snake_case. Should we add a > README.md to the directory that documents this exception to the coding > style ? > I've use camel only for c++ object method, everything else follow GST style for naming, types are Camel func and var lower case snake. When note, I made typo, that happens. It depends if you intend to keep the plugin forever or want in in GST long term. > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ > > Can we try to avoid commented-out code ? Is this because > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ? > I can add it, that's usually what I do when. I'm bored and need something easy. > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, > > +}; > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) > > + > > +static inline GstVideoFormat > > The compiler should be smart enough to inline functions where needed, > you don't have to instruct it to do so specifically. > Ok. > > +drm_to_gst_format(guint drm_fourcc) > > +{ > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) > > + if (format_map[i].drm_fourcc == drm_fourcc) > > + return format_map[i].gst_format; > > If you want to make this more C++, you can define format_map as an > std::array, drop FORMAT_MAP_SIZE, and write > > for (const auto &format : format_map) { > if (format.drm_fourcc == drm_fourcc) > return format.gst_format; > } > > > + return GST_VIDEO_FORMAT_UNKNOWN; > > +} > > + > > +static GstStructure * > > +bare_structure_from_fourcc(guint fourcc) > > +{ > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc); > > + > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) > > + return NULL; > > C++ uses nullptr instead of NULL. This comment applies to the whole > series. > I know, annoying to switch habit. All the remaining are the one I forgot. > > + > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED) > > + return gst_structure_new("video/x-raw", "format", > G_TYPE_STRING, > > + > gst_video_format_to_string(gst_format), NULL); > > + > > + switch (fourcc) { > > + case DRM_FORMAT_MJPEG: > > + return gst_structure_new_empty("image/jpeg"); > > + default: > > + break; > > Maybe > > return nullptr; > > here and dropping the return at the end of the function ? > I guess as I'm not using single return that could be better. > > + } > > + > > + return NULL; > > +} > > + > > +GstCaps * > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) > > +{ > > + GstCaps *caps = gst_caps_new_empty(); > > + > > + for (unsigned int fourcc : formats.pixelformats()) { > > + g_autoptr(GstStructure) bare_s = > bare_structure_from_fourcc(fourcc); > > g_autoptr is a really bad name when used in C++ code and the auto > keyword :-( Nothing we can do about it though, but I think you should > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use > unique_ptr extensively to convey ownership information. > Do you have an example for that using a third party refcount system? If we don't use GStreamer / Glib recounts, we will break many assumption, and cause hard to catch errors. Each type have its own cleanup function, we know how to expend to that, but macros are needed anyway. I think your request is a lot of work, we need multiple different types of wrapper class, since this is not all about GObject (GstStructure is a red counted boxed type). > > + > > + if (!bare_s) { > > + GST_WARNING("Unsupported DRM format %" > GST_FOURCC_FORMAT, > > + GST_FOURCC_ARGS(fourcc)); > > + continue; > > + } > > + > > + for (const Size &size : formats.sizes(fourcc)) { > > + GstStructure *s = gst_structure_copy(bare_s); > > + gst_structure_set(s, > > + "width", G_TYPE_INT, size.width, > > + "height", G_TYPE_INT, > size.height, > > + NULL); > > + gst_caps_append_structure(caps, s); > > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get > the raw pointer, and add bare_s.release() here to avoid the automatic > delete. This would avoid the need to call gst_structure_copy(). > > And now that I've written this, I realise that unique_ptr will use > std::default_delete by default, while you need a specific cleanup > function. You could still use a unique_ptr with a custom deleter, but it > may not be worth it. Shame g_autptr doesn't support the equivalent of > std::unique_ptr::release() :-( > > > + } > > + > > + const SizeRange &range = formats.range(fourcc); > > + if (range.hStep && range.vStep) { > > + GstStructure *s = gst_structure_copy(bare_s); > > + > > + gst_structure_set(s, > > + "width", GST_TYPE_INT_RANGE, > range.min.width, range.max.width, range.hStep, > > + "height", GST_TYPE_INT_RANGE, > range.min.height, range.max.height, range.vStep, > > + NULL); > > It would be nice if the gstreamer element could use the same line > wrapping rules as the rest of the code. > > > + gst_caps_append_structure(caps, s); > > + } > > + } > > + > > + return caps; > > +} > > diff --git a/src/gstreamer/gstlibcamera-utils.h > b/src/gstreamer/gstlibcamera-utils.h > > new file mode 100644 > > index 0000000..4545512 > > --- /dev/null > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -0,0 +1,18 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2020, Collabora Ltd. > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > + * > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions > > + */ > > + > > +#include <gst/gst.h> > > +#include <gst/video/video.h> > > + > > +#include <libcamera/stream.h> > > + > > These can go after the header guard too. This comment applies to the > rest of the series. > > > +#ifndef __GST_LIBCAMERA_UTILS_H_ > > Missing > > #define __GST_LIBCAMERA_UTILS_H_ > > The rest of the code base has two _ at the end of the header guards, how > about doing the same for all patches in this series ? > > > + > > +GstCaps *gst_libcamera_stream_formats_to_caps(const > libcamera::StreamFormats &formats); > > + > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */ > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > index 32d4233..39a34e7 100644 > > --- a/src/gstreamer/meson.build > > +++ b/src/gstreamer/meson.build > > @@ -1,5 +1,6 @@ > > libcamera_gst_sources = [ > > 'gstlibcamera.c', > > + 'gstlibcamera-utils.cpp', > > Nitpicking, - comes before ., so this line should go first. > > > 'gstlibcamerasrc.cpp', > > ] > > > > -- > Regards, > > Laurent Pinchart >
Hi Nicolas, On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote: > Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit : > > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote: > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > This transform the basic information found in StreamFormats to GstCaps. > > > > s/transform/transformts/ Without the last t of course :-) > > > This can be handy to reply to early caps query or inside a device > > > provider. Note that we ignored generated range as they are harmful to > > > caps negotiation. We also don't simplify the caps for readability > > > reasons, so some of the discrete value may be included in a range. > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > --- > > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++ > > > src/gstreamer/gstlibcamera-utils.h | 18 +++++ > > > src/gstreamer/meson.build | 1 + > > > 3 files changed, 121 insertions(+) > > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp > > > create mode 100644 src/gstreamer/gstlibcamera-utils.h > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > > new file mode 100644 > > > index 0000000..2540be0 > > > --- /dev/null > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > > @@ -0,0 +1,102 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2020, Collabora Ltd. > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + * > > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function > > > + */ > > > + > > > +#include "gstlibcamera-utils.h" > > > > Could you add a blank line here, as per the coding style ? > > > > > +#include <linux/drm_fourcc.h> > > > + > > > +using namespace libcamera; > > > + > > > +static struct { > > > + GstVideoFormat gst_format; > > > + guint drm_fourcc; > > > +} format_map[] = { > > > > Open question, should we use camelCase instead of snake_case ? I see > > you're mixing the two, after a quick glance with snake_case used for > > C-style functions and camelCase for C++ code. I don't mind that, > > especially given that the GStreamer API is snake_case. Should we add a > > README.md to the directory that documents this exception to the coding > > style ? > > I've use camel only for c++ object method, everything else follow GST style > for naming, types are Camel func and var lower case snake. When note, I > made typo, that happens. It depends if you intend to keep the plugin > forever or want in in GST long term. I think it makes sense to plan for merging it in gstreamer, so a custom coding style is fine by me, as long as we document it clearly. > > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, > > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, > > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, > > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, > > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, > > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, > > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, > > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, > > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, > > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ > > > > Can we try to avoid commented-out code ? Is this because > > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ? > > I can add it, that's usually what I do when. I'm bored and need something > easy. > > > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, > > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, > > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, > > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, > > > +}; > > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) > > > + > > > +static inline GstVideoFormat > > > > The compiler should be smart enough to inline functions where needed, > > you don't have to instruct it to do so specifically. > > Ok. > > > > +drm_to_gst_format(guint drm_fourcc) > > > +{ > > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) > > > + if (format_map[i].drm_fourcc == drm_fourcc) > > > + return format_map[i].gst_format; > > > > If you want to make this more C++, you can define format_map as an > > std::array, drop FORMAT_MAP_SIZE, and write > > > > for (const auto &format : format_map) { > > if (format.drm_fourcc == drm_fourcc) > > return format.gst_format; > > } > > > > > + return GST_VIDEO_FORMAT_UNKNOWN; > > > +} > > > + > > > +static GstStructure * > > > +bare_structure_from_fourcc(guint fourcc) > > > +{ > > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc); > > > + > > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) > > > + return NULL; > > > > C++ uses nullptr instead of NULL. This comment applies to the whole > > series. > > I know, annoying to switch habit. All the remaining are the one I forgot. > > > > + > > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED) > > > + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING, > > > + > > gst_video_format_to_string(gst_format), NULL); > > > + > > > + switch (fourcc) { > > > + case DRM_FORMAT_MJPEG: > > > + return gst_structure_new_empty("image/jpeg"); > > > + default: > > > + break; > > > > Maybe > > > > return nullptr; > > > > here and dropping the return at the end of the function ? > > > > I guess as I'm not using single return that could be better. > > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +GstCaps * > > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) > > > +{ > > > + GstCaps *caps = gst_caps_new_empty(); > > > + > > > + for (unsigned int fourcc : formats.pixelformats()) { > > > + g_autoptr(GstStructure) bare_s = > > bare_structure_from_fourcc(fourcc); > > > > g_autoptr is a really bad name when used in C++ code and the auto > > keyword :-( Nothing we can do about it though, but I think you should > > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use > > unique_ptr extensively to convey ownership information. > > Do you have an example for that using a third party refcount system? If we > don't use GStreamer / Glib recounts, we will break many assumption, and > cause hard to catch errors. Each type have its own cleanup function, we > know how to expend to that, but macros are needed anyway. I think your > request is a lot of work, we need multiple different types of wrapper > class, since this is not all about GObject (GstStructure is a red counted > boxed type). If I understand glib right (which is far from a given :-)), this would become static std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))> bare_structure_from_fourcc(guint fourcc) { ... return std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); } and here auto bare_s = bare_structure_from_fourcc(fourcc); You could simplify this with template<typename T> using gst_unique_ptr = std::unique_ptr<T, decltype(&_GLIB_AUTOPTR_FUNC_NAME(T))>; static gst_unique_ptr<GstStructure> bare_structure_from_fourcc(guint fourcc) { ... return gst_unique_ptr<GstStructure>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); } ... gst_unique_ptr<GstStructure> bare_s = bare_structure_from_fourcc(fourcc); (not test-compiled though) > > > + > > > + if (!bare_s) { > > > + GST_WARNING("Unsupported DRM format %" > > GST_FOURCC_FORMAT, > > > + GST_FOURCC_ARGS(fourcc)); > > > + continue; > > > + } > > > + > > > + for (const Size &size : formats.sizes(fourcc)) { > > > + GstStructure *s = gst_structure_copy(bare_s); > > > + gst_structure_set(s, > > > + "width", G_TYPE_INT, size.width, > > > + "height", G_TYPE_INT, > > size.height, > > > + NULL); > > > + gst_caps_append_structure(caps, s); > > > > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get > > the raw pointer, and add bare_s.release() here to avoid the automatic > > delete. This would avoid the need to call gst_structure_copy(). > > > > And now that I've written this, I realise that unique_ptr will use > > std::default_delete by default, while you need a specific cleanup > > function. You could still use a unique_ptr with a custom deleter, but it > > may not be worth it. Shame g_autptr doesn't support the equivalent of > > std::unique_ptr::release() :-( > > > > > + } > > > + > > > + const SizeRange &range = formats.range(fourcc); > > > + if (range.hStep && range.vStep) { > > > + GstStructure *s = gst_structure_copy(bare_s); > > > + > > > + gst_structure_set(s, > > > + "width", GST_TYPE_INT_RANGE, > > range.min.width, range.max.width, range.hStep, > > > + "height", GST_TYPE_INT_RANGE, > > range.min.height, range.max.height, range.vStep, > > > + NULL); > > > > It would be nice if the gstreamer element could use the same line > > wrapping rules as the rest of the code. > > > > > + gst_caps_append_structure(caps, s); > > > + } > > > + } > > > + > > > + return caps; > > > +} > > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > > new file mode 100644 > > > index 0000000..4545512 > > > --- /dev/null > > > +++ b/src/gstreamer/gstlibcamera-utils.h > > > @@ -0,0 +1,18 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2020, Collabora Ltd. > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + * > > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions > > > + */ > > > + > > > +#include <gst/gst.h> > > > +#include <gst/video/video.h> > > > + > > > +#include <libcamera/stream.h> > > > + > > > > These can go after the header guard too. This comment applies to the > > rest of the series. > > > > > +#ifndef __GST_LIBCAMERA_UTILS_H_ > > > > Missing > > > > #define __GST_LIBCAMERA_UTILS_H_ > > > > The rest of the code base has two _ at the end of the header guards, how > > about doing the same for all patches in this series ? > > > > > + > > > +GstCaps *gst_libcamera_stream_formats_to_caps(const > > libcamera::StreamFormats &formats); > > > + > > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */ > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > > index 32d4233..39a34e7 100644 > > > --- a/src/gstreamer/meson.build > > > +++ b/src/gstreamer/meson.build > > > @@ -1,5 +1,6 @@ > > > libcamera_gst_sources = [ > > > 'gstlibcamera.c', > > > + 'gstlibcamera-utils.cpp', > > > > Nitpicking, - comes before ., so this line should go first. > > > > > 'gstlibcamerasrc.cpp', > > > ]
On mar, 2020-02-11 at 18:34 +0200, Laurent Pinchart wrote: > Hi Nicolas, > > On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote: > > Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit : > > > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote: > > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > > > This transform the basic information found in StreamFormats to GstCaps. > > > > > > s/transform/transformts/ > > Without the last t of course :-) > > > > > This can be handy to reply to early caps query or inside a device > > > > provider. Note that we ignored generated range as they are harmful to > > > > caps negotiation. We also don't simplify the caps for readability > > > > reasons, so some of the discrete value may be included in a range. > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > --- > > > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++ > > > > src/gstreamer/gstlibcamera-utils.h | 18 +++++ > > > > src/gstreamer/meson.build | 1 + > > > > 3 files changed, 121 insertions(+) > > > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp > > > > create mode 100644 src/gstreamer/gstlibcamera-utils.h > > > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > > > new file mode 100644 > > > > index 0000000..2540be0 > > > > --- /dev/null > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > > > @@ -0,0 +1,102 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2020, Collabora Ltd. > > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > + * > > > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function > > > > + */ > > > > + > > > > +#include "gstlibcamera-utils.h" > > > > > > Could you add a blank line here, as per the coding style ? > > > > > > > +#include <linux/drm_fourcc.h> > > > > + > > > > +using namespace libcamera; > > > > + > > > > +static struct { > > > > + GstVideoFormat gst_format; > > > > + guint drm_fourcc; > > > > +} format_map[] = { > > > > > > Open question, should we use camelCase instead of snake_case ? I see > > > you're mixing the two, after a quick glance with snake_case used for > > > C-style functions and camelCase for C++ code. I don't mind that, > > > especially given that the GStreamer API is snake_case. Should we add a > > > README.md to the directory that documents this exception to the coding > > > style ? > > > > I've use camel only for c++ object method, everything else follow GST style > > for naming, types are Camel func and var lower case snake. When note, I > > made typo, that happens. It depends if you intend to keep the plugin > > forever or want in in GST long term. > > I think it makes sense to plan for merging it in gstreamer, so a custom > coding style is fine by me, as long as we document it clearly. > > > > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, > > > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, > > > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, > > > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, > > > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, > > > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, > > > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, > > > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, > > > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, > > > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ > > > > > > Can we try to avoid commented-out code ? Is this because > > > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ? > > > > I can add it, that's usually what I do when. I'm bored and need something > > easy. > > > > > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, > > > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, > > > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, > > > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, > > > > +}; > > > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) > > > > + > > > > +static inline GstVideoFormat > > > > > > The compiler should be smart enough to inline functions where needed, > > > you don't have to instruct it to do so specifically. > > > > Ok. > > > > > > +drm_to_gst_format(guint drm_fourcc) > > > > +{ > > > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) > > > > + if (format_map[i].drm_fourcc == drm_fourcc) > > > > + return format_map[i].gst_format; > > > > > > If you want to make this more C++, you can define format_map as an > > > std::array, drop FORMAT_MAP_SIZE, and write > > > > > > for (const auto &format : format_map) { > > > if (format.drm_fourcc == drm_fourcc) > > > return format.gst_format; > > > } > > > > > > > + return GST_VIDEO_FORMAT_UNKNOWN; > > > > +} > > > > + > > > > +static GstStructure * > > > > +bare_structure_from_fourcc(guint fourcc) > > > > +{ > > > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc); > > > > + > > > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) > > > > + return NULL; > > > > > > C++ uses nullptr instead of NULL. This comment applies to the whole > > > series. > > > > I know, annoying to switch habit. All the remaining are the one I forgot. > > > > > > + > > > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED) > > > > + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING, > > > > + > > > gst_video_format_to_string(gst_format), NULL); > > > > + > > > > + switch (fourcc) { > > > > + case DRM_FORMAT_MJPEG: > > > > + return gst_structure_new_empty("image/jpeg"); > > > > + default: > > > > + break; > > > > > > Maybe > > > > > > return nullptr; > > > > > > here and dropping the return at the end of the function ? > > > > > > > I guess as I'm not using single return that could be better. > > > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +GstCaps * > > > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) > > > > +{ > > > > + GstCaps *caps = gst_caps_new_empty(); > > > > + > > > > + for (unsigned int fourcc : formats.pixelformats()) { > > > > + g_autoptr(GstStructure) bare_s = > > > bare_structure_from_fourcc(fourcc); > > > > > > g_autoptr is a really bad name when used in C++ code and the auto > > > keyword :-( Nothing we can do about it though, but I think you should > > > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use > > > unique_ptr extensively to convey ownership information. > > > > Do you have an example for that using a third party refcount system? If we > > don't use GStreamer / Glib recounts, we will break many assumption, and > > cause hard to catch errors. Each type have its own cleanup function, we > > know how to expend to that, but macros are needed anyway. I think your > > request is a lot of work, we need multiple different types of wrapper > > class, since this is not all about GObject (GstStructure is a red counted > > boxed type). > > If I understand glib right (which is far from a given :-)), this would > become > > static std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))> That is very slippery to call private macros. Private macros are not part of the API, if you use them, you are on your own. I'm really not convince by this. > bare_structure_from_fourcc(guint fourcc) > { > ... > return std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); > } > > and here > > auto bare_s = bare_structure_from_fourcc(fourcc); > > You could simplify this with > > template<typename T> > using gst_unique_ptr = std::unique_ptr<T, decltype(&_GLIB_AUTOPTR_FUNC_NAME(T))>; > > static gst_unique_ptr<GstStructure> bare_structure_from_fourcc(guint fourcc) > { > ... > return gst_unique_ptr<GstStructure>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); > } > > ... > > gst_unique_ptr<GstStructure> bare_s = bare_structure_from_fourcc(fourcc); > > (not test-compiled though) > > > > > + > > > > + if (!bare_s) { > > > > + GST_WARNING("Unsupported DRM format %" > > > GST_FOURCC_FORMAT, > > > > + GST_FOURCC_ARGS(fourcc)); > > > > + continue; > > > > + } > > > > + > > > > + for (const Size &size : formats.sizes(fourcc)) { > > > > + GstStructure *s = gst_structure_copy(bare_s); > > > > + gst_structure_set(s, > > > > + "width", G_TYPE_INT, size.width, > > > > + "height", G_TYPE_INT, > > > size.height, > > > > + NULL); > > > > + gst_caps_append_structure(caps, s); > > > > > > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get > > > the raw pointer, and add bare_s.release() here to avoid the automatic > > > delete. This would avoid the need to call gst_structure_copy(). > > > > > > And now that I've written this, I realise that unique_ptr will use > > > std::default_delete by default, while you need a specific cleanup > > > function. You could still use a unique_ptr with a custom deleter, but it > > > may not be worth it. Shame g_autptr doesn't support the equivalent of > > > std::unique_ptr::release() :-( > > > > > > > + } > > > > + > > > > + const SizeRange &range = formats.range(fourcc); > > > > + if (range.hStep && range.vStep) { > > > > + GstStructure *s = gst_structure_copy(bare_s); > > > > + > > > > + gst_structure_set(s, > > > > + "width", GST_TYPE_INT_RANGE, > > > range.min.width, range.max.width, range.hStep, > > > > + "height", GST_TYPE_INT_RANGE, > > > range.min.height, range.max.height, range.vStep, > > > > + NULL); > > > > > > It would be nice if the gstreamer element could use the same line > > > wrapping rules as the rest of the code. > > > > > > > + gst_caps_append_structure(caps, s); > > > > + } > > > > + } > > > > + > > > > + return caps; > > > > +} > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > > > new file mode 100644 > > > > index 0000000..4545512 > > > > --- /dev/null > > > > +++ b/src/gstreamer/gstlibcamera-utils.h > > > > @@ -0,0 +1,18 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2020, Collabora Ltd. > > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > + * > > > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions > > > > + */ > > > > + > > > > +#include <gst/gst.h> > > > > +#include <gst/video/video.h> > > > > + > > > > +#include <libcamera/stream.h> > > > > + > > > > > > These can go after the header guard too. This comment applies to the > > > rest of the series. > > > > > > > +#ifndef __GST_LIBCAMERA_UTILS_H_ > > > > > > Missing > > > > > > #define __GST_LIBCAMERA_UTILS_H_ > > > > > > The rest of the code base has two _ at the end of the header guards, how > > > about doing the same for all patches in this series ? > > > > > > > + > > > > +GstCaps *gst_libcamera_stream_formats_to_caps(const > > > libcamera::StreamFormats &formats); > > > > + > > > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */ > > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > > > index 32d4233..39a34e7 100644 > > > > --- a/src/gstreamer/meson.build > > > > +++ b/src/gstreamer/meson.build > > > > @@ -1,5 +1,6 @@ > > > > libcamera_gst_sources = [ > > > > 'gstlibcamera.c', > > > > + 'gstlibcamera-utils.cpp', > > > > > > Nitpicking, - comes before ., so this line should go first. > > > > > > > 'gstlibcamerasrc.cpp', > > > > ]
Hi Nicolas, On Tue, Feb 11, 2020 at 11:52:48AM -0500, Nicolas Dufresne wrote: > On mar, 2020-02-11 at 18:34 +0200, Laurent Pinchart wrote: > > On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote: > >> Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit : > >>> On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote: > >>>> From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >>>> > >>>> This transform the basic information found in StreamFormats to GstCaps. > >>> > >>> s/transform/transformts/ > > > > Without the last t of course :-) > > > >>>> This can be handy to reply to early caps query or inside a device > >>>> provider. Note that we ignored generated range as they are harmful to > >>>> caps negotiation. We also don't simplify the caps for readability > >>>> reasons, so some of the discrete value may be included in a range. > >>>> > >>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >>>> --- > >>>> src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++ > >>>> src/gstreamer/gstlibcamera-utils.h | 18 +++++ > >>>> src/gstreamer/meson.build | 1 + > >>>> 3 files changed, 121 insertions(+) > >>>> create mode 100644 src/gstreamer/gstlibcamera-utils.cpp > >>>> create mode 100644 src/gstreamer/gstlibcamera-utils.h > >>>> > >>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > >>>> new file mode 100644 > >>>> index 0000000..2540be0 > >>>> --- /dev/null > >>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp > >>>> @@ -0,0 +1,102 @@ > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>> +/* > >>>> + * Copyright (C) 2020, Collabora Ltd. > >>>> + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >>>> + * > >>>> + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function > >>>> + */ > >>>> + > >>>> +#include "gstlibcamera-utils.h" > >>> > >>> Could you add a blank line here, as per the coding style ? > >>> > >>>> +#include <linux/drm_fourcc.h> > >>>> + > >>>> +using namespace libcamera; > >>>> + > >>>> +static struct { > >>>> + GstVideoFormat gst_format; > >>>> + guint drm_fourcc; > >>>> +} format_map[] = { > >>> > >>> Open question, should we use camelCase instead of snake_case ? I see > >>> you're mixing the two, after a quick glance with snake_case used for > >>> C-style functions and camelCase for C++ code. I don't mind that, > >>> especially given that the GStreamer API is snake_case. Should we add a > >>> README.md to the directory that documents this exception to the coding > >>> style ? > >> > >> I've use camel only for c++ object method, everything else follow GST style > >> for naming, types are Camel func and var lower case snake. When note, I > >> made typo, that happens. It depends if you intend to keep the plugin > >> forever or want in in GST long term. > > > > I think it makes sense to plan for merging it in gstreamer, so a custom > > coding style is fine by me, as long as we document it clearly. > > > >>>> + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, > >>>> + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, > >>>> + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, > >>>> + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, > >>>> + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, > >>>> + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, > >>>> + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, > >>>> + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, > >>>> + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, > >>>> + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ > >>> > >>> Can we try to avoid commented-out code ? Is this because > >>> GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ? > >> > >> I can add it, that's usually what I do when. I'm bored and need something > >> easy. > >> > >>>> + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, > >>>> + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, > >>>> + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, > >>>> + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, > >>>> +}; > >>>> +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) > >>>> + > >>>> +static inline GstVideoFormat > >>> > >>> The compiler should be smart enough to inline functions where needed, > >>> you don't have to instruct it to do so specifically. > >> > >> Ok. > >> > >>>> +drm_to_gst_format(guint drm_fourcc) > >>>> +{ > >>>> + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) > >>>> + if (format_map[i].drm_fourcc == drm_fourcc) > >>>> + return format_map[i].gst_format; > >>> > >>> If you want to make this more C++, you can define format_map as an > >>> std::array, drop FORMAT_MAP_SIZE, and write > >>> > >>> for (const auto &format : format_map) { > >>> if (format.drm_fourcc == drm_fourcc) > >>> return format.gst_format; > >>> } > >>> > >>>> + return GST_VIDEO_FORMAT_UNKNOWN; > >>>> +} > >>>> + > >>>> +static GstStructure * > >>>> +bare_structure_from_fourcc(guint fourcc) > >>>> +{ > >>>> + GstVideoFormat gst_format = drm_to_gst_format(fourcc); > >>>> + > >>>> + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) > >>>> + return NULL; > >>> > >>> C++ uses nullptr instead of NULL. This comment applies to the whole > >>> series. > >> > >> I know, annoying to switch habit. All the remaining are the one I forgot. > >> > >>>> + > >>>> + if (gst_format != GST_VIDEO_FORMAT_ENCODED) > >>>> + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING, > >>>> + > >>> gst_video_format_to_string(gst_format), NULL); > >>>> + > >>>> + switch (fourcc) { > >>>> + case DRM_FORMAT_MJPEG: > >>>> + return gst_structure_new_empty("image/jpeg"); > >>>> + default: > >>>> + break; > >>> > >>> Maybe > >>> > >>> return nullptr; > >>> > >>> here and dropping the return at the end of the function ? > >>> > >> > >> I guess as I'm not using single return that could be better. > >> > >>>> + } > >>>> + > >>>> + return NULL; > >>>> +} > >>>> + > >>>> +GstCaps * > >>>> +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) > >>>> +{ > >>>> + GstCaps *caps = gst_caps_new_empty(); > >>>> + > >>>> + for (unsigned int fourcc : formats.pixelformats()) { > >>>> + g_autoptr(GstStructure) bare_s = > >>> bare_structure_from_fourcc(fourcc); > >>> > >>> g_autoptr is a really bad name when used in C++ code and the auto > >>> keyword :-( Nothing we can do about it though, but I think you should > >>> use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use > >>> unique_ptr extensively to convey ownership information. > >> > >> Do you have an example for that using a third party refcount system? If we > >> don't use GStreamer / Glib recounts, we will break many assumption, and > >> cause hard to catch errors. Each type have its own cleanup function, we > >> know how to expend to that, but macros are needed anyway. I think your > >> request is a lot of work, we need multiple different types of wrapper > >> class, since this is not all about GObject (GstStructure is a red counted > >> boxed type). > > > > If I understand glib right (which is far from a given :-)), this would > > become > > > > static std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))> > > That is very slippery to call private macros. Private macros are not part of the > API, if you use them, you are on your own. I'm really not convince by this. Please note that the review comments should be treated as friendly suggestions on how to improve the code :-) As with any suggestions, they can sometimes be completely incorrect, or just not mandatory. As discussed separately on IRC, I think this case falls in the incorrect category given that switching to unique_ptr wouldn't remove the need for gst_structure_copy() as I initially thought. > > bare_structure_from_fourcc(guint fourcc) > > { > > ... > > return std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); > > } > > > > and here > > > > auto bare_s = bare_structure_from_fourcc(fourcc); > > > > You could simplify this with > > > > template<typename T> > > using gst_unique_ptr = std::unique_ptr<T, decltype(&_GLIB_AUTOPTR_FUNC_NAME(T))>; > > > > static gst_unique_ptr<GstStructure> bare_structure_from_fourcc(guint fourcc) > > { > > ... > > return gst_unique_ptr<GstStructure>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure)); > > } > > > > ... > > > > gst_unique_ptr<GstStructure> bare_s = bare_structure_from_fourcc(fourcc); > > > > (not test-compiled though) > > > >>>> + > >>>> + if (!bare_s) { > >>>> + GST_WARNING("Unsupported DRM format %" > >>> GST_FOURCC_FORMAT, > >>>> + GST_FOURCC_ARGS(fourcc)); > >>>> + continue; > >>>> + } > >>>> + > >>>> + for (const Size &size : formats.sizes(fourcc)) { > >>>> + GstStructure *s = gst_structure_copy(bare_s); > >>>> + gst_structure_set(s, > >>>> + "width", G_TYPE_INT, size.width, > >>>> + "height", G_TYPE_INT, > >>> size.height, > >>>> + NULL); > >>>> + gst_caps_append_structure(caps, s); > >>> > >>> Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get > >>> the raw pointer, and add bare_s.release() here to avoid the automatic > >>> delete. This would avoid the need to call gst_structure_copy(). > >>> > >>> And now that I've written this, I realise that unique_ptr will use > >>> std::default_delete by default, while you need a specific cleanup > >>> function. You could still use a unique_ptr with a custom deleter, but it > >>> may not be worth it. Shame g_autptr doesn't support the equivalent of > >>> std::unique_ptr::release() :-( > >>> > >>>> + } > >>>> + > >>>> + const SizeRange &range = formats.range(fourcc); > >>>> + if (range.hStep && range.vStep) { > >>>> + GstStructure *s = gst_structure_copy(bare_s); > >>>> + > >>>> + gst_structure_set(s, > >>>> + "width", GST_TYPE_INT_RANGE, > >>> range.min.width, range.max.width, range.hStep, > >>>> + "height", GST_TYPE_INT_RANGE, > >>> range.min.height, range.max.height, range.vStep, > >>>> + NULL); > >>> > >>> It would be nice if the gstreamer element could use the same line > >>> wrapping rules as the rest of the code. > >>> > >>>> + gst_caps_append_structure(caps, s); > >>>> + } > >>>> + } > >>>> + > >>>> + return caps; > >>>> +} > >>>> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > >>>> new file mode 100644 > >>>> index 0000000..4545512 > >>>> --- /dev/null > >>>> +++ b/src/gstreamer/gstlibcamera-utils.h > >>>> @@ -0,0 +1,18 @@ > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>> +/* > >>>> + * Copyright (C) 2020, Collabora Ltd. > >>>> + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >>>> + * > >>>> + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions > >>>> + */ > >>>> + > >>>> +#include <gst/gst.h> > >>>> +#include <gst/video/video.h> > >>>> + > >>>> +#include <libcamera/stream.h> > >>>> + > >>> > >>> These can go after the header guard too. This comment applies to the > >>> rest of the series. > >>> > >>>> +#ifndef __GST_LIBCAMERA_UTILS_H_ > >>> > >>> Missing > >>> > >>> #define __GST_LIBCAMERA_UTILS_H_ > >>> > >>> The rest of the code base has two _ at the end of the header guards, how > >>> about doing the same for all patches in this series ? > >>> > >>>> + > >>>> +GstCaps *gst_libcamera_stream_formats_to_caps(const > >>> libcamera::StreamFormats &formats); > >>>> + > >>>> +#endif /* __GST_LIBCAMERA_UTILS_H_ */ > >>>> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > >>>> index 32d4233..39a34e7 100644 > >>>> --- a/src/gstreamer/meson.build > >>>> +++ b/src/gstreamer/meson.build > >>>> @@ -1,5 +1,6 @@ > >>>> libcamera_gst_sources = [ > >>>> 'gstlibcamera.c', > >>>> + 'gstlibcamera-utils.cpp', > >>> > >>> Nitpicking, - comes before ., so this line should go first. > >>> > >>>> 'gstlibcamerasrc.cpp', > >>>> ]
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp new file mode 100644 index 0000000..2540be0 --- /dev/null +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -0,0 +1,102 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2020, Collabora Ltd. + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> + * + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function + */ + +#include "gstlibcamera-utils.h" +#include <linux/drm_fourcc.h> + +using namespace libcamera; + +static struct { + GstVideoFormat gst_format; + guint drm_fourcc; +} format_map[] = { + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG }, + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 }, + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 }, + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 }, + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 }, + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 }, + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 }, + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 }, + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 }, + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */ + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY }, + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY }, + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV }, + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU }, +}; +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map) + +static inline GstVideoFormat +drm_to_gst_format(guint drm_fourcc) +{ + for (guint i = 0; i < FORMAT_MAP_SIZE; i++) + if (format_map[i].drm_fourcc == drm_fourcc) + return format_map[i].gst_format; + return GST_VIDEO_FORMAT_UNKNOWN; +} + +static GstStructure * +bare_structure_from_fourcc(guint fourcc) +{ + GstVideoFormat gst_format = drm_to_gst_format(fourcc); + + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN) + return NULL; + + if (gst_format != GST_VIDEO_FORMAT_ENCODED) + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING, + gst_video_format_to_string(gst_format), NULL); + + switch (fourcc) { + case DRM_FORMAT_MJPEG: + return gst_structure_new_empty("image/jpeg"); + default: + break; + } + + return NULL; +} + +GstCaps * +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats) +{ + GstCaps *caps = gst_caps_new_empty(); + + for (unsigned int fourcc : formats.pixelformats()) { + g_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(fourcc); + + if (!bare_s) { + GST_WARNING("Unsupported DRM format %" GST_FOURCC_FORMAT, + GST_FOURCC_ARGS(fourcc)); + continue; + } + + for (const Size &size : formats.sizes(fourcc)) { + GstStructure *s = gst_structure_copy(bare_s); + gst_structure_set(s, + "width", G_TYPE_INT, size.width, + "height", G_TYPE_INT, size.height, + NULL); + gst_caps_append_structure(caps, s); + } + + const SizeRange &range = formats.range(fourcc); + if (range.hStep && range.vStep) { + GstStructure *s = gst_structure_copy(bare_s); + + gst_structure_set(s, + "width", GST_TYPE_INT_RANGE, range.min.width, range.max.width, range.hStep, + "height", GST_TYPE_INT_RANGE, range.min.height, range.max.height, range.vStep, + NULL); + gst_caps_append_structure(caps, s); + } + } + + return caps; +} diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h new file mode 100644 index 0000000..4545512 --- /dev/null +++ b/src/gstreamer/gstlibcamera-utils.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2020, Collabora Ltd. + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> + * + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions + */ + +#include <gst/gst.h> +#include <gst/video/video.h> + +#include <libcamera/stream.h> + +#ifndef __GST_LIBCAMERA_UTILS_H_ + +GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); + +#endif /* __GST_LIBCAMERA_UTILS_H_ */ diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build index 32d4233..39a34e7 100644 --- a/src/gstreamer/meson.build +++ b/src/gstreamer/meson.build @@ -1,5 +1,6 @@ libcamera_gst_sources = [ 'gstlibcamera.c', + 'gstlibcamera-utils.cpp', 'gstlibcamerasrc.cpp', ]