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

[Umang Jain]

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

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