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

Wrap Instants to support distant past and future #617

Closed
wants to merge 2 commits into from
Closed

Conversation

alexlapa
Copy link
Collaborator

Part of #615

As discussed here an here

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.

@alexlapa
Copy link
Collaborator Author

@algesten ,

Okay so atm this is not ready for merge but rather a wip intermediate for further discussion.

The problem is that I'm really not sure that this is going to be a net positive in terms of code base quality:

  1. time_tricks are mostly staying since we have to calculate NTP timestamps on multiple occasions and current implementation seems to be the only sane way. I have some ideas for refactoring but the necessity to correlate Instant to SystemTime will stay anyways.
  2. API surface area is actually pretty wide, and current str0m::Instant => std::Instant works like this, so its pretty much no difference from what it used to be. Alternatively, it may work like str0m::Instant => Option<std::Instant> with obvious downsides of having a dozen or two of new unwraps. Also there is a single but pretty notable case when not_happening() is assigned to a field of an API struct which is a RtpPacket.timestamp. So it will need a pretty extensive refactoring if we want Into<std::Instant> conversion to be failable.

So it seems to me that limited usage of these new types might be a better option, as implemented in #615. But let me know what you think about this.

@algesten
Copy link
Owner

@alexlapa thank you so much for taking the time to look into this.

The problem is that I'm really not sure that this is going to be a net positive in terms of code base quality:

I agree with your assessment. There clearly is a lot more conversion std:time::Instant <-> str0m::Instant doing this than I realized.

I'm sorry to have wasted your time, but this gave me a very useful insight (that I can't take ureq's approach to str0m). Let's roll this back and go with the other solve in #615

@algesten algesten closed this Jan 23, 2025
@alexlapa alexlapa deleted the wrap-instants branch January 24, 2025 06:14
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