Skip to content

Wip/gv underrun fixes #1009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
EmmanuelP wants to merge 2 commits into
base: main
Choose a base branch
from
+17 −4
Draft

Wip/gv underrun fixes #1009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7238be
gvstream: fix underrun count
EmmanuelP May 13, 2025
7743c7c
gvstream: mark buffer status as underrun if buffer was late in input …
EmmanuelP May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/arvbuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ G_BEGIN_DECLS
* @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
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
* @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)

Copilot uses AI. Check for mistakes.
*/

typedef enum {
Expand All @@ -56,7 +57,8 @@ typedef enum {
ARV_BUFFER_STATUS_SIZE_MISMATCH,
ARV_BUFFER_STATUS_FILLING,
ARV_BUFFER_STATUS_ABORTED,
ARV_BUFFER_STATUS_PAYLOAD_NOT_SUPPORTED
ARV_BUFFER_STATUS_PAYLOAD_NOT_SUPPORTED,
ARV_BUFFER_STATUS_UNDERRUN
} ArvBufferStatus;

/**
Expand Down
17 changes: 14 additions & 3 deletions src/arvgvstream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ typedef struct {
gboolean resend_ratio_reached;

gboolean extended_ids;
gboolean underrun;
} ArvGvStreamFrameData;
Comment on lines 125 to 127
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

struct _ArvGvStreamThreadData {
Expand Down Expand Up @@ -162,6 +163,8 @@ struct _ArvGvStreamThreadData {

gboolean use_packet_socket;

guint64 underrun_frame_id;

Comment on lines 165 to +167
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
/* Statistics */

guint64 n_completed_buffers;
Expand Down Expand Up @@ -374,8 +377,10 @@ _find_frame_data (ArvGvStreamThreadData *thread_data,

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;
}
Comment on lines 377 to +383
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return NULL;
}

Expand All @@ -389,11 +394,13 @@ _find_frame_data (ArvGvStreamThreadData *thread_data,
thread_data->callback (thread_data->callback_data,
ARV_STREAM_CALLBACK_TYPE_BUFFER_DONE,
buffer);
thread_data->underrun_frame_id = 0;
return NULL;
}

frame = g_new0 (ArvGvStreamFrameData, 1);

frame->underrun = thread_data->underrun_frame_id == frame_id;
frame->disable_resend_request = FALSE;

frame->frame_id = frame_id;
Expand Down Expand Up @@ -432,6 +439,8 @@ _find_frame_data (ArvGvStreamThreadData *thread_data,

arv_histogram_fill (thread_data->histogram, 1, 0);

thread_data->underrun_frame_id = 0;

return frame;
}

Expand Down Expand Up @@ -834,7 +843,9 @@ _check_frame_completion (ArvGvStreamThreadData *thread_data,
* acquisition start. */
(frame->frame_id != thread_data->last_frame_id || frame->last_valid_packet != 0) &&
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 :
Comment on lines 845 to +847
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
ARV_BUFFER_STATUS_TIMEOUT;
arv_warning_stream_thread ("[GvStream::check_frame_completion] Timeout for frame %"
G_GUINT64_FORMAT " at dt = %" G_GUINT64_FORMAT,
Comment on lines 849 to 850
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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",

Copilot uses AI. Check for mistakes.
frame->frame_id, time_us - frame->first_packet_time_us);
Expand Down
Loading