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

[Vedant Paranjape]

Hi,
Got it. I'll send a completely new patch by merging the two commits on my
local repo using git rebase.

Also, let me know if the commit message looks good to you. I did changes
according to your comments on the v1 patch

Regards,
Vedant Paranjape

On Wed, 2 Jun, 2021, 01:41 Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Vedant,
>
> On Wed, Jun 02, 2021 at 12:57:26AM +0530, Vedant Paranjape wrote:
> > Hey,
> > I think I messed up with the commit history in this patch, do you want me
> > to rebase the commit in v1 and v2 together and send a new patch ?
>
> Please do. As a general rule, patches should be based on the master
> branch of libcamera. In this particular case, we won't apply v1 (given
> that it didn't pass the review step), so v2 has to be self-contained.
> There are multiple reasons for that, two major ones are to avoid
> bisection breakages (if v1 introduces a problem that gets noticed during
> review, applying v1 and a fix on top of it would cause breakages when
> testing the version with v1 without the fix), and to show the whole
> context during review (if I were to review this v2, I would need to
> first apply your v1, then v2, and review the result).
>
> > On Wed, Jun 2, 2021 at 12:54 AM 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.
> > >
> > > 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 put
> > > a error on GstBus and gracefully exit.
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > ---
> > >  src/gstreamer/gstlibcamerasrc.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > > b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 8b6057df..910c0f83 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -375,9 +375,9 @@ 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);
> > > -       if (state->config_ == nullptr) {
> > > +       if (state->config_ == nullptr && state->config_->size() ==
> state->srcpads_.size()) {
> > >                 GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > -                                 ("Failed to generate camera
> configuration from roles"),
> > > +                                 ("Failed to generate camera
> configuration from"),
> > >                                   ("Camera::generateConfiguration()
> returned nullptr"));
> > >                 gst_task_stop(task);
> > >                 return;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210602/b00c21ed/attachment.htm>