[libcamera-devel,v3,2/3] qcam: Stop timer on timeout

2074 diff mbox series

Stopping the timer will reset the Timer::deadline_ field to 0 fixing
potential bugs and call QtEventDispatcher::unregisterTimer() which will
take care of the cleanup.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/qcam/qt_event_dispatcher.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Hi Niklas,

Thank you for the patch.

On Mon, Sep 30, 2019 at 11:59:05PM +0200, Niklas Söderlund wrote:
> Stopping the timer will reset the Timer::deadline_ field to 0 fixing
> potential bugs and call QtEventDispatcher::unregisterTimer() which will
> take care of the cleanup.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/qcam/qt_event_dispatcher.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp
> index 98d2472c37856642..43588ef081a3f633 100644
> --- a/src/qcam/qt_event_dispatcher.cpp
> +++ b/src/qcam/qt_event_dispatcher.cpp
> @@ -131,9 +131,9 @@ void QtEventDispatcher::unregisterTimer(Timer *timer)
>  void QtEventDispatcher::timerEvent(QTimerEvent *event)
>  {
>  	auto it = timers_.find(event->timerId());
> -	timerIds_.erase(it->second);
> -	killTimer(it->first);
> -	timers_.erase(it);
> +	Timer *timer = it->second;

Could this be simplified to

	Timer *timer = timers_[event->timerId()];
	timer->stop();

? If you think there's a risk that timerEvent would be called with a
non-registered timer (bug in Qt or in qcam ?), then you can keep using
find(), but you should then check for it == timers_.end(). I'll let you
check if this would be overkill or useful.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	timer->stop();
>  }
>  
>  void QtEventDispatcher::processEvents()

Hi Laurent,

Thanks for your feedback.

On 2019-10-01 04:06:45 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Sep 30, 2019 at 11:59:05PM +0200, Niklas Söderlund wrote:
> > Stopping the timer will reset the Timer::deadline_ field to 0 fixing
> > potential bugs and call QtEventDispatcher::unregisterTimer() which will
> > take care of the cleanup.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/qcam/qt_event_dispatcher.cpp | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp
> > index 98d2472c37856642..43588ef081a3f633 100644
> > --- a/src/qcam/qt_event_dispatcher.cpp
> > +++ b/src/qcam/qt_event_dispatcher.cpp
> > @@ -131,9 +131,9 @@ void QtEventDispatcher::unregisterTimer(Timer *timer)
> >  void QtEventDispatcher::timerEvent(QTimerEvent *event)
> >  {
> >  	auto it = timers_.find(event->timerId());
> > -	timerIds_.erase(it->second);
> > -	killTimer(it->first);
> > -	timers_.erase(it);
> > +	Timer *timer = it->second;
> 
> Could this be simplified to
> 
> 	Timer *timer = timers_[event->timerId()];
> 	timer->stop();
> 
> ? If you think there's a risk that timerEvent would be called with a
> non-registered timer (bug in Qt or in qcam ?), then you can keep using
> find(), but you should then check for it == timers_.end(). I'll let you
> check if this would be overkill or useful.

I like this change and I have taken it in together with your tag,
thanks!

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> > +	timer->stop();
> >  }
> >  
> >  void QtEventDispatcher::processEvents()
> 
> -- 
> Regards,
> 
> Laurent Pinchart