| Message ID | 20210601205640.14334-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Le mercredi 02 juin 2021 à 02:26 +0530, Vedant Paranjape a écrit : > Error checking the output from generateConfiguration() was missing in > the code. Only assert was added as a guard which checked if the size of Perhaps a little rephrase would help ? > generated camera config was equal to size of roles passed to it. > > If the roles argument has a invalid member, it will return a nullptr and then > 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@gmail.com> With better text, you have my: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.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()); > > for (gsize i = 0; i < state->srcpads_.size(); i++) {
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" ? > > 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. > 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@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. Paul > > for (gsize i = 0; i < state->srcpads_.size(); i++) { > -- > 2.25.1 >
Hi Paul On Wed, Jun 2, 2021 at 3:15 PM <paul.elder@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@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*
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@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@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
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()); for (gsize i = 0; i < state->srcpads_.size(); i++) {
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. If the roles argument has a invalid member, it will return a nullptr and then 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@gmail.com> --- src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)