<div dir="ltr"><div dir="ltr">Hi Kieran,</div><div dir="ltr"><br></div><div>Thank you for the feedback!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 5 May 2022 at 10:36, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Quoting Naushir Patuck via libcamera-devel (2022-05-05 09:48:24)<br>
> Only enable/reset the watchdog timer when there are buffers queued in the V4L2<br>
> device. Otherwise, we may trigger spurious warnings when the watchdog times out<br>
> even if there are no buffers queued in the device.<br>
<br>
aha yes - I can see how that was tripping up on python interactive<br>
sessions or otherwise underflowing when it's not at all a fault of the<br>
V4L2VideoDevice.<br>
<br>
<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/libcamera/v4l2_videodevice.cpp | 22 +++++++++++++++-------<br>
>  1 file changed, 15 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp<br>
> index 5b4637b1a39e..430715afd554 100644<br>
> --- a/src/libcamera/v4l2_videodevice.cpp<br>
> +++ b/src/libcamera/v4l2_videodevice.cpp<br>
> @@ -1662,8 +1662,11 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â return ret;<br>
>  Â  Â  Â  Â }<br>
>  <br>
> -  Â  Â  Â if (queuedBuffers_.empty())<br>
> +  Â  Â  Â if (queuedBuffers_.empty()) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â fdBufferNotifier_->setEnabled(true);<br>
> +  Â  Â  Â  Â  Â  Â  Â if (watchdogDuration_)<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));<br>
> +  Â  Â  Â }<br>
<br>
I guess this could also be set outside of the if (queuedBuffer_.empty())<br>
- but it would artificially delay the watchdog every time a buffer was<br>
queued. In the event that more than one buffer is required to be queued<br>
(to satisfy internal requirements perhaps?) that might actually be<br>
beneficial ... But I think either way is fine.<br></blockquote><div><br></div><div>It could... I thought it might be ever so slightly more logically correct if this</div><div>was inside the if() block.  I'll keep it in there unless there are further objections.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
>  <br>
>  Â  Â  Â  Â queuedBuffers_[buf.index] = buffer;<br>
>  <br>
> @@ -1742,16 +1745,21 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()<br>
>  Â  Â  Â  Â  Â  Â  Â  Â return nullptr;<br>
>  Â  Â  Â  Â }<br>
>  <br>
> -  Â  Â  Â if (watchdogDuration_.count())<br>
> -  Â  Â  Â  Â  Â  Â  Â watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));<br>
> -<br>
>  Â  Â  Â  Â cache_->put(buf.index);<br>
>  <br>
>  Â  Â  Â  Â FrameBuffer *buffer = it->second;<br>
>  Â  Â  Â  Â queuedBuffers_.erase(it);<br>
>  <br>
> -  Â  Â  Â if (queuedBuffers_.empty())<br>
> +  Â  Â  Â if (queuedBuffers_.empty()) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â fdBufferNotifier_->setEnabled(false);<br>
> +  Â  Â  Â  Â  Â  Â  Â watchdog_.stop();<br>
> +  Â  Â  Â } else if (watchdogDuration_) {<br>
> +  Â  Â  Â  Â  Â  Â  Â /*<br>
> +  Â  Â  Â  Â  Â  Â  Â  * Restart the watchdog timer if there are buffers still queued<br>
> +  Â  Â  Â  Â  Â  Â  Â  * in the device.<br>
> +  Â  Â  Â  Â  Â  Â  Â  */<br>
> +  Â  Â  Â  Â  Â  Â  Â watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));<br>
<br>
Why do we need/have all these casts? Is either watchdogDuration_ not a<br>
duration? or is watchdog_.start() not accepting a duration? Either of<br>
those would remove a lot of casting surely?<br></blockquote><div><br></div>This is a bit of a minor annoyance because of how I defined the underlying type<br>of utils::Duration to a double. The std::chrono library forbids implicit casts<br><div>from integer to float types.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">watchdogDuration_ is a utils::Duration which stores std::nano, and I see<br>
Timer start takes a std::chrono::milliseconds. I think it would make<br>
sense to add a 'void start(std::chrono::duration)' to the timer class<br>
and simplify these casts. But that doesn't have to be fixed in this<br>
patch.<br>
<br>
Would you like to do that as a patch on top? If you don't want to let me<br>
know and I'll handle it after this patch is merged.<br></blockquote><div><br></div><div>What we've talked about doing is to switch utils::Duration to use an integer<br>underlying type, which would allow implicit conversions and remove the ugly<br>casts like the above. I believe Laurent was still thinking this through as there<br></div><div>were also plans to make utils::Duration part of the public API...</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> +  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  ? FrameMetadata::FrameError<br>
> @@ -1847,7 +1855,7 @@ int V4L2VideoDevice::streamOn()<br>
>  Â  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â state_ = State::Streaming;<br>
> -  Â  Â  Â if (watchdogDuration_.count())<br>
> +  Â  Â  Â if (watchdogDuration_ && !queuedBuffers_.empty())<br>
>  Â  Â  Â  Â  Â  Â  Â  Â watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));<br>
>  <br>
>  Â  Â  Â  Â return 0;<br>
> @@ -1924,7 +1932,7 @@ void V4L2VideoDevice::setDequeueTimeout(utils::Duration timeout)<br>
>  Â  Â  Â  Â watchdogDuration_ = timeout;<br>
>  <br>
>  Â  Â  Â  Â watchdog_.stop();<br>
> -  Â  Â  Â if (watchdogDuration_.count() && state_ == State::Streaming)<br>
> +  Â  Â  Â if (watchdogDuration_ && state_ == State::Streaming && !queuedBuffers_.empty())<br>
>  Â  Â  Â  Â  Â  Â  Â  Â watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(timeout));<br>
>  }<br>
>  <br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>