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

[paul.elder at ideasonboard.com]

On Wed, Jun 02, 2021 at 05:06:48PM +0530, Vedant Paranjape wrote:
> 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,

In theory, yes. In practice, no:

../../src/gstreamer/gstlibcamerasrc.cpp:376:18: error: invalid
conversion from ‘int’ to
‘std::vector<libcamera::StreamRole>::value_type’ {aka
‘libcamera::StreamRole’} [-fpermissive]

That's what we have types and compiler errors for :D

(unless you static_cast it)

> I'll make it invalid/unsupported role.

But yeah, that's fine.

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

Ah okay. I guess it's fine to keep then.

> 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

assert() asserts that the expression is true. If it is not true, then it
terminates. So assert makes sure that the expression is always true.


Paul