The intended behavior of the pacer is that it should send audio (i.e high priority) packets immediatelly even if the pacer is paused and even if the available media budget has been used. The existing code will not send audio if the pacing budget has been used. It will normally send audio when the pacer is paused, but if the media budget was already used up when the pacer was paused, it would neither send audio nor update the budget.
Updated the implementatation and the unit test. Added logging for when the pacer is paused, and when it receives a zero bitrate estimate.
BUG=webrtc:6155
Review-Url: https://codereview.webrtc.org/2193183003
Cr-Commit-Position: refs/heads/master@{#13592}
Retransmissions are supposed to be sent before normal packets by the pacer, but the current implementation will only use it if the second packet is a retransmission and the first packet is not. It misses the case where the first packet is retransmission and the second packet is not.
This CL fixes the comparator and adds a unit test.
Also changed the SendAndExpectPacket function to propagate the retransmission flag to the expectations. Previously, all packets were expected to be normal packets.
BUG=webrtc:6124
Review-Url: https://codereview.webrtc.org/2156063004
Cr-Commit-Position: refs/heads/master@{#13502}
1. It moves calculation of the needed padding to VideoSendStream instead of ViEEncoder and only does it once per send Stream instead of every time the network estimate changes.
2. The maximum amount of padding sent was prior to this cl calculated and updated based on network estimate changes. However, it can only change based on encoder configuration changes and if send streams are added or removed. This cl change the VideoSendStream/VieEncoder to notify the BitrateAllocator of changes to the needed padding bitrate and for BitrateAllocator to notify Call of these changes.
3. Fixed an issue in the SendPacer where it could send a padding packet before sending a real packet. This caused the test EndToEndTest.RestartingSendStreamPreservesRtpStatesWithRtx to fail with these refactorings since the pacer suddenly could send a padding packet before the encoder had produced its first frame.
BUG=webrtc:5687
Review-Url: https://codereview.webrtc.org/1993113003
Cr-Commit-Position: refs/heads/master@{#13149}
This reverts commit e30c27205148b34ba421184efe65f6a0780b436d (https://codereview.webrtc.org/1958053002/)
Original reverted cl is in patch set #1.
Changes in following patch sets.
The cl now also make sure SendPacer starts with the configured bitrate provided in a call to CongestionController::SetBweBitrates)()
It turns out that the failing tests in 609816 is due to a bug in the current code that runs the proper at 300kbit regardless of configured start bitrate.
Original cl description:
Remove SendPacer from ViEEncoder
This CL moves the logic where the ViEEncoder pause if the pacer is full to the BitrateController. If the queue is full, the controller reports a bitrate of zero to Call (and BitrateAllocator)
BUG=chromium:609816, webrtc:5687
TBR=mflodman@webrtc.org
NOTRY=True // Due to bug in android_x86 cq builder....
Review-Url: https://codereview.webrtc.org/1958113003
Cr-Commit-Position: refs/heads/master@{#12688}
This reverts commit 825eb58d59940a4c3c9837595c4b3b07059c93ca.
This Relands the cl reviewed in https://codereview.webrtc.org/1917793002/
patchset #1 is a pure reland.
patchset #2 fix an overflow in BitrateProber that caused WebRtcVideoChannel2BaseTest.TwoStreamsSendAndReceive to fail.
Original cl description:
Remove SendPacer from ViEEncoder
This CL moves the logic where the ViEEncoder pause if the pacer is full to the BitrateController. If the queue is full, the controller reports a bitrate of zero to Call (and BitrateAllocator)
R=stefan@webrtc.orgTBR=mflodman@webrtc.org
BUG=webrtc:5687
Review URL: https://codereview.webrtc.org/1947873002 .
Cr-Commit-Position: refs/heads/master@{#12630}
This CL moves the logic where the ViEEncoder pause if the pacer is full to the BitrateController. If the queue is full, the controller reports a bitrate of zero to Call (and BitrateAllocator)
BUG=webrtc:5687
Review-Url: https://codereview.webrtc.org/1917793002
Cr-Commit-Position: refs/heads/master@{#12620}
Except in places where this would break out-of-tree code,
such as Chromium.
BUG=webrtc:5520
Review URL: https://codereview.webrtc.org/1785173002
Cr-Commit-Position: refs/heads/master@{#12037}
Reason for revert:
Revert breaks other uses, a fix will be rolled into Chromium instead.
Original issue's description:
> Revert of Remove ignored return code from modules. (patchset #3 id:40001 of https://codereview.webrtc.org/1703833002/ )
>
> Reason for revert:
> Breaks Chromium.
>
> Original issue's description:
> > Remove ignored return code from modules.
> >
> > ModuleProcessImpl doesn't act on return codes and having them around is
> > confusing (it's unclear what an error return code here would do even).
> >
> > BUG=
> > R=tommi@webrtc.org
> >
> > Committed: f14c47a58c
>
> TBR=tommi@webrtc.org,pbos@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=
>
> Committed: https://crrev.com/da33a8a2a22f6d19ba2a8cce963beafbdbaa8fd8
> Cr-Commit-Position: refs/heads/master@{#11761}
TBR=tommi@webrtc.org,torbjorng@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review URL: https://codereview.webrtc.org/1737013002
Cr-Commit-Position: refs/heads/master@{#11762}
Reason for revert:
Breaks Chromium.
Original issue's description:
> Remove ignored return code from modules.
>
> ModuleProcessImpl doesn't act on return codes and having them around is
> confusing (it's unclear what an error return code here would do even).
>
> BUG=
> R=tommi@webrtc.org
>
> Committed: f14c47a58cTBR=tommi@webrtc.org,pbos@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review URL: https://codereview.webrtc.org/1736663004
Cr-Commit-Position: refs/heads/master@{#11761}
ModuleProcessImpl doesn't act on return codes and having them around is
confusing (it's unclear what an error return code here would do even).
BUG=
R=tommi@webrtc.org
Review URL: https://codereview.webrtc.org/1703833002 .
Cr-Commit-Position: refs/heads/master@{#11747}
We should only account for audio packets in the pacer budget if we also
are allocating bandwidth for the audio streams.
BUG=chromium:567659,webrtc:5263
R=mflodman@webrtc.org
Review URL: https://codereview.webrtc.org/1524763002 .
Cr-Commit-Position: refs/heads/master@{#11053}
Currently we limit the enocder so that frames aren't encoded if the
expected pacer queue is longer than 2s. However, if the queue is full
and the bitrate suddenly drops (or there is a large overshoot), the
queue time can be long than the limit.
This CL allows the pacer to temporarily boost the pacing bitrate over
the 2s window.
BUG=
Review URL: https://codereview.webrtc.org/1412293003
Cr-Commit-Position: refs/heads/master@{#10729}
Since the pacer is always enabled, removing enable/disable which makes
all packet queueing succeed. Also renaming one of the ::SendPackets
::InsertPacket to avoid confusion.
BUG=webrtc:1695, webrtc:2629
R=stefan@webrtc.org
Review URL: https://codereview.webrtc.org/1392513002 .
Cr-Commit-Position: refs/heads/master@{#10211}
To make this possible padding only packets will have the same timestamp
as the previously sent media packet, as long as RTX is not enabled. This
has the side effect that if we send only padding for a long time without
sending media, a receive-side jitter buffer could potentially overflow.
In practice this shouldn't be an issue, partly because RTX is recommended and
used by default, but also because padding typically is terminated before being
received by a client. It is also not an issue for bandwidth estimation as long
as abs-send-time is used instead of toffset.
BUG=chromium:425925
R=mflodman@webrtc.org, sprang@webrtc.org, tommi@webrtc.org
Review URL: https://codereview.webrtc.org/1327933003 .
Cr-Commit-Position: refs/heads/master@{#9984}
Packet queue in the paced sender is now based on a priority queue rather than having a separate fifo-queue per priority level. This allows more flexible sorting and cleaner usage.
Packets with earlier capture times are now prioritized higher. In situations with high packet loss, the queue might contain packets from several subsequent frames. Retransmit packets from the earlier frames first, since the later ones will probably be dependent on these.
Also, don't force sending of packets after a certain time of inactivity or when packets grow too old, since this was causing consistent overuse on poor connections. Instead, drop frames in vie encoder if pacer queue is too long.
BUG=
R=mflodman@webrtc.org, stefan@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/27869004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@7617 4adac7df-926f-26a2-2b94-8c16560cd09d
Also changes the name of a variable which has been hijacked by windef.h (included by windows.h), which forces #define near and #define far upon us. This issue was introduced via the following inclusion chain:
bwe_test_framework_unittest.cc includes
paced_sender.h
tick_util.h
windows.h
windef.h
And causes EXPECT_NEAR(foo, bar, near); to expand to EXPECT_NEAR(foo, bar,); generating a very confusing compile error.
R=pbos@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/20849004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@6606 4adac7df-926f-26a2-2b94-8c16560cd09d
New interface uses two bitrates (max/min). The pace multiplier is also
removed from the interface and instead utilized outside. Min bitrate
will be filled with padding if there's not enough media to transmit.
Also fixes a bug in minimum transmission bitrate that made it ignore
REMBs. A regression test has been added to catch it.
BUG=3014
R=mflodman@webrtc.org, stefan@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/10059004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@5723 4adac7df-926f-26a2-2b94-8c16560cd09d
- A bug was introduced in r4234 causing no paced packets to be sent.
- Only update the sequence number counter if a padding packet is actually going to be sent, to avoid packet loss.
- Have all packets go through the pacer if pacing is enabled to avoid reordering.
- Fix race condition on reading capture_time_ms_/timestamp_ in rtp_sender.cc.
BUG=1837
TEST=trybots and vie_auto_test --automated
R=mflodman@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/1682004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@4246 4adac7df-926f-26a2-2b94-8c16560cd09d
