| Message ID | 20220829100251.935217-1-umang.jain@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Mon, Aug 29, 2022 at 03:32:51PM +0530, Umang Jain via libcamera-devel wrote: > The 'interlace-mode' for libcamerasrc will always be 'progressive'. I'm *really* crossing my fingers, hoping that you're right :-) > Provide it via fixated caps mechanism [1] > > [1] https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Rishi, Can you please check this patch as well? I think it will closely > co-relate with the framerate being captured in caps, as fixate > negotitation mechanism. > --- > src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 16d70fea..24a2e33e 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > gst_element_remove_pad(element, pad); > } > > +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] GstBaseSrc *bsrc, > + GstCaps *caps) > +{ > + GstStructure *structure; This could be a local variable within the loop, up to you. > + GstCaps *fixated_caps; > + > + fixated_caps = gst_caps_make_writable(caps); > + > + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { > + structure = gst_caps_get_structure(fixated_caps, i); > + if (gst_structure_has_field(structure, "interlace-mode")) > + gst_structure_fixate_field_string(structure, "interlace-mode", > + "progressive"); > + else > + gst_structure_set(structure, "interlace-mode", G_TYPE_STRING, > + "progressive", NULL); > + } > + > + fixated_caps = gst_caps_fixate(fixated_caps); > + > + return fixated_caps; Just return gst_caps_fixate(fixated_caps); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> But I'm no expert here. > +} > + > static void > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > { > GstElementClass *element_class = GST_ELEMENT_CLASS(klass); > GObjectClass *object_class = G_OBJECT_CLASS(klass); > + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; > + > + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; > > object_class->set_property = gst_libcamera_src_set_property; > object_class->get_property = gst_libcamera_src_get_property;
Quoting Laurent Pinchart via libcamera-devel (2022-08-30 02:01:46) > Hi Umang, > > Thank you for the patch. > > On Mon, Aug 29, 2022 at 03:32:51PM +0530, Umang Jain via libcamera-devel wrote: > > The 'interlace-mode' for libcamerasrc will always be 'progressive'. > > I'm *really* crossing my fingers, hoping that you're right :-) > > > Provide it via fixated caps mechanism [1] > > > > [1] https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > Rishi, Can you please check this patch as well? I think it will closely > > co-relate with the framerate being captured in caps, as fixate > > negotitation mechanism. > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 16d70fea..24a2e33e 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > > gst_element_remove_pad(element, pad); > > } > > > > +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] GstBaseSrc *bsrc, > > + GstCaps *caps) > > +{ > > + GstStructure *structure; > > This could be a local variable within the loop, up to you. > > > + GstCaps *fixated_caps; > > + > > + fixated_caps = gst_caps_make_writable(caps); > > + > > + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { > > + structure = gst_caps_get_structure(fixated_caps, i); > > + if (gst_structure_has_field(structure, "interlace-mode")) > > + gst_structure_fixate_field_string(structure, "interlace-mode", > > + "progressive"); > > + else > > + gst_structure_set(structure, "interlace-mode", G_TYPE_STRING, > > + "progressive", NULL); > > + } > > + > > + fixated_caps = gst_caps_fixate(fixated_caps); > > + > > + return fixated_caps; > > Just > > return gst_caps_fixate(fixated_caps); > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > But I'm no expert here. Aha - I'm excited to see more gstreamer improvements here. I believe this was reported when trying to connect the v4l2h264enc testing on RPi. Perhaps it would be nice to have a before and after test case, but I'm not currently sure of the whole pipeline that caused issues with interlace-mode. This sounds and looks good though. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +} > > + > > static void > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > { > > GstElementClass *element_class = GST_ELEMENT_CLASS(klass); > > GObjectClass *object_class = G_OBJECT_CLASS(klass); > > + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; > > + > > + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; > > > > object_class->set_property = gst_libcamera_src_set_property; > > object_class->get_property = gst_libcamera_src_get_property; > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On 8/31/22 6:44 PM, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-08-30 02:01:46) >> Hi Umang, >> >> Thank you for the patch. >> >> On Mon, Aug 29, 2022 at 03:32:51PM +0530, Umang Jain via libcamera-devel wrote: >>> The 'interlace-mode' for libcamerasrc will always be 'progressive'. >> I'm *really* crossing my fingers, hoping that you're right :-) >> >>> Provide it via fixated caps mechanism [1] >>> >>> [1] https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> Rishi, Can you please check this patch as well? I think it will closely >>> co-relate with the framerate being captured in caps, as fixate >>> negotitation mechanism. >>> --- >>> src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >>> index 16d70fea..24a2e33e 100644 >>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>> @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) >>> gst_element_remove_pad(element, pad); >>> } >>> >>> +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] GstBaseSrc *bsrc, >>> + GstCaps *caps) >>> +{ >>> + GstStructure *structure; >> This could be a local variable within the loop, up to you. >> >>> + GstCaps *fixated_caps; >>> + >>> + fixated_caps = gst_caps_make_writable(caps); >>> + >>> + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { >>> + structure = gst_caps_get_structure(fixated_caps, i); >>> + if (gst_structure_has_field(structure, "interlace-mode")) >>> + gst_structure_fixate_field_string(structure, "interlace-mode", >>> + "progressive"); >>> + else >>> + gst_structure_set(structure, "interlace-mode", G_TYPE_STRING, >>> + "progressive", NULL); >>> + } >>> + >>> + fixated_caps = gst_caps_fixate(fixated_caps); >>> + >>> + return fixated_caps; >> Just >> >> return gst_caps_fixate(fixated_caps); >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> But I'm no expert here. > Aha - I'm excited to see more gstreamer improvements here. > > I believe this was reported when trying to connect the v4l2h264enc > testing on RPi. I've asked Rishi to test the patch on RPi. Rishikesh, do you have any update? As far as I can remember the conversation was around: https://bugs.libcamera.org/show_bug.cgi?id=75#c8 With this patch, one doesn't need to manually specify 'interlace-mode' to make the pipeline work > > Perhaps it would be nice to have a before and after test case, but I'm > not currently sure of the whole pipeline that caused issues with > interlace-mode. > > This sounds and looks good though. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> +} >>> + >>> static void >>> gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) >>> { >>> GstElementClass *element_class = GST_ELEMENT_CLASS(klass); >>> GObjectClass *object_class = G_OBJECT_CLASS(klass); >>> + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; >>> + >>> + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; >>> >>> object_class->set_property = gst_libcamera_src_set_property; >>> object_class->get_property = gst_libcamera_src_get_property; >> -- >> Regards, >> >> Laurent Pinchart
Quoting Umang Jain (2022-08-31 14:18:16) > Hi Kieran, > > On 8/31/22 6:44 PM, Kieran Bingham wrote: > > Quoting Laurent Pinchart via libcamera-devel (2022-08-30 02:01:46) > >> Hi Umang, > >> > >> Thank you for the patch. > >> > >> On Mon, Aug 29, 2022 at 03:32:51PM +0530, Umang Jain via libcamera-devel wrote: > >>> The 'interlace-mode' for libcamerasrc will always be 'progressive'. > >> I'm *really* crossing my fingers, hoping that you're right :-) > >> > >>> Provide it via fixated caps mechanism [1] > >>> > >>> [1] https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation > >>> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>> --- > >>> Rishi, Can you please check this patch as well? I think it will closely > >>> co-relate with the framerate being captured in caps, as fixate > >>> negotitation mechanism. > >>> --- > >>> src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ > >>> 1 file changed, 26 insertions(+) > >>> > >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > >>> index 16d70fea..24a2e33e 100644 > >>> --- a/src/gstreamer/gstlibcamerasrc.cpp > >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp > >>> @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) > >>> gst_element_remove_pad(element, pad); > >>> } > >>> > >>> +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] GstBaseSrc *bsrc, > >>> + GstCaps *caps) > >>> +{ > >>> + GstStructure *structure; > >> This could be a local variable within the loop, up to you. > >> > >>> + GstCaps *fixated_caps; > >>> + > >>> + fixated_caps = gst_caps_make_writable(caps); > >>> + > >>> + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { > >>> + structure = gst_caps_get_structure(fixated_caps, i); > >>> + if (gst_structure_has_field(structure, "interlace-mode")) > >>> + gst_structure_fixate_field_string(structure, "interlace-mode", > >>> + "progressive"); > >>> + else > >>> + gst_structure_set(structure, "interlace-mode", G_TYPE_STRING, > >>> + "progressive", NULL); > >>> + } > >>> + > >>> + fixated_caps = gst_caps_fixate(fixated_caps); > >>> + > >>> + return fixated_caps; > >> Just > >> > >> return gst_caps_fixate(fixated_caps); > >> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> But I'm no expert here. > > Aha - I'm excited to see more gstreamer improvements here. > > > > I believe this was reported when trying to connect the v4l2h264enc > > testing on RPi. > > I've asked Rishi to test the patch on RPi. > > Rishikesh, do you have any update? As far as I can remember the > conversation was around: > > https://bugs.libcamera.org/show_bug.cgi?id=75#c8 > > With this patch, one doesn't need to manually specify 'interlace-mode' > to make the pipeline work Great. Perhaps a reference to Bug: https://bugs.libcamera.org/show_bug.cgi?id=75#c8 It's related enough by the looks of it, but that bug will be closed after we have this, colorimetry and framerate support integrated I guess. -- Kieran > > > > Perhaps it would be nice to have a before and after test case, but I'm > > not currently sure of the whole pipeline that caused issues with > > interlace-mode. > > > > This sounds and looks good though. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >>> +} > >>> + > >>> static void > >>> gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > >>> { > >>> GstElementClass *element_class = GST_ELEMENT_CLASS(klass); > >>> GObjectClass *object_class = G_OBJECT_CLASS(klass); > >>> + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; > >>> + > >>> + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; > >>> > >>> object_class->set_property = gst_libcamera_src_set_property; > >>> object_class->get_property = gst_libcamera_src_get_property; > >> -- > >> Regards, > >> > >> Laurent Pinchart >
On 8/31/22 6:48 PM, Umang Jain via libcamera-devel wrote: > Hi Kieran, > > On 8/31/22 6:44 PM, Kieran Bingham wrote: >> Quoting Laurent Pinchart via libcamera-devel (2022-08-30 02:01:46) >>> Hi Umang, >>> >>> Thank you for the patch. >>> >>> On Mon, Aug 29, 2022 at 03:32:51PM +0530, Umang Jain via >>> libcamera-devel wrote: >>>> The 'interlace-mode' for libcamerasrc will always be 'progressive'. >>> I'm *really* crossing my fingers, hoping that you're right :-) >>> >>>> Provide it via fixated caps mechanism [1] >>>> >>>> [1] >>>> https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> Rishi, Can you please check this patch as well? I think it will >>>> closely >>>> co-relate with the framerate being captured in caps, as fixate >>>> negotitation mechanism. >>>> --- >>>> src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>> index 16d70fea..24a2e33e 100644 >>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>> @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement >>>> *element, GstPad *pad) >>>> gst_element_remove_pad(element, pad); >>>> } >>>> +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] >>>> GstBaseSrc *bsrc, >>>> + GstCaps *caps) >>>> +{ >>>> + GstStructure *structure; >>> This could be a local variable within the loop, up to you. >>> >>>> + GstCaps *fixated_caps; >>>> + >>>> + fixated_caps = gst_caps_make_writable(caps); >>>> + >>>> + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { >>>> + structure = gst_caps_get_structure(fixated_caps, i); >>>> + if (gst_structure_has_field(structure, >>>> "interlace-mode")) >>>> + gst_structure_fixate_field_string(structure, "interlace-mode", >>>> + "progressive"); >>>> + else >>>> + gst_structure_set(structure, >>>> "interlace-mode", G_TYPE_STRING, >>>> + "progressive", NULL); >>>> + } >>>> + >>>> + fixated_caps = gst_caps_fixate(fixated_caps); >>>> + >>>> + return fixated_caps; >>> Just >>> >>> return gst_caps_fixate(fixated_caps); >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> But I'm no expert here. >> Aha - I'm excited to see more gstreamer improvements here. >> >> I believe this was reported when trying to connect the v4l2h264enc >> testing on RPi. > > I've asked Rishi to test the patch on RPi. > > Rishikesh, do you have any update? As far as I can remember the > conversation was around: > > https://bugs.libcamera.org/show_bug.cgi?id=75#c8 > > With this patch, one doesn't need to manually specify 'interlace-mode' > to make the pipeline work hmm, still failing in my testing, I will investigate but later... >> >> Perhaps it would be nice to have a before and after test case, but I'm >> not currently sure of the whole pipeline that caused issues with >> interlace-mode. >> >> This sounds and looks good though. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>>> +} >>>> + >>>> static void >>>> gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) >>>> { >>>> GstElementClass *element_class = GST_ELEMENT_CLASS(klass); >>>> GObjectClass *object_class = G_OBJECT_CLASS(klass); >>>> + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; >>>> + >>>> + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; >>>> object_class->set_property = gst_libcamera_src_set_property; >>>> object_class->get_property = gst_libcamera_src_get_property; >>> -- >>> Regards, >>> >>> Laurent Pinchart >
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 16d70fea..24a2e33e 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -800,11 +800,37 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad) gst_element_remove_pad(element, pad); } +static GstCaps *gst_libcamera_src_src_fixate([[maybe_unused]] GstBaseSrc *bsrc, + GstCaps *caps) +{ + GstStructure *structure; + GstCaps *fixated_caps; + + fixated_caps = gst_caps_make_writable(caps); + + for (guint i = 0; i < gst_caps_get_size(fixated_caps); ++i) { + structure = gst_caps_get_structure(fixated_caps, i); + if (gst_structure_has_field(structure, "interlace-mode")) + gst_structure_fixate_field_string(structure, "interlace-mode", + "progressive"); + else + gst_structure_set(structure, "interlace-mode", G_TYPE_STRING, + "progressive", NULL); + } + + fixated_caps = gst_caps_fixate(fixated_caps); + + return fixated_caps; +} + static void gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) { GstElementClass *element_class = GST_ELEMENT_CLASS(klass); GObjectClass *object_class = G_OBJECT_CLASS(klass); + GstBaseSrcClass *gstbasesrc_class = (GstBaseSrcClass *)klass; + + gstbasesrc_class->fixate = gst_libcamera_src_src_fixate; object_class->set_property = gst_libcamera_src_set_property; object_class->get_property = gst_libcamera_src_get_property;
The 'interlace-mode' for libcamerasrc will always be 'progressive'. Provide it via fixated caps mechanism [1] [1] https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html?gi-language=c#fixed-negotiation Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- Rishi, Can you please check this patch as well? I think it will closely co-relate with the framerate being captured in caps, as fixate negotitation mechanism. --- src/gstreamer/gstlibcamerasrc.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)