[libcamera-devel] [PATCH v1] gstreamer: Provide framerate support for libcamerasrc.

[Rishikesh Donadkar]

>
> Regarding the subject, you should have mentioend [RFC] as you mentioned
> you are still working on it and could've mention the missing pieces below.

Noted.

> Great results, happy to see. Also you could've mentioned the platform on
> which you tested.
> In this case RPi 4 + OV5647
>
Noted.

> > +void
> > +gst_libcamera_configure_controls_from_caps(ControlList &controls,
> [[maybe_unused]] GstCaps *caps)
> > +{
> > +     /* read framerate from caps - convert to integer and set to
> frame_time. */
> > +     GstStructure *s = gst_caps_get_structure(caps, 0);
> > +     gint fps_n = -1, fps_d = -1;
> > +     if (gst_structure_has_field(s, "framerate"))
> > +             gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d);
>
> I think if the user input is malformed, get_fraction() will return a
> FALSE. We should check that for invalid input and log errors.
>
Okay.

> > +
> > +     if (fps_n < 0 || fps_d < 0)
> > +             return;
>
> Can it happen that only the numerator is mentioned? Have you tested
> something like : "framerate=30"  instead of "framerate=30/1"
>
I tested for the case "framerate=30". We cannot accept (int)30 for the
framerate field that
is supposed to be a fraction. Passing 30 fails the negotiation as
gst_pad_peer_query_caps()
returns empty caps here.
g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);

if (gst_caps_is_empty(caps)) {
flow_ret = GST_FLOW_NOT_NEGOTIATED;
break;
}

> Since frame_duration is int64_t, and fps_d and fps_n are both integers,
> can we do:
>
>              int64_t frame_duration = (fps_d * 1000000) / fps_n; ?
> > +     controls.set(controls::FrameDurationLimits,
> > +                  Span<const int64_t, 2>({
> static_cast<int64_t>(frame_duration), static_cast<int64_t>(frame_duration)
> }));
>
> you could then drop the cast from here  I believe
>
Right.

> nit: To be pedantic, we are only parsing controls needed at
> camera::StarT(); so maybe
>          gst_libcamera_get_init_ctrls_from_caps(...);
> ?

Yes, this will make more sense.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220830/9e0bb54a/attachment.htm>