[libcamera-devel] [PATCH] gstreamer: Provide interlace-mode as fixated caps

[Kieran Bingham]

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 at 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 at 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 at 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
>