[libcamera-devel,v1] Fix todo in gst_libcamera_src_task_enter()

12472 diff mbox series

Error checking the output from generateConfiguration() was missing in
the code. Only assert was added as a guard which checked if the
generated camera config are equal to the roles passed to it. But since,
if generateConfiguration() is passed a invalid role, it returns a
nullptr. Added error checking so that it checks the return value, and
sends an error message on GstBus.

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
---
 src/gstreamer/gstlibcamerasrc.cpp | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Hi Vedant,

Thank you for the patch.

I'll let Nicolas comment on the change itself, here's a comment on the
subject in the meantime.

The subjlect line should describe the effects of the change, so that
someone reading it without looking at the rest of the commit message
could get an idea of what is being done. Also, please look at the
history of the file (and the directory) to see how to format the subject
(in this case, it should start with a 'gst:' prefix).

On Sat, May 29, 2021 at 02:30:11AM +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
> generated camera config are equal to the roles passed to it. But since,
> if generateConfiguration() is passed a invalid role, it returns a
> nullptr. Added error checking so that it checks the return value, and
> sends an error message on GstBus.

The commit message itself should tell the reason for the change. You're
nearly there, you're describing the change itself, but you don't tell
why this is a good idea.

> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 87246b40..8b6057df 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -375,11 +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.
> -	 */
> -	g_assert(state->config_->size() == state->srcpads_.size());
> +	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;
> +	}
>  
>  	for (gsize i = 0; i < state->srcpads_.size(); i++) {
>  		GstPad *srcpad = state->srcpads_[i];

Le dimanche 30 mai 2021 à 19:56 +0300, Laurent Pinchart a écrit :
> Hi Vedant,
> 
> Thank you for the patch.
> 
> I'll let Nicolas comment on the change itself, here's a comment on the
> subject in the meantime.
> 
> The subjlect line should describe the effects of the change, so that
> someone reading it without looking at the rest of the commit message
> could get an idea of what is being done. Also, please look at the
> history of the file (and the directory) to see how to format the subject
> (in this case, it should start with a 'gst:' prefix).
> 
> On Sat, May 29, 2021 at 02:30:11AM +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
> > generated camera config are equal to the roles passed to it. But since,
> > if generateConfiguration() is passed a invalid role, it returns a
> > nullptr. Added error checking so that it checks the return value, and
> > sends an error message on GstBus.
> 
> The commit message itself should tell the reason for the change. You're
> nearly there, you're describing the change itself, but you don't tell
> why this is a good idea.
> 
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 87246b40..8b6057df 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -375,11 +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.
> > -	 */
> > -	g_assert(state->config_->size() == state->srcpads_.size());
> > +	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;
> > +	}

This change seems correct in regard to today's behaviour of libcamera.

> >  
> >  	for (gsize i = 0; i < state->srcpads_.size(); i++) {

I'm just a little worried that we dropped the guard for overrun. We have the
number of pads that match the number of roles, I understood that
generateConfiguration() will fail (return nullptr) if it cannot return the same
number of stream. Though, if that rule change in the future, we would have no
guard and would fail or crash a lot deeper in the code.

Perhaps keep that original assertion ?

> >  		GstPad *srcpad = state->srcpads_[i];
>