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

Support negative time deltas in BWE #615

Merged
merged 6 commits into from
Jan 24, 2025
Merged

Support negative time deltas in BWE #615

merged 6 commits into from
Jan 24, 2025

Conversation

alexlapa
Copy link
Collaborator

So first of all we should be able to handle negative send deltas in ArrivalGroup as discussed before: 0, 1, 2

So this is pretty much what this PR does so far, but it might be extended as discussed here.

So, I can leave it as it is, right now it only affects packet/bwe and these new types are only used where it's expected that negative time deltas are possible. Alternatively:

  1. Always prefer using these new types instead of std's in BWE? They work pretty much like libwebrtc's Timestamp and TimeDelta so they should fit.
  2. Also use it to cover similar concepts as not_happening(), already_happened()? I haven't really though about it but first thing that comes to mind is just using SuperInstant instead of those Instants that can be already_happened or not_happening with some simple conversion SuperInstant::as_instant() -> Instant that will just return already_happened || Exact(Instatnt) || not_happening. Might also just put a Deref -> &Instant on it.

So let me know what you think about this.

Also I've returned that ArrivalGroupAccumulator test on libwebrtc captured data, but now it also runs on negative send deltas. Ill remove it before merge.

@algesten
Copy link
Owner

Looks good.

Yeah. So I want to replace as much as possible of the time_hacks.rs. Ideally I'd like to call these things Instant and Duration (instead of SuperInstant and TimeDelta). The names deliberately shadow the std-lib ones because they should function 99% like the std-lib does. That means a casual reader of the code should not be surprised by new time types.

Our external API is however std-lib Instant and Duration, which means there's work to go through exactly all the internal code where this would affect.

I'd prefer if the SuperInstant/TimeDelta could be moved to a separate PR that handles that and leave this PR to only deal with the BWE consequences.

@thomaseizinger
Copy link
Collaborator

Our external API is however std-lib Instant and Duration, which means there's work to go through exactly all the internal code where this would affect.

Might be worth adopting something like https://github.com/obi1kenobi/cargo-semver-checks to ensure changes to the public API are automatically linted!

@alexlapa
Copy link
Collaborator Author

Might be worth adopting something like https://github.com/obi1kenobi/cargo-semver-checks to ensure changes to the public API are automatically linted!

Well in this specific case E0446 should be enough I suppose.

@algesten
Copy link
Owner

Given the findings in #617. Let's go further with this PR by making the negative-time handling variants local to the BWE module. SuperInstant is maybe a questionable name, but TimeDelta is good. Let's isolate them to bwe module so they don't spread across the rest of the code base for now.

And again, sorry for making you doing a wild goose chase in a direction that doesn't work.

@alexlapa
Copy link
Collaborator Author

SuperInstant is maybe a questionable name

That's how a similar thing was named before. I would probably go with Timestamp since thats how it's named in libwebrtc. Let me know if you'd like to have a different name.

@alexlapa alexlapa marked this pull request as ready for review January 24, 2025 10:15
@alexlapa alexlapa requested a review from algesten January 24, 2025 10:15
@algesten
Copy link
Owner

SuperInstant is maybe a questionable name

That's how a similar thing was named before. I would probably go with Timestamp since thats how it's named in libwebrtc. Let me know if you'd like to have a different name.

Let's rename and stick closer to libWebRTC.

Copy link
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Some minor comments

src/packet/bwe/time.rs Outdated Show resolved Hide resolved
src/packet/bwe/time.rs Outdated Show resolved Hide resolved
src/packet/bwe/time.rs Outdated Show resolved Hide resolved
src/packet/bwe/time.rs Outdated Show resolved Hide resolved
src/packet/bwe/time.rs Show resolved Hide resolved
@alexlapa alexlapa requested a review from algesten January 24, 2025 13:47
@algesten algesten merged commit d433ab4 into main Jan 24, 2025
25 checks passed
@algesten algesten deleted the negative-time-deltas branch January 24, 2025 13:55
@algesten
Copy link
Owner

@alexlapa Thanks!!!

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.

3 participants