| Message ID | 20220919025709.34528-2-rishikeshdonadkar@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Rishikesh, Thank you for the patch. On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote: > Iterate over each GValue in the colorimetry list, > retrieve the colorimetry string, convert to colorspace, > set the colorspace in the stream_cfg and validate the camera configuration. > Retrieve the colorspace after validation, convert to colorimetry and > check if it is same as the colorimetry requested. > > If none of the colorimetry requested is supported by the camera (i.e. not > the same after validation) then set the stream_cfg to the previous > configuration that was present before trying new colorimetry. > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++----- > src/gstreamer/gstlibcamera-utils.h | 4 ++- > src/gstreamer/gstlibcamerasrc.cpp | 2 +- > 3 files changed, 44 insertions(+), 11 deletions(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 244a4a79..f73b6349 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -316,9 +316,9 @@ 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(std::unique_ptr<CameraConfiguration> &cam_cfg, > + StreamConfiguration &stream_cfg, > + GstCaps *caps) > { > GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat); > guint i; > @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > stream_cfg.size.width = width; > stream_cfg.size.height = height; > > - /* Configure colorimetry */ > - if (gst_structure_has_field(s, "colorimetry")) { > - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > - GstVideoColorimetry colorimetry; > + const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry"); Trailing underscores are reserved for class members. > + if (!colorimetry_) > + return; > > - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > - g_critical("Invalid colorimetry %s", colorimetry_str); > + /* Configure Colorimetry */ > + GstVideoColorimetry colorimetry; > + if (GST_VALUE_HOLDS_LIST(colorimetry_)) { > + StreamConfiguration pristine_stream_cfg = stream_cfg; > + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) { > + const GValue *val = gst_value_list_get_value(colorimetry_, i); > + > + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) { > + g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val)); > + continue; > + } > + > + std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry); > + stream_cfg.colorSpace = clrSpace; > + > + /* Validate the configuration and check if the requested Wrong style. On this topic, lines are too long. Line wraps can help, but it may also indicate that some code could be split to separate functions (I haven't checked). > + * colorimetry can be applied to the sensor. Otherwise, > + * restore the configuration back to pristine one. > + */ > + if (cam_cfg->validate() != CameraConfiguration::Invalid) { The RPi pipeline handler (and other pipeline handlers) requires the same color space to be used for all streams, and it will pick (if I recall correctly) the color space of the largest non-raw stream and apply it to all other streams. For multi-stream use cases, validating the color space on streams individually will probably not work as you're expecting. You'll need a more complex logic. > + /* Check the colorspace returned after validation, with the colorspace before validation */ > + if (stream_cfg.colorSpace == clrSpace) { > + g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry)); > + break; > + } else { > + stream_cfg = pristine_stream_cfg; > + } > + } > + } > + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) { > + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_))) > + g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_)); > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry); Why does this not use validate() ? The asymmetry between the two makes me think something is wrong. > + } else { > + GST_WARNING("colorimetry field type should only be string or list."); > } > } > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index 164189a2..7afad576 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -8,6 +8,7 @@ > > #pragma once > > +#include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > #include <libcamera/stream.h> > > @@ -16,7 +17,8 @@ > > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg, Let's pass a normal reference to the function: void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg, > + libcamera::StreamConfiguration &stream_cfg, > GstCaps *caps); > #if !GST_CHECK_VERSION(1, 17, 1) > gboolean gst_task_resume(GstTask *task); > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 16d70fea..25059914 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > > /* Fixate caps and configure the stream. */ > caps = gst_caps_make_writable(caps); > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > + gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps); gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps); > } > > if (flow_ret != GST_FLOW_OK)
Hi Laurent, Thank you for the comments On 9/19/22 11:22 PM, Laurent Pinchart wrote: > Hi Rishikesh, > > Thank you for the patch. > > On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote: >> Iterate over each GValue in the colorimetry list, >> retrieve the colorimetry string, convert to colorspace, >> set the colorspace in the stream_cfg and validate the camera configuration. >> Retrieve the colorspace after validation, convert to colorimetry and >> check if it is same as the colorimetry requested. >> >> If none of the colorimetry requested is supported by the camera (i.e. not >> the same after validation) then set the stream_cfg to the previous >> configuration that was present before trying new colorimetry. >> >> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> >> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> >> --- >> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++----- >> src/gstreamer/gstlibcamera-utils.h | 4 ++- >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- >> 3 files changed, 44 insertions(+), 11 deletions(-) >> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp >> index 244a4a79..f73b6349 100644 >> --- a/src/gstreamer/gstlibcamera-utils.cpp >> +++ b/src/gstreamer/gstlibcamera-utils.cpp >> @@ -316,9 +316,9 @@ 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(std::unique_ptr<CameraConfiguration> &cam_cfg, >> + StreamConfiguration &stream_cfg, >> + GstCaps *caps) >> { >> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat); >> guint i; >> @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, >> stream_cfg.size.width = width; >> stream_cfg.size.height = height; >> >> - /* Configure colorimetry */ >> - if (gst_structure_has_field(s, "colorimetry")) { >> - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); >> - GstVideoColorimetry colorimetry; >> + const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry"); > Trailing underscores are reserved for class members. > >> + if (!colorimetry_) >> + return; >> >> - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) >> - g_critical("Invalid colorimetry %s", colorimetry_str); >> + /* Configure Colorimetry */ >> + GstVideoColorimetry colorimetry; >> + if (GST_VALUE_HOLDS_LIST(colorimetry_)) { >> + StreamConfiguration pristine_stream_cfg = stream_cfg; >> + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) { >> + const GValue *val = gst_value_list_get_value(colorimetry_, i); >> + >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) { >> + g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val)); >> + continue; >> + } >> + >> + std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry); >> + stream_cfg.colorSpace = clrSpace; >> + >> + /* Validate the configuration and check if the requested > Wrong style. On this topic, lines are too long. Line wraps can help, but > it may also indicate that some code could be split to separate functions > (I haven't checked). > >> + * colorimetry can be applied to the sensor. Otherwise, >> + * restore the configuration back to pristine one. >> + */ >> + if (cam_cfg->validate() != CameraConfiguration::Invalid) { > The RPi pipeline handler (and other pipeline handlers) requires the same > color space to be used for all streams, and it will pick (if I recall > correctly) the color space of the largest non-raw stream and apply it to > all other streams. For multi-stream use cases, validating the color > space on streams individually will probably not work as you're > expecting. You'll need a more complex logic. We indeed had a discussion around it I think but I think it was my idea to develop on top of this actually. Rishikesh, what do you think about handling this? We can have a exploratory session together as I think I haven't been much exposed to multi-stream usage via gstreamer plugin. > >> + /* Check the colorspace returned after validation, with the colorspace before validation */ >> + if (stream_cfg.colorSpace == clrSpace) { >> + g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry)); >> + break; >> + } else { >> + stream_cfg = pristine_stream_cfg; >> + } >> + } >> + } >> + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) { >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_))) >> + g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_)); >> >> stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry); > Why does this not use validate() ? The asymmetry between the two makes > me think something is wrong. For single colorimetry, the validate happens right after the function's call in src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned back, is exposed as caps. If the user-defined colorimetry as whatever is exposed in caps, negotiation shall pass. For multiple colorimetry, we need to "select" a supported colorspace candidate from the user-provided list, hence need to run through validate() to see what sensor agrees on any-one of them. Which is found to be suitable, is set in the stream configuration and re-validated (although futile) in src/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and then exposed through caps. Hence, an extra validation is not required for the single-colorimetry case (which currently is the behaviour on master as well). Does that makes sense? > >> + } else { >> + GST_WARNING("colorimetry field type should only be string or list."); >> } >> } >> >> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h >> index 164189a2..7afad576 100644 >> --- a/src/gstreamer/gstlibcamera-utils.h >> +++ b/src/gstreamer/gstlibcamera-utils.h >> @@ -8,6 +8,7 @@ >> >> #pragma once >> >> +#include <libcamera/camera.h> >> #include <libcamera/camera_manager.h> >> #include <libcamera/stream.h> >> >> @@ -16,7 +17,8 @@ >> >> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); >> GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); >> -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, >> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg, > Let's pass a normal reference to the function: > > void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg, ah good point, ack > >> + libcamera::StreamConfiguration &stream_cfg, >> GstCaps *caps); >> #if !GST_CHECK_VERSION(1, 17, 1) >> gboolean gst_task_resume(GstTask *task); >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >> index 16d70fea..25059914 100644 >> --- a/src/gstreamer/gstlibcamerasrc.cpp >> +++ b/src/gstreamer/gstlibcamerasrc.cpp >> @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, >> >> /* Fixate caps and configure the stream. */ >> caps = gst_caps_make_writable(caps); >> - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); >> + gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps); > gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps); > >> } >> >> if (flow_ret != GST_FLOW_OK)
Hello Umang and Laurent, On Tue, Sep 20, 2022 at 12:18 AM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Laurent, > > Thank you for the comments > On 9/19/22 11:22 PM, Laurent Pinchart wrote: > > Hi Rishikesh, > > > > Thank you for the patch. > > > > On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote: > >> Iterate over each GValue in the colorimetry list, > >> retrieve the colorimetry string, convert to colorspace, > >> set the colorspace in the stream_cfg and validate the camera configuration. > >> Retrieve the colorspace after validation, convert to colorimetry and > >> check if it is same as the colorimetry requested. > >> > >> If none of the colorimetry requested is supported by the camera (i.e. not > >> the same after validation) then set the stream_cfg to the previous > >> configuration that was present before trying new colorimetry. > >> > >> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > >> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >> --- > >> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++----- > >> src/gstreamer/gstlibcamera-utils.h | 4 ++- > >> src/gstreamer/gstlibcamerasrc.cpp | 2 +- > >> 3 files changed, 44 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > >> index 244a4a79..f73b6349 100644 > >> --- a/src/gstreamer/gstlibcamera-utils.cpp > >> +++ b/src/gstreamer/gstlibcamera-utils.cpp > >> @@ -316,9 +316,9 @@ 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(std::unique_ptr<CameraConfiguration> &cam_cfg, > >> + StreamConfiguration &stream_cfg, > >> + GstCaps *caps) > >> { > >> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat); > >> guint i; > >> @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > >> stream_cfg.size.width = width; > >> stream_cfg.size.height = height; > >> > >> - /* Configure colorimetry */ > >> - if (gst_structure_has_field(s, "colorimetry")) { > >> - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); > >> - GstVideoColorimetry colorimetry; > >> + const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry"); > > Trailing underscores are reserved for class members. > > > >> + if (!colorimetry_) > >> + return; > >> > >> - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) > >> - g_critical("Invalid colorimetry %s", colorimetry_str); > >> + /* Configure Colorimetry */ > >> + GstVideoColorimetry colorimetry; > >> + if (GST_VALUE_HOLDS_LIST(colorimetry_)) { > >> + StreamConfiguration pristine_stream_cfg = stream_cfg; > >> + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) { > >> + const GValue *val = gst_value_list_get_value(colorimetry_, i); > >> + > >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) { > >> + g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val)); > >> + continue; > >> + } > >> + > >> + std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry); > >> + stream_cfg.colorSpace = clrSpace; > >> + > >> + /* Validate the configuration and check if the requested > > Wrong style. On this topic, lines are too long. Line wraps can help, but > > it may also indicate that some code could be split to separate functions > > (I haven't checked). Ack. > > > > >> + * colorimetry can be applied to the sensor. Otherwise, > >> + * restore the configuration back to pristine one. > >> + */ > >> + if (cam_cfg->validate() != CameraConfiguration::Invalid) { > > The RPi pipeline handler (and other pipeline handlers) requires the same > > color space to be used for all streams, and it will pick (if I recall > > correctly) the color space of the largest non-raw stream and apply it to > > all other streams. For multi-stream use cases, validating the color > > space on streams individually will probably not work as you're > > expecting. You'll need a more complex logic. > > We indeed had a discussion around it I think but I think it was my idea > to develop on top of this actually. > > Rishikesh, what do you think about handling this? We can have a > exploratory session together as I think I haven't been much exposed to > multi-stream usage via gstreamer plugin. Sure, even I have not been much exposed to multi-stream usage, maybe Vedant might be able to help with this. > > > > >> + /* Check the colorspace returned after validation, with the colorspace before validation */ > >> + if (stream_cfg.colorSpace == clrSpace) { > >> + g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry)); > >> + break; > >> + } else { > >> + stream_cfg = pristine_stream_cfg; > >> + } > >> + } > >> + } > >> + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) { > >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_))) > >> + g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_)); > >> > >> stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry); > > Why does this not use validate() ? The asymmetry between the two makes > > me think something is wrong. > > For single colorimetry, the validate happens right after the function's > call in src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned > back, is exposed as caps. If the user-defined colorimetry as whatever is > exposed in caps, negotiation shall pass. > > For multiple colorimetry, we need to "select" a supported colorspace > candidate from the user-provided list, hence need to run through > validate() to see what sensor agrees on any-one of them. Which is found > to be suitable, is set in the stream configuration and re-validated > (although futile) in > src/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and > then exposed through caps. > > Hence, an extra validation is not required for the single-colorimetry > case (which currently is the behaviour on master as well). Does that > makes sense? > > > > >> + } else { > >> + GST_WARNING("colorimetry field type should only be string or list."); > >> } > >> } > >> > >> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > >> index 164189a2..7afad576 100644 > >> --- a/src/gstreamer/gstlibcamera-utils.h > >> +++ b/src/gstreamer/gstlibcamera-utils.h > >> @@ -8,6 +8,7 @@ > >> > >> #pragma once > >> > >> +#include <libcamera/camera.h> > >> #include <libcamera/camera_manager.h> > >> #include <libcamera/stream.h> > >> > >> @@ -16,7 +17,8 @@ > >> > >> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > >> GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); > >> -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > >> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg, > > Let's pass a normal reference to the function: > > > > void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg, > > ah good point, ack > > > >> + libcamera::StreamConfiguration &stream_cfg, > >> GstCaps *caps); > >> #if !GST_CHECK_VERSION(1, 17, 1) > >> gboolean gst_task_resume(GstTask *task); > >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > >> index 16d70fea..25059914 100644 > >> --- a/src/gstreamer/gstlibcamerasrc.cpp > >> +++ b/src/gstreamer/gstlibcamerasrc.cpp > >> @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > >> > >> /* Fixate caps and configure the stream. */ > >> caps = gst_caps_make_writable(caps); > >> - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); > >> + gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps); > > gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps); > > > >> } > >> > >> if (flow_ret != GST_FLOW_OK) > Regards, Rishikesh Donadkar
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 244a4a79..f73b6349 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -316,9 +316,9 @@ 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(std::unique_ptr<CameraConfiguration> &cam_cfg, + StreamConfiguration &stream_cfg, + GstCaps *caps) { GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat); guint i; @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, stream_cfg.size.width = width; stream_cfg.size.height = height; - /* Configure colorimetry */ - if (gst_structure_has_field(s, "colorimetry")) { - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry"); - GstVideoColorimetry colorimetry; + const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry"); + if (!colorimetry_) + return; - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str)) - g_critical("Invalid colorimetry %s", colorimetry_str); + /* Configure Colorimetry */ + GstVideoColorimetry colorimetry; + if (GST_VALUE_HOLDS_LIST(colorimetry_)) { + StreamConfiguration pristine_stream_cfg = stream_cfg; + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) { + const GValue *val = gst_value_list_get_value(colorimetry_, i); + + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) { + g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val)); + continue; + } + + std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry); + stream_cfg.colorSpace = clrSpace; + + /* Validate the configuration and check if the requested + * colorimetry can be applied to the sensor. Otherwise, + * restore the configuration back to pristine one. + */ + if (cam_cfg->validate() != CameraConfiguration::Invalid) { + /* Check the colorspace returned after validation, with the colorspace before validation */ + if (stream_cfg.colorSpace == clrSpace) { + g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry)); + break; + } else { + stream_cfg = pristine_stream_cfg; + } + } + } + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) { + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_))) + g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_)); stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry); + } else { + GST_WARNING("colorimetry field type should only be string or list."); } } diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index 164189a2..7afad576 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -8,6 +8,7 @@ #pragma once +#include <libcamera/camera.h> #include <libcamera/camera_manager.h> #include <libcamera/stream.h> @@ -16,7 +17,8 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg, + libcamera::StreamConfiguration &stream_cfg, GstCaps *caps); #if !GST_CHECK_VERSION(1, 17, 1) gboolean gst_task_resume(GstTask *task); diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 16d70fea..25059914 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -503,7 +503,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, /* Fixate caps and configure the stream. */ caps = gst_caps_make_writable(caps); - gst_libcamera_configure_stream_from_caps(stream_cfg, caps); + gst_libcamera_configure_stream_from_caps(state->config_, stream_cfg, caps); } if (flow_ret != GST_FLOW_OK)