Skip to content
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.

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

tmr: add tmr_continue() #754

Merged
merged 1 commit into from
Apr 7, 2023
Merged

tmr: add tmr_continue() #754

merged 1 commit into from
Apr 7, 2023

Conversation

vanrein
Copy link
Contributor

@vanrein vanrein commented Apr 6, 2023

to allow jiffie-accurate interval timer continuation
usually called from within the handler function

RTP timing can normally be derived best from an audio source.
This does not work when delivering content from a file, or when
delivering generated content.

The tmr_continue() call is a slight modification of tmr_start()
and can be used to continue a timer after an internal, which is
added to the current timeout, avoiding stacking timing errors.

@vanrein vanrein force-pushed the tmr_continue branch 2 times, most recently from 29f1276 to 27faf00 Compare April 6, 2023 20:54
@vanrein
Copy link
Contributor Author

vanrein commented Apr 6, 2023

Notes:

  1. The FreeBSD check that failed does not look like I caused it.
  2. For fresh software I would have included the bool syncnow flag in tmr_start_dbg() but I chose to keep the ABI in tact and so use an extra function call to share code.

@sreimers
Copy link
Member

sreimers commented Apr 7, 2023

I wonder if tmr_continue() helps. timers are not very accurate, since the main event loop runs multiple tasks, so its normal that a tmr event can be late. If you need accurate timing, its better to use a separate thread with sys_msleep(), especially if blocking filesystem access is involved.

@vanrein
Copy link
Contributor Author

vanrein commented Apr 7, 2023

I think you are talking about jitter, and I agree that this is not addressed by this patch. The common resolution for transmission+network jitter is a buffer on the receiving end.

But to send sound file contents or generated audio, there is no tool in libre. What is needed, is a way to have a regular interval, so as to maintain an average speed. If this is done with repeated calls to tmr_start() then jitter piles up without hope for correction. The proposed tmr_continue() still introduces jitter, but it does not pile up, so jitter correction at the receiver is possible.

I also got this working with setitimer(ITIMER_REAL) and signal() handling SIGALRM, but it feels awkward to mix concurrency models. Moreover, the behaviour I had was unpredictably wrong, with restarts of the transmitted data, possibly due to subtle memory/cache problems. With tmr_continue() I am not seeing the file restarts that I noticed with a signal handler, and that screwed up the sound.

I think that tmr_continue() adds value, but also agree that it is subtle.

src/tmr/tmr.c Outdated
void tmr_start_dbg(struct tmr *tmr, uint64_t delay, tmr_h *th, void *arg,
const char *file, int line)
{
tmr_startcont_dbg (tmr, delay, true, th, arg, file, line);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tmr_startcont_dbg (tmr, delay, true, th, arg, file, line);
tmr_startcont_dbg(tmr, delay, true, th, arg, file, line);

src/tmr/tmr.c Outdated Show resolved Hide resolved
@vanrein
Copy link
Contributor Author

vanrein commented Apr 7, 2023

FYI, I considered another formulation in tmr_start() but that would make timer continuation the default and that might break applications that assume that they may reuse a timer after a period of non-use.

    if (tmr->jfs == 0)
        tmr->jfs = tmr_jiffies ();
    tmr->jfs += period;

This would recognise the all-zero situation after tmr_init().

Having no detailed documentation the (very readable) code serves as an API, so I thought it better to not break with existing behaviour. A lighter variant, but possibly even less predictable, would have been

    uint64_t jfs_now = tmr_jiffies ();
    if (tmr->jfs + period < now)
        tmr->jfs = now;
    tmr->jfs += period;

The benefits of the form in this pull request are unaltered and more consistent behaviour and fewer calls to tmr_jiffies().

@sreimers sreimers changed the title Add tmr_continue() and tmr_continue_dbg() tmr: add tmr_continue() Apr 7, 2023
to allow jiffie-accurate interval timer continuation
usually called from within the handler function
@sreimers sreimers merged commit 0fe696c into baresip:main Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants