| Message ID | 20220711153711.36679-1-rishikeshdonadkar@gmail.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Rishikesh, I'm still uneasy on the design decision you are taking in this entire series. I'm always available on IRC for discussions if you need to. In general, your approach seems to only take a subset of libcamera and gstreamer (the well known colorspace names). I would personally completely ignore these user friendly goodies and just map four lower level enums: range, matrix, transfer function and primaries. This way, we can map the full range of options without ever having to update the code everytime something new is used by some hardware. GStreamer will transparently translate this to a "well-known-name" in caps. regards, Nicolas Le lundi 11 juillet 2022 à 21:07 +0530, Rishikesh Donadkar a écrit : > This series aims to add colorimetry support to the libcamera gstreamer > element. > > If colorimetry is specified by the user in the gstreamer pipeline, try to > apply the libcamera colorspace corresponding to the GStreamer colorimetry > to the camera if the colorspace passes the validation then push the > colorimetry corresponding to the valid colorspace in the caps. > If the colorimetry pushed into the caps is same as requested negotiation > will succeed, else negotiation will fail. > > If no colorimetry is specified by the user in the gstreamer pipeline, > push the GStreamer colorimetry corresponding to the current colorspace > applied to the camera into the caps and the negotiation will be take > care of by the gstreamer. If the colorimetry corresponding to the current > libcamera colorspace is not available in gstreamer then remove the > colorimetry field if present in the caps. > > [PATCH 1/5] - Made a verbose commit message. Introduced a new helper function > for converting std::string representation of colorimetry into > G_STRING_TYPE. Removed unnecessary g_free(). Removed the std::optional > wrapper around the colorSpace when converting into colorspace and > passed the value of the colorSpace instead to the function as it is > checked to have value before passing to the function. > Eliminated the code which was checking if the colorimetry is valid > colorimetry will be valid always because we are working with predefined > colorimetries in gstreamer. > > [PATCH 2/5] - Simplified the if-else block. Eliminated the code where colorimetry > was set to nullptr when it was not known. Insted the field is left > unset. If the field is present and is unset then remove the field. > > [PATCH 3/5] - colorSpace was passed by reference as a function argument in the > function colorimetry_form_colorspace(), Changed the function to > return the colorspace instead and setting the colorspace in the > StreamConfiguration was done after the colorspace is returned from > the function. The local functions that were used in the same file > were declared static. > > [PATCH 4/5] - Made the changes required due to the changes made in patch 3 > > [PATCH 5/5] - If the camera cannot deliver the colorspace requested in the > gstreamer pipeline then the colorspace is reset to the value > that was present before the attempt was made to set the > colorspace corresponding to the requested colorimetry. > > Rishikesh Donadkar (5): > gstreamer: convert from libcamera colorspace to GStreamer colorimetry. > gstreamer: Update the obtained colorimetry in caps. > gstreamer: Convert from GStreamer colorimetry to libcamera ColorSpace > gstreamer: Configure the colorSpace in the Stream configuration. > gstreamer: Reset the colorSpace if not the same after validation. > > src/gstreamer/gstlibcamera-utils.cpp | 69 ++++++++++++++++++++++++++++ > src/gstreamer/gstlibcamerasrc.cpp | 12 +++++ > 2 files changed, 81 insertions(+) >
Hi Nicolas, Thank you for the feedback. On 7/11/22 23:02, Nicolas Dufresne via libcamera-devel wrote: > Hi Rishikesh, > > I'm still uneasy on the design decision you are taking in this entire series. > I'm always available on IRC for discussions if you need to. > > In general, your approach seems to only take a subset of libcamera and gstreamer > (the well known colorspace names). I would personally completely ignore these > user friendly goodies and just map four lower level enums: range, matrix, > transfer function and primaries. This way, we can map the full range of options > without ever having to update the code everytime something new is used by some > hardware. GStreamer will transparently translate this to a "well-known-name" in > caps. So, you prefer to breaking down the GST_VIDEO_COLORIMETRY to lower enums and create a libcamera::Colorspace out of it? I assumed that the GStreamer would need to work only on standard colorspaces on both libcamera and GStreamer's side and we don't need to carry all the lower level enums mapping in GStreamer element. But your point of being able to use it when something new is used, is good. So in that context, I agree on breaking it down to lower enums and creating a libcamera::Colorspace out of it. My only worry in that case is that the assembled Colorspace might have one or more lower levels enums as "Invalid" (i.e. not present in libcamera's colorspace.cpp) and we proceed with such a Colorspace to be applied on the sensor? Should we allow this? Or should we check if the constructed colorspace belongs to a list of standard colorspaces (I sent a isStandardColorSpace() helper patch on the list). Any thoughts? > > regards, > Nicolas > > Le lundi 11 juillet 2022 à 21:07 +0530, Rishikesh Donadkar a écrit : >> This series aims to add colorimetry support to the libcamera gstreamer >> element. >> >> If colorimetry is specified by the user in the gstreamer pipeline, try to >> apply the libcamera colorspace corresponding to the GStreamer colorimetry >> to the camera if the colorspace passes the validation then push the >> colorimetry corresponding to the valid colorspace in the caps. >> If the colorimetry pushed into the caps is same as requested negotiation >> will succeed, else negotiation will fail. >> >> If no colorimetry is specified by the user in the gstreamer pipeline, >> push the GStreamer colorimetry corresponding to the current colorspace >> applied to the camera into the caps and the negotiation will be take >> care of by the gstreamer. If the colorimetry corresponding to the current >> libcamera colorspace is not available in gstreamer then remove the >> colorimetry field if present in the caps. >> >> [PATCH 1/5] - Made a verbose commit message. Introduced a new helper function >> for converting std::string representation of colorimetry into >> G_STRING_TYPE. Removed unnecessary g_free(). Removed the std::optional >> wrapper around the colorSpace when converting into colorspace and >> passed the value of the colorSpace instead to the function as it is >> checked to have value before passing to the function. >> Eliminated the code which was checking if the colorimetry is valid >> colorimetry will be valid always because we are working with predefined >> colorimetries in gstreamer. >> >> [PATCH 2/5] - Simplified the if-else block. Eliminated the code where colorimetry >> was set to nullptr when it was not known. Insted the field is left >> unset. If the field is present and is unset then remove the field. >> >> [PATCH 3/5] - colorSpace was passed by reference as a function argument in the >> function colorimetry_form_colorspace(), Changed the function to >> return the colorspace instead and setting the colorspace in the >> StreamConfiguration was done after the colorspace is returned from >> the function. The local functions that were used in the same file >> were declared static. >> >> [PATCH 4/5] - Made the changes required due to the changes made in patch 3 >> >> [PATCH 5/5] - If the camera cannot deliver the colorspace requested in the >> gstreamer pipeline then the colorspace is reset to the value >> that was present before the attempt was made to set the >> colorspace corresponding to the requested colorimetry. >> >> Rishikesh Donadkar (5): >> gstreamer: convert from libcamera colorspace to GStreamer colorimetry. >> gstreamer: Update the obtained colorimetry in caps. >> gstreamer: Convert from GStreamer colorimetry to libcamera ColorSpace >> gstreamer: Configure the colorSpace in the Stream configuration. >> gstreamer: Reset the colorSpace if not the same after validation. >> >> src/gstreamer/gstlibcamera-utils.cpp | 69 ++++++++++++++++++++++++++++ >> src/gstreamer/gstlibcamerasrc.cpp | 12 +++++ >> 2 files changed, 81 insertions(+) >>
Le mardi 12 juillet 2022 à 11:49 +0530, Umang Jain via libcamera-devel a écrit : > Hi Nicolas, > > Thank you for the feedback. > > On 7/11/22 23:02, Nicolas Dufresne via libcamera-devel wrote: > > Hi Rishikesh, > > > > I'm still uneasy on the design decision you are taking in this entire series. > > I'm always available on IRC for discussions if you need to. > > > > In general, your approach seems to only take a subset of libcamera and gstreamer > > (the well known colorspace names). I would personally completely ignore these > > user friendly goodies and just map four lower level enums: range, matrix, > > transfer function and primaries. This way, we can map the full range of options > > without ever having to update the code everytime something new is used by some > > hardware. GStreamer will transparently translate this to a "well-known-name" in > > caps. > > > So, you prefer to breaking down the GST_VIDEO_COLORIMETRY to lower enums > and create a libcamera::Colorspace out of it? > > I assumed that the GStreamer would need to work only on standard > colorspaces on both libcamera and GStreamer's side and we don't need to > carry all the lower level enums mapping in GStreamer element. GStreamer have no strict requirement in this regard. There is a software converter that should handle all the valid combination if converting is needed. > > But your point of being able to use it when something new is used, is > good. So in that context, I agree on breaking it down to lower enums and > creating a libcamera::Colorspace out of it. My only worry in that case > is that the assembled Colorspace might have one or more lower levels > enums as "Invalid" (i.e. not present in libcamera's colorspace.cpp) and > we proceed with such a Colorspace to be applied on the sensor? If you found no match with what downstream have selected (no intersection), you should fail the negotiation. Though, in general, downstream filter will only suggest a default and leave it open for other values to be used. > > Should we allow this? Or should we check if the constructed colorspace > belongs to a list of standard colorspaces (I sent a > isStandardColorSpace() helper patch on the list). Any thoughts? > > > > > > regards, > > Nicolas > > > > Le lundi 11 juillet 2022 à 21:07 +0530, Rishikesh Donadkar a écrit : > > > This series aims to add colorimetry support to the libcamera gstreamer > > > element. > > > > > > If colorimetry is specified by the user in the gstreamer pipeline, try to > > > apply the libcamera colorspace corresponding to the GStreamer colorimetry > > > to the camera if the colorspace passes the validation then push the > > > colorimetry corresponding to the valid colorspace in the caps. > > > If the colorimetry pushed into the caps is same as requested negotiation > > > will succeed, else negotiation will fail. > > > > > > If no colorimetry is specified by the user in the gstreamer pipeline, > > > push the GStreamer colorimetry corresponding to the current colorspace > > > applied to the camera into the caps and the negotiation will be take > > > care of by the gstreamer. If the colorimetry corresponding to the current > > > libcamera colorspace is not available in gstreamer then remove the > > > colorimetry field if present in the caps. > > > > > > [PATCH 1/5] - Made a verbose commit message. Introduced a new helper function > > > for converting std::string representation of colorimetry into > > > G_STRING_TYPE. Removed unnecessary g_free(). Removed the std::optional > > > wrapper around the colorSpace when converting into colorspace and > > > passed the value of the colorSpace instead to the function as it is > > > checked to have value before passing to the function. > > > Eliminated the code which was checking if the colorimetry is valid > > > colorimetry will be valid always because we are working with predefined > > > colorimetries in gstreamer. > > > > > > [PATCH 2/5] - Simplified the if-else block. Eliminated the code where colorimetry > > > was set to nullptr when it was not known. Insted the field is left > > > unset. If the field is present and is unset then remove the field. > > > > > > [PATCH 3/5] - colorSpace was passed by reference as a function argument in the > > > function colorimetry_form_colorspace(), Changed the function to > > > return the colorspace instead and setting the colorspace in the > > > StreamConfiguration was done after the colorspace is returned from > > > the function. The local functions that were used in the same file > > > were declared static. > > > > > > [PATCH 4/5] - Made the changes required due to the changes made in patch 3 > > > > > > [PATCH 5/5] - If the camera cannot deliver the colorspace requested in the > > > gstreamer pipeline then the colorspace is reset to the value > > > that was present before the attempt was made to set the > > > colorspace corresponding to the requested colorimetry. > > > > > > Rishikesh Donadkar (5): > > > gstreamer: convert from libcamera colorspace to GStreamer colorimetry. > > > gstreamer: Update the obtained colorimetry in caps. > > > gstreamer: Convert from GStreamer colorimetry to libcamera ColorSpace > > > gstreamer: Configure the colorSpace in the Stream configuration. > > > gstreamer: Reset the colorSpace if not the same after validation. > > > > > > src/gstreamer/gstlibcamera-utils.cpp | 69 ++++++++++++++++++++++++++++ > > > src/gstreamer/gstlibcamerasrc.cpp | 12 +++++ > > > 2 files changed, 81 insertions(+) > > >
This series aims to add colorimetry support to the libcamera gstreamer element. If colorimetry is specified by the user in the gstreamer pipeline, try to apply the libcamera colorspace corresponding to the GStreamer colorimetry to the camera if the colorspace passes the validation then push the colorimetry corresponding to the valid colorspace in the caps. If the colorimetry pushed into the caps is same as requested negotiation will succeed, else negotiation will fail. If no colorimetry is specified by the user in the gstreamer pipeline, push the GStreamer colorimetry corresponding to the current colorspace applied to the camera into the caps and the negotiation will be take care of by the gstreamer. If the colorimetry corresponding to the current libcamera colorspace is not available in gstreamer then remove the colorimetry field if present in the caps. [PATCH 1/5] - Made a verbose commit message. Introduced a new helper function for converting std::string representation of colorimetry into G_STRING_TYPE. Removed unnecessary g_free(). Removed the std::optional wrapper around the colorSpace when converting into colorspace and passed the value of the colorSpace instead to the function as it is checked to have value before passing to the function. Eliminated the code which was checking if the colorimetry is valid colorimetry will be valid always because we are working with predefined colorimetries in gstreamer. [PATCH 2/5] - Simplified the if-else block. Eliminated the code where colorimetry was set to nullptr when it was not known. Insted the field is left unset. If the field is present and is unset then remove the field. [PATCH 3/5] - colorSpace was passed by reference as a function argument in the function colorimetry_form_colorspace(), Changed the function to return the colorspace instead and setting the colorspace in the StreamConfiguration was done after the colorspace is returned from the function. The local functions that were used in the same file were declared static. [PATCH 4/5] - Made the changes required due to the changes made in patch 3 [PATCH 5/5] - If the camera cannot deliver the colorspace requested in the gstreamer pipeline then the colorspace is reset to the value that was present before the attempt was made to set the colorspace corresponding to the requested colorimetry. Rishikesh Donadkar (5): gstreamer: convert from libcamera colorspace to GStreamer colorimetry. gstreamer: Update the obtained colorimetry in caps. gstreamer: Convert from GStreamer colorimetry to libcamera ColorSpace gstreamer: Configure the colorSpace in the Stream configuration. gstreamer: Reset the colorSpace if not the same after validation. src/gstreamer/gstlibcamera-utils.cpp | 69 ++++++++++++++++++++++++++++ src/gstreamer/gstlibcamerasrc.cpp | 12 +++++ 2 files changed, 81 insertions(+)