Send unpaced media as padding. #395
Conversation
src/packet/pacer.rs
Outdated
| QueuePriority::Empty => true, | ||
| }; | ||
| q.snapshot.packet_count > 0 && is_none_empty_queue | ||
| }); |
There was a problem hiding this comment.
At the moment this change looks a bit like it does nothing. We remove !q.unpaced, but then, if there were any content in an unpaced queue, we would have returned earlier. The is_none_empty_queue bool is always true, should we maybe consider this PR a draft given the comment?
There was a problem hiding this comment.
The key to the PR is that "&& !q.unpaced" is removed. That's what fixes that hot loop that's triggered by enabling audio RTX with BWE. But Hugo suggested I also filter out queues with QueuePriority::Padding. But that caused the BWE test called test_realistic to fail.
I would be happy to remove the QueuePriority stuff and just reduce this PR down to removing "&& !q.unpaced", because that's what's important to the fix, and perhaps leave a TODO to look into filtering by QueuePriority.
There was a problem hiding this comment.
This prevents a hot loop when you enable RTX for audio and BWE/pacing at the same time without having to know to call .set_unpaced(false) for the audio.
Is the starting point here right? Is it really meaning to change !q.unpaced?
- RTX is required for BWE
.set_unpaced(false)is required for BWE
Should the default unpaced flag logic be different for this scenario?
We of course want no hot looping, but is the solve here the right one?
There was a problem hiding this comment.
The current behaviour is:
- We only do BWE calculations on video media
- We only pace video media
- We only do RTX on video media
At the moment this change looks a bit like it does nothing. We remove !q.unpaced, but then, if there were any content in an unpaced queue, we would have returned earlier. The is_none_empty_queue bool is always true, should we maybe consider this PR a draft given the comment?
The root cause that @pthatcher identified is that we don't set a reasonable fake value for first_unsent when an audio queue only contains padding. We could fix it that way, but that would introduce a new problem(we would prioritize queued padding on an audio queue over queued media on a video queue). This is why we need to remove the !q.unpaced condition.
The priority stuff is not correct I think, just remove that, we already have code that will select the queue with the highest priority.
There was a problem hiding this comment.
- We only do BWE calculations on video media
- We only pace video media
- We only do RTX on video media
It is possible to enable RTX for audio (point 3) and it's possible to turn on pacing for audio (point 2). The only point I'm unsure about is 1 – you mean we don't produce queue stats for audio?
There was a problem hiding this comment.
The only point I'm unsure about is 1 – you mean we don't produce queue stats for audio?
No, I was under the impression that we didn't enable TWCC for audio and it therefore wasn't counted, seems we do and I was wrong. It might have been that browsers themselves didn't use to negotiate TWCC for audio
There was a problem hiding this comment.
I removed the QueuePriority stuff. Now the PR just removes the !unpaced check (other than some name/comment cleanup).
By the way, libwebrtc used to not negotiate TWCC for audio by default, but it was always possible to do so (with SDP munging). It does so by default now, last I checked. But that was irrelevant to how I found the bug, which was using the Direct API to enable audio RTX and audio TWCC directly. It will always be possible to enable audio RTX with audio TWCC together.
The real question is: if audio RTX and audio TWCC are enabled, can audio be unpaced? If so, then it shouldn't result in a hot loop. If not, then we need a way of making sure audio is paced when audio TWCC is enabled.
There was a problem hiding this comment.
The real question is: if audio RTX and audio TWCC are enabled, can audio be unpaced? If so, then it shouldn't result in a hot loop. If not, then we need a way of making sure audio is paced when audio TWCC is enabled.
Yes it can be, but in theory it shouldn't be. I think the bandwidth of audio might be low enough that it does not matter. The pacing is mostly about avoiding overloading the link when sending large keyframes after all
pthatcher
force-pushed
the
peter/send-padding-with-unpaced-media-pr
branch
from
6b31b6e
to
8d23067
Compare
This prevents a hot loop when you enable RTX for audio and BWE/pacing at the same time without having to know to call .set_unpaced(false) for the audio.