[libcamera-devel] [PATCH v3] gstreamer: Add error checking in gst_libcamera_src_task_enter()

[Vedant Paranjape]

Hi Paul

On Wed, Jun 2, 2021 at 3:15 PM <paul.elder at ideasonboard.com> wrote:

> Hi Vedant,
>
> On Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:
> > Error checking the output from generateConfiguration() was missing in
> > the code. Only assert was added as a guard which checked if the size of
> > generated camera config was equal to size of roles passed to it.
>
> Maybe "The return value from generateConfiguration() was not checked" ?
>

Sounds much better, will make the change.


> >
> > If the roles argument has a invalid member, it will return a nullptr and
> then
>
> It's not really about roles having an invalid member, it's about the
> camera not supporting a requested role.
>

Well, I could add a random number to StreamRoles vector, but I get your
point, I'll make it invalid/unsupported role.


> > trying to access a member on a nullptr for size comparison will result
> in a
> > segmentation fault. So, if the function returns a nullptr, it will
> simply push
> > an error message on GstBus and gracefully exit.
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> b/src/gstreamer/gstlibcamerasrc.cpp
> > index 87246b40..ccc61590 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> >
> >       /* Generate the stream configurations, there should be one per
> pad. */
> >       state->config_ = state->cam_->generateConfiguration(roles);
> > -     /*
> > -      * \todo Check if camera may increase or decrease the number of
> streams
> > -      * regardless of the number of roles.
> > -      */
> > +     if (state->config_ == nullptr) {
> > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > +                               ("Failed to generate camera
> configuration from roles"),
> > +                               ("Camera::generateConfiguration()
> returned nullptr"));
> > +             gst_task_stop(task);
> > +             return;
> > +     }
> >       g_assert(state->config_->size() == state->srcpads_.size());
>
> I thought the whole point of adding the error checking is to remove this
> assert?
>
> If the return value is not nullptr, then the number of
> StreamConfigurations in the CameraConfiguration is guaranteed to be the
> same as the number of roles that were requested. If any role is not
> supported, then nullptr will be returned. So this assertion, if reached,
> will always be true.
>
>
I agree with this part, but Nicolas insisted on keeping it there. In case
something changes in the future.
Also, it asserts if the following expression turns false. You mean this? if
I understood it rightly.
https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert

Regards
*Vedant Paranjape*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210602/9226496c/attachment.htm>