Added dynamic resampling

[aw-sohonet]

Added a dynamic resampling method that will control the amount of samples in the buffer for Decklink cards so that any drift occurring that causes overflows or underflows can be avoided.

It's worth noting that while this fix works well for Decklink cards, for 32bit Audio support there is a conversion between 32Bit integers and floats so that the Speex float resampler can be used (before a conversion back). If not all cards produce 32Bit integer audio, then this will cause any resampling that happens with these cards to break.

In longer term testing this has produced desirable results, which removes any popping, or glitches in the audio.

The threaded performance improvements make it so that the highest quality resampling can be used with minimal impact. Threaded vs unthreaded for quality 10 gave an increase in performance of roughly 250% (across 16 channels of data).

Fixed

• Fixed resampler outputting the zero bytes it inserts at the beginning of the resample period.
• Fixed remainder audio being added back to the next audio frame regardless of whether or not resampling is occuring.

Changed

• Changed the 'drift_fix' option to be 'no_drift_fix' and for the drift fix for decklink cards to always be active.

Added

• Added scaling support for the deck drift audio fixer class. This will now scale the amount required to resample based on the distance between the target buffer and the average of the target buffer (within the last 10 seconds).
• Added threading for resampling across multiple channels (significant performance increase noted).
• Added support for 32bit Integer resampling (by conversion to 32bit Float).
• Added removal of the tail buffer of audio from the resampler when switching between different sample rates.
• Added new parameters for the decklink:
  • 'targetbuffer' - The target amount of samples to have in the buffer (per channel).
  • 'minresample' - The minimum amount the resample delta can be when scaling is applied. Measured in Hz.
  • 'maxresample' - The maximum amount the resample delta can be when scaling is applied. Measured in Hz.
• Added statistics for tracking a number of useful cumulative stats for the decklink card. Included is the amount of times requests for resampling have been committed, underflows, overflows, etc.
• Added new functions to the audio_frame2_resampler class for use when querying its state.

[alatteri]

are there recommended values for the new settings?

'targetbuffer' - The target amount of samples to have in the buffer (per channel).
'minresample' - The minimum amount the resample delta can be when scaling is applied. Measured in Hz.
'maxresample' - The maximum amount the resample delta can be when scaling is applied. Measured in Hz.

[benroeder]

I would leave the defaults, but they are there if you need to tune them @alatteri

[aw-sohonet]

@alatteri - To expand on what Ben has said.

The default for the targetbuffer for a Decklink is currently set to 2700 samples (per channel). The drift code itself actually will try to stay within 600 samples of this target before it will even attempt to resample. You can tweak it but my recommendation is to stick with the default as this got us good results.

The minresample is set to be 1Hz by default. The maxresample is set to be 30Hz by default. These are deltas from the existing sample rate for however you intend to sway the buffer. As the drift happens over a very long period of time, you generally should not have a need to resample more than this. As mentioned above, no resampling even occurs if you're inside of the target range (default 2100 - 3300 samples per channel in the buffer). Once you leave, the drift fixing class will calculate the distance you are away from this range and will resample accordingly. It scales the distance between (100 and 600 samples) and applies a min and max resample delta based on this. So at a distance of 100 samples it would apply a 1Hz (or whatever minresample is set to) delta to try and correct the drift. At a distance of 600 it will apply 30Hz (or whatever maxresample is set to). Currently I haven't set either the distance from the target buffer or the min/max of the scaling calculation to be parameterised, but the values work well enough during our testing.

As said, the recommendation is to leave them as the defaults as our testing has been fairly successful so far, but hopefully the above nicely explains what the defaults are and the impact they have when you change them.

[alatteri]

@aw-sohonet Thanks for the detailed response, and for coding up this fix. Eager for the Pull to be rolled into master.

[benroeder]

@MartinPulec does this make sense to you ?

[MartinPulec]

I think it may be merged sooner or later. However, the merge request has 40 commits while altering also some of core audio processing internals, so we definitely need to review it. We may also want to slightly modify the code, eg. these:

