<div dir="ltr"><div dir="ltr">Hi Paul<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 2, 2021 at 3:15 PM <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Vedant,<br>
<br>
On Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:<br>
> Error checking the output from generateConfiguration() was missing in<br>
> the code. Only assert was added as a guard which checked if the size of<br>
> generated camera config was equal to size of roles passed to it.<br>
<br>
Maybe "The return value from generateConfiguration() was not checked" ?<br></blockquote><div><br></div><div><div>Sounds much better, will make the change.</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> If the roles argument has a invalid member, it will return a nullptr and then<br>
<br>
It's not really about roles having an invalid member, it's about the<br>
camera not supporting a requested role.<br></blockquote><div><br></div><div><div>Well, I could add a random number to StreamRoles vector, but I get your point, I'll make it invalid/unsupported role.</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> trying to access a member on a nullptr for size comparison will result in a<br>
> segmentation fault. So, if the function returns a nullptr, it will simply push<br>
> an error message on GstBus and gracefully exit.<br>
> <br>
> Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank">vedantparanjape160201@gmail.com</a>><br>
> ---<br>
>  src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----<br>
>  1 file changed, 7 insertions(+), 4 deletions(-)<br>
> <br>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp<br>
> index 87246b40..ccc61590 100644<br>
> --- a/src/gstreamer/gstlibcamerasrc.cpp<br>
> +++ b/src/gstreamer/gstlibcamerasrc.cpp<br>
> @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,<br>
>  <br>
>       /* Generate the stream configurations, there should be one per pad. */<br>
>       state->config_ = state->cam_->generateConfiguration(roles);<br>
> -     /*<br>
> -      * \todo Check if camera may increase or decrease the number of streams<br>
> -      * regardless of the number of roles.<br>
> -      */<br>
> +     if (state->config_ == nullptr) {<br>
> +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,<br>
> +                               ("Failed to generate camera configuration from roles"),<br>
> +                               ("Camera::generateConfiguration() returned nullptr"));<br>
> +             gst_task_stop(task);<br>
> +             return;<br>
> +     }<br>
>       g_assert(state->config_->size() == state->srcpads_.size());<br>
<br>
I thought the whole point of adding the error checking is to remove this<br>
assert?<br>
<br>
If the return value is not nullptr, then the number of<br>
StreamConfigurations in the CameraConfiguration is guaranteed to be the<br>
same as the number of roles that were requested. If any role is not<br>
supported, then nullptr will be returned. So this assertion, if reached,<br>
will always be true.<br>
<br></blockquote><div> <div>I agree with this part, but Nicolas insisted on keeping it there. In case something changes in the future. <br></div><div>Also, it asserts if the following expression turns false. You mean this? if I understood it rightly. <br></div><div><a href="https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert" target="_blank">https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert</a></div><div><br></div><div><div>Regards</div><font color="#888888"><div><i><b>Vedant Paranjape</b></i></div></font></div></div></div></div>