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

Patches #2

Open
wants to merge 2 commits into
base: patches
Choose a base branch
from
Open

Patches #2

wants to merge 2 commits into from

Conversation

imeteora
Copy link

No description provided.

…esting

 * don't resize input frames to a "standard" WebRTC resolution

 * make SrtpSession::Init public so we can call it before any peers
   are created and thereby avoid a data race

 * make PacedSender::ShouldSendNextPacket notify listener that the
   queue has been flushed.  This allows us to throttle frame creation
   so we don't get ahead of the network and introduce unecessary
   latency.  It's a total hack, though, since it only allows one
   listener per process, whereas we would preferably support multiple
   peers, each listening to its own PacedSender.

 * use the default vp8 config instead of overriding the defaults.  CBR
   does not seem to work as well for screen content, and none of the
   other overrides seem to add any value -- this may be worth
   revisiting, though.

 * don't delay rendering video frames on the decode side.  This
   speeds up the integration test by avoiding added latency, but would
   have no effect on a stand-alone screen capture program, since it
   wouldn't include a decoder.
ecovate::Listener is a hack to allow us to listen for certain events
that the WebRTC stack doesn't ordinarly report to the application,
such as when the network layer is ready to send and when it's ready
for more data.  Ideally, we'd pass this listener all the way down the
stack, but that would require changing a lot of code, so we use a
hack: acquire a global lock and update a global reference to the
desired listener before calling PeerConnection::SetLocalDescription,
then clear the global reference and global lock.  That way, each
instance of PeerConnection gets a listener appropriate to that
listener, whereas the previous implementation only worked if there was
only one PeerConnection per process.

This commit also includes a couple of minor changes to ensure every
frame the application gives to the WebRTC stack makes it all the way
to the network (e.g. no frame dropping and no multithreading-induced
nondeterminism).  This makes writing a reliable integration test much
easier and aids debugging.
@dicej
Copy link
Member

dicej commented Dec 17, 2014

Hi. I'm a little confused about this pull request. The patches branch already has these changes, but has been rebased against a newer version of the upstream code from https://chromium.googlesource.com/external/webrtc.

Also, I didn't realize anyone outside ReadyTalk was using these patches, so I didn't realize that rebasing might cause confusion. I'll just do regular merges going forward if you and/or others are also using this code.

dicej pushed a commit that referenced this pull request May 14, 2015
ViEChannels without default encoders doesn't register a receive codec by
default. This makes VideoReceiver::Decode return early, causing a
high-priority thread to effectively be busy looping. This would be
expected to wreck more havoc in a more cross-platform manner than it has
visibly done. On Windows XP however it manages to bring the whole
machine to a grinding halt forcing a reboot if CPU usage hits 100%.

BUG=chromium:470013
[email protected]

Review URL: https://webrtc-codereview.appspot.com/48049004

Cr-Commit-Position: refs/heads/master@{#8976}
(cherry picked from commit 3949e86)

Review URL: https://webrtc-codereview.appspot.com/46009004

Cr-Commit-Position: refs/branch-heads/43@{#2}
Cr-Branched-From: 7351f46-refs/heads/master@{#8926}
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