• not sure if it is wise to keep the resampling default
• log levels are a bit arguable - logging as error non-error, flooding the output in verbose mode (verbose should be only a bit more chatty than "info", but should be still human-observable without filtering)
• I think that parts definitely need to go into the separate file, eg. the decklink fixer itself, which is a bit hack anyways
• complexity of some parts of code are significantly increased - eg. decode_audio_frame, maybe just separating to a function would help
• the changes in types.cpp will also need to be revised since it is a core

Just for curiosity, why I am author of bbc9184dd? It is possible that I've done something wrong to commandeer it, but it seems to be so also in the original repo.

[aw-sohonet]

Hi @MartinPulec,

Thanks for getting back! Just to respond to a couple of your points:

• When you say about the resampling default, do you mean that you're not keen to always apply a drift fix? I have noticed that in other places a drift fix is referred to as a "framesync". Perhaps this is a nicer name?
• The point you make about the log levels is a fair point. I will change the output that is spammed into the log to DEBUG.
• I agree with your statement about moving the Decklink fixer class into it's own file. I think I made it generic enough that it generally does not apply only to the Decklink anyway. I kept it there for now, as it does currently only apply to the Decklink.
• I can extrapolate the resampling done into a separate function if you think it will reduce the complexity. Obviously this will need to be carefully reviewed still as it's in your core loop.
• Which changes in types.cpp do you think need to be revised?

I tried merging all of the commits into a single commit. I was attempting to exclude the change you made at the beginning of the branch and only merge down my commits, but that did not work and instead it added an additional commit (which included your initial commits on the branch).

[alatteri]

Hey everyone.... Any movement on this? It would be a great benefit to get this mainlined as we still get the buffer overflows with Decklink.

[MartinPulec]

Would it be possible rather to push as a merge request only the unmerged branch (eg. commit b53aa634a)? I think that ideally the merge request should be a straight branch without branching.

[aw-sohonet]

Hi Martin. Sorry I'm not entirely sure what you mean. The commit b39af61 is a resolution of merge conflicts with master, so it is not possible to merge the branch without having first resolved those conflicts.

I can roll back this PR, to be at the commit you asked for by rewriting the history, but I would then need to recommit the changes I have made in order to resolve the conflicts (which I notice there are more since I last resolved the conflicts).

Please let me know if I have interpreted what you are saying incorrectly!

[MartinPulec]

I think that it is a bit misunderstanding - I meant the commit b53aa63, not b39af61 (the merge commit), unfortunately the hashes are similar.

In my opinion, the pull request doesn't need to be merged/mergeable. If you do the merge with master, after the merge of the pull request I'd need it once again, since has been merged with an older version. I've just tried to request a pull requeste #246 and it seems that only decklink.cpp is conflicting. Is it be possible to issue the pull request like that?

[aw-sohonet]

Hi Martin, I have forced the commit back to the hash that you asked for! It should now as I left it without the merged changes from master. Is this what you were after?

[MartinPulec]

Is this what you were after?

exactly! thanks

anyways, could you make one more change? Would it be possible to remove the commit bbc9184d? I believe that this is just squashed commits 44ae96d3..3a6ad8449 (44ae96d3 is a common ancestor for both). Both references, which contain the same code, are then merged together.

I've already tried the removal in #247 and it works flawlessly. Or is there a reason to have merged together the same set of changes once squashed and in the other branch unsquashed? If bbc9184d is held for the summary commit message, I'd see better options, like creating an extra commit with that commit message and eg. dropping a line to NEWS.

[aw-sohonet]

@MartinPulec - I have done as you asked and removed the squash commit. It appears to have deleted every comment.

[MartinPulec]

@MartinPulec - I have done as you asked and removed the squash commit. It appears to have deleted every comment.

I think it doesn't matter much.

Anyways, sorry for the remarks but I don't think it is ideal yet. I think that the merge commit is still there (bbc9184d) but also most commits go to the final merge in 2 paths, eg b53aa634 and 11efbce61 (and are perhaps also included in the squash). Would it be possible to do something like (or exactly so as) the tag merge-test2? You can look at it with either gitk or git log --graph:

