Conversation
Count only one underrun by frame id.
EmmanuelP
force-pushed
the
wip/gv-underrun-fixes
branch
from
afd8a4b to
7743c7c
Compare
There was a problem hiding this comment.
Pull request overview
Adds an explicit “underrun” classification to the GigE Vision streaming path so frames that time out due to late buffer availability can be distinguished from genuine network/packet-loss timeouts (related to #849).
Changes:
- Track a per-frame underrun condition in
arvgvstreamand emitARV_BUFFER_STATUS_UNDERRUNon completion-timeout for those frames. - De-duplicate underrun statistics counting to avoid incrementing underruns for every packet while no input buffers are available.
- Extend
ArvBufferStatuswith a newARV_BUFFER_STATUS_UNDERRUNvalue and document it.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/arvgvstream.c |
Tracks underrun state per frame and maps timeout completion to UNDERRUN when appropriate. |
src/arvbuffer.h |
Adds new public buffer status enum value and gtk-doc entry for underruns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| guint64 underrun_frame_id; | ||
|
|
There was a problem hiding this comment.
underrun_frame_id is indented with spaces while other members in _ArvGvStreamThreadData use tabs. Please fix indentation to match the existing formatting conventions in this struct.
|
|
||
| buffer = arv_stream_pop_input_buffer (thread_data->stream); | ||
| if (buffer == NULL) { | ||
| thread_data->n_underruns++; | ||
|
|
||
| if (thread_data->underrun_frame_id != frame_id) { | ||
| thread_data->n_underruns++; | ||
| thread_data->underrun_frame_id = frame_id; | ||
| } |
There was a problem hiding this comment.
underrun_frame_id is used to de-duplicate underrun counting purely by comparing the raw frame_id. For non-extended IDs the frame id wraps (16-bit), so during long buffer starvation this can undercount underruns and can also mis-tag a later wrapped frame as an underrun. Consider de-duplicating using a monotonic sequence derived from last_frame_id/frame_id_inc (or track an explicit underrun-active generation) instead of the raw frame_id alone.
| arv_warning_stream_thread ("[GvStream::check_frame_completion] Timeout for frame %" | ||
| G_GUINT64_FORMAT " at dt = %" G_GUINT64_FORMAT, |
There was a problem hiding this comment.
When frame->underrun is true the buffer status is set to ARV_BUFFER_STATUS_UNDERRUN, but the log message still says "Timeout for frame". This is misleading for users diagnosing underruns; update the message (and/or include the chosen status) so logs match the emitted buffer status.
| arv_warning_stream_thread ("[GvStream::check_frame_completion] Timeout for frame %" | |
| G_GUINT64_FORMAT " at dt = %" G_GUINT64_FORMAT, | |
| arv_warning_stream_thread ("[GvStream::check_frame_completion] %s for frame %" | |
| G_GUINT64_FORMAT " at dt = %" G_GUINT64_FORMAT, | |
| frame->underrun ? "Underrun" : "Timeout", |
| time_us - frame->last_packet_time_us >= thread_data->frame_retention_us) { | ||
| frame->buffer->priv->status = ARV_BUFFER_STATUS_TIMEOUT; | ||
| frame->buffer->priv->status = frame->underrun ? | ||
| ARV_BUFFER_STATUS_UNDERRUN : |
There was a problem hiding this comment.
This change introduces a new externally visible status (ARV_BUFFER_STATUS_UNDERRUN) and changes the observable outcome of _check_frame_completion. There doesn’t appear to be test coverage ensuring underrun-induced frame completion is classified as UNDERRUN (and not TIMEOUT) in the GigE stream path; adding a regression test (e.g., using the fake GigE stream and deliberately starving input buffers) would help prevent regressions.
| * @ARV_BUFFER_STATUS_FILLING: the image is currently being filled | ||
| * @ARV_BUFFER_STATUS_ABORTED: the filling was aborted before completion | ||
| * @ARV_BUFFER_STATUS_PAYLOAD_NOT_SUPPORTED: payload not yet supported | ||
| * @ARV_BUFFER_STATUS_UNDERRUN: buffer found late in input queue and timeout reached before all packets are received |
There was a problem hiding this comment.
The new ARV_BUFFER_STATUS_UNDERRUN docstring is a bit ambiguous about what condition triggers it. The implementation sets it when no input buffer was available for the frame at some point (buffer underrun) and the frame later hits the retention timeout; consider rewording to reflect that specific trigger so API users can distinguish it from network/packet-loss timeouts.
| * @ARV_BUFFER_STATUS_UNDERRUN: buffer found late in input queue and timeout reached before all packets are received | |
| * @ARV_BUFFER_STATUS_UNDERRUN: at some point no input buffer was available for this frame (buffer underrun), and when a buffer later became available the frame had already exceeded its retention timeout (distinct from network/packet-loss timeouts) |
| gboolean extended_ids; | ||
| gboolean underrun; | ||
| } ArvGvStreamFrameData; |
There was a problem hiding this comment.
The new underrun field is indented with spaces while surrounding struct members use tabs. This makes the struct formatting inconsistent; please align indentation with the rest of the file’s style.
1 participant
An incomplete attempt to detect underrun conditions, as a fix to #849 .
The complete implementation should take resent packets into account.