libjitsi icon indicating copy to clipboard operation
libjitsi copied to clipboard

[RemoteBitrateEstimator] Use the RTP extension abs-send-time to correctly calculate inter-departure time.

Open imedwei opened this issue 8 years ago • 10 comments

Jitsi uses the RTP timestamp for the calculation of inter-departure time. This is not ideal because the RTP timestamp is recorded before the pacer introduces artificial delay in the inter-arrival times. We fixed this by doing the same as Chrome does and use the RTP extension abs-send-time to calculate deltas in the remote bitrate estimator.

With calls on jitsi, the bitrate sometimes will unexpectedly drop. This is most noticeable with screenshare content in the following scenario:

  1. Artificially high inter-group delay variation (d(i) in draft-ietf-rmcat-gcc-02) because the departure times are recorded before the pacer queue. Overuse is asserted.
  2. If overuse occurs during a static screenshare scene, the bitrate drops to 30kbps due to a ratcheting down to the low incoming bitrate.
  3. High dynamic scene changes then cause the encoded bitrate to far exceed the target bitrate.
  4. The pacer limits the transmit rate to the target bitrate which can cause the packets to be sent out over an interval of 5-10 seconds.

Test Plan: Unit test covering the affected logic when we move to the 24-bit abs-send-time. Manual tests of scenarios where the bitrate drop to 30kbps caused high screenshare latency.

imedwei avatar Jan 16 '17 21:01 imedwei

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

jitsi-jenkins avatar Jan 16 '17 21:01 jitsi-jenkins

By the way, we have now signed the Atlassian Corporation CLA for Highfive Technologies, Inc. Hope we start sending many more patches your way.

imedwei avatar Jan 16 '17 21:01 imedwei

Thanks for the contribution! The CLA is probably fine, but jenkins has a separate whitelist...not sure if adding there works right now, so ignore him if he goes on.

Jenkins, add to whitelist please.

bgrozev avatar Jan 16 '17 21:01 bgrozev

Yep we need to add contributors per project using the command in comments or directly into jenkins interface, adding usernames. Maybe we can request a feature in the plugin to have the global admins inherited in all pr-testing projects.

damencho avatar Jan 16 '17 21:01 damencho

I'm not sure how to read what failed in this test. How may I run this test? My local mvn test succeeded.

imedwei avatar Jan 17 '17 02:01 imedwei

Seems like that was a flaky build? When I pushed some minor javadoc changes, the build succeeded.

imedwei avatar Jan 17 '17 03:01 imedwei

@bgrozev Anything we need to do to get this this change upstreamed? We noticed a dramatic improvement in our screenshare latency performance as well as a reduction in bitrate drops for camera video.

imedwei avatar Jan 26 '17 02:01 imedwei

@imedwei after just a quick glance there's one thing that I'd like to avoid -- defining the abs-send-time extension id as a constant in RawPacket. I'm working on adding some code which handles extensions in a somewhat abstract way right now. What do you think about waiting for a couple of days (next week, realisticly) and then refactoring a little to use that? I'll have abs-send-time reading support in mind.

bgrozev avatar Jan 26 '17 15:01 bgrozev

That sounds fine to me. Thanks for the look. Since these are among my first upstream commits to jitsi, I'm grateful for any feedback so that we can improve our current and future commits.

imedwei avatar Jan 26 '17 16:01 imedwei

Hey @imedwei, We just merged the RTP header extensions code I was referring to: https://github.com/jitsi/libjitsi/pull/249 . It should be easy to adapt your code to iterate over the extensions with RawPacket.getHeaderExtensions().

In order to get the negotiated ID of the abs-send-time extension you can expose MediaStreamImpl#absSendTimeEngine to VideoMediaStreamImpl, and then use its extensionID field. I would also prefer to keep the abs-send-time specific code in AbsSendTimeEngine.java.

Some general comments based on our code convention: New files should include the license header. Add yourself as @author. Javadocs on every method/field.

Thanks again for the contribution.

bgrozev avatar Feb 01 '17 18:02 bgrozev