git fetch --tags https://github.com/MartinPulec/UltraGrid.git
git log --graph merge-test2
git fetch https://github.com/aw-sohonet/UltraGrid.git wip-deck-drift
git log --graph FETCH_HEAD

The first one is almost linear, while the second is complex - it may not be a problem if those were independent changes, but here the point is that most changes are there included in 2 (or even 3) paths.

[aw-sohonet]

Hi @MartinPulec - The git graph when I pushed it looked like what you had requested. - Hopefully it looks like you want it to be 😄! Let me know if there's anything else that needs to be done.

[alatteri]

Hi everyone.... just wondering if we can get any movement on this... would be wonderful to have this fix in master.

Thanks!

[MartinPulec]

Hi @alatteri, I think that it will be quite soon. But I don't want to give some promises now, I am doing a few slight changes. In the end, if it is important for you, you can use this forked version for now (or this version, which includes some of my changes).

[MartinPulec]

Merged, with some additional changes described below.

Functional:

• drift fix it is disabled by default
• passing wrong integer values to related options of the decklink display causes exceptions not continuation with default values
• speex was re-added and is default if both speex and sox are compiled in (use --param resampler=soxr to select sox) - on one hand sox is presumably better in terms of performance and quality but it adds 20 ms latency
• changed various logging spots

Non-functional:

• added to CI + removed a submodule (not sure what was the intent of it)
• removed some unused code (including some logging that has low information value like measuring putf duration, which is non-blocking)
• separated resampler(s) logic to a separate file
• ditto drift fixer and summary

I wasn't sure about DecklinkAudioSummary calls in display_decklink_put_audio_frame - I've moved them all to AudioDriftFixer::update. It really helps to separates the code and it can be guarded by a single variable.

Finally, I didn't test thoroughly the drift-fixer merge and subsequent changes if it still work since I don't have appropriate test setup. But if something breaks it shouldn't be hard to fix. Feel free to post eventual changes as a a pull requests.

[alatteri]

• drift fix it is disabled by default

What is the option to enable it?

• speex was re-added and is default if both speex and sox are compiled in (use --param resampler=soxr to select sox) - on one hand sox is presumably better in terms of performance and quality but it adds 20 ms latency

Is the latency automatically accounted for, are do we need to do a manual delay option to resync the video?

[MartinPulec]

What is the option to enable it?

It is documented – -d decklink:drift_fix

Is the latency automatically accounted for, are do we need to do a manual delay option to resync the video?

You mean if UltraGrid adds extra latency for video? No, it doesn't. I don't think it would make some sense to do it (proactively) because audio and video is processed independently so the A/V sync is a bit indeterminate (DeckLink audio buffer is 2 or even 3 video frames long).

[aw-sohonet]

@MartinPulec - Thanks for completing the merge conflict work and merging! It is much appreciated 🎉

[alatteri]

OK...so if we use sox, is the latency a consistent 20ms? Which means to maintain A/V since we would add "--audio-delay 20" or do we just need to test and adjust, or am I misunderstanding the situation?

You mean if UltraGrid adds extra latency for video? No, it doesn't. I don't think it would make some sense to do it (proactively) because audio and video is processed independently so the A/V sync is a bit indeterminate (DeckLink audio buffer is 2 or even 3 video frames long).

[MartinPulec]

OK...so if we use sox, is the latency a consistent 20ms?

Didn't tried myself but there is an evaluation showing that.

I guess rather --audio-delay -20 or am I wrong? But I think rather B is correct because I don't know how stable/exact the delay will be and how full does the resampler keep the buffer – perhaps guys from Sohonet would know?

[aw-sohonet]

From our testing the 20ms latency is a little overblown, but obviously this depends on your hardware. From our testing, when using threading, the latency is anywhere between 6ms and 10ms (although I'm doing this from memory).

I don't know if Martin restored the Speex resampler with the threading work I did, but that gives a significant boost on higher channel counts as well. The reason we did not use Speex though is that variable sample rates causes the resampler to insert a small anomaly when it was destroyed.