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

[Laurent Pinchart]

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 at 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];

-- 
Regards,

Laurent Pinchart