[libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make stop() idempotent

[Jacopo Mondi]

Hello,

On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:
> Hi Nicolas, Jacopo,
>
> On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:
> > Hi Jacopo,
> >
> > Em 2021-05-04 04:41, Jacopo Mondi escreveu:
> >

[snip]

> >> Personally, I would be in favour of calling stop() only when it's
> >> appropriate instead of shortcutting the camera state machine. What is
> >> the use case that makes it hard to call stop() only when actually
> >> required ?
> >
> > The use case is always calling stop() in the destructor of the capture in
> > lc-compliance, which makes the cleanup path simpler as we can safely fail an
> > assert and still have the camera stopped.
> >
> > That said, it wouldn't be hard to call stop() only when required. We could track
> > whether the camera was started internally in lc-compliance. But it seemed like
> > this would be a common pattern and handling it by making Camera::stop() itself
> > be idempotent would be beneficial.
> >
> > I'll wait for others to comment on this as well.
>
> I think being able to clean up easily on shutdowns is beneficial.
>
> I would consider this use case to be that of being able to call
>
> 	p = NULL;
> 	...
> 	kfree(p);
>
> rather than
> 	if (p)
> 		kfree(p);
>
> It's a convenience feature, so that extra state doesn't have to be
> repeatedly maintained elsewhere.
>

Might be a personal opinion, but I would prefer, as a general idea,
a stricter API. I concur there's not much value in the error message
when stop() is called from the wrong state, but sometime it helped me
finding a wrong patter when working on the camera HAL. I guess it
might be beneficial for other use cases too...

Anyway, I won't push, whatever's best for the majority is fine with
me.

Thanks
  j

> --
> Kieran
>
>
> >
> > Thanks,
> > Nícolas
> >
> >>
> >> Thanks
> >> j
> >>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
> >>>  	if (ret < 0)
> >>>  		return ret;
> >>> --
> >>> 2.31.1
> >>>
> >>> _______________________________________________
> >>> libcamera-devel mailing list
> >>> libcamera-devel at lists.libcamera.org
> >>> https://lists.libcamera.org/listinfo/libcamera-devel
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran