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

SimulcastConsumer::GetDesiredBitrate(): Choose the highest bitrate among all Producer streams #992

Merged
merged 13 commits into from
Jan 27, 2023

Conversation

ibc
Copy link
Member

@ibc ibc commented Jan 27, 2023

Instead of assuming that the highest simulcast stream is the one consuming highest bitrate, iterate all them and pick up the highest value.

This is because when the sender enables a higher stream, initially the bitrate of it could be less than the bitrate of a lower stream. In a few seconds it will become higher. However don't report its irrelevant current bitrate in the meantime.

NOTE: This PR is not intended to fix #989

…ong all Producer streams

Instead of assuming that the highest simulcast stream is the one consuming highest bitrate, iterate all them and pick up the highest value.

This is because wWhen the sender enables a higher stream, initially the bitrate of it could be less than the bitrate of a lower stream. In a few seconds it will become higher. However don't report its irrelevant current bitrate in the meantime.
@github-actions
Copy link

Pull reviewers stats

Stats of the last 365 days for mediasoup:

User Total reviews Time to review Total comments
ibc
🥇
67
▀▀▀▀
2h 58m
173
▀▀▀▀▀▀
jmillan
🥈
56
▀▀▀
15h 39m
55
▀▀
nazar-pc
🥉
29
▀▀
2h 20m
41
ggarber
4
3h 11m
3
eli-schwartz
2
1h 34m
2
fippo
1
8h 29m
2
SteveMcFarlin
1
2d 8h 1m
3
Hartigan
1
2d 5h 7m
6
balazskreith
1
55d 2h 38m
▀▀▀▀▀▀▀▀▀
12

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

@nazar-pc I strongly think we should freeze Rust version in mediasoup-rust to avoid these unexpected and totally unrelated CI errors: https://github.com/versatica/mediasoup/actions/runs/4023390642/jobs/6914204443

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Sure, I'll add config that locks the version so we can update it manually. These changes are easy to fix though, just run cargo clippy --all-targets and fix things until it stops complaining.

The changes here make sense to me.

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

Problem is that cargo clippy works for me in my computer. How should I update Rust? Via brew install rust? Or brew install cargo?

@nazar-pc
Copy link
Collaborator

New version was out yesterday with new lints enabled by default: https://blog.rust-lang.org/2023/01/26/Rust-1.67.0.html
Updating can be done easily with rustup update.

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

@nazar-pc I need help here:
CleanShot-2023-01-27-at-12 25 50@2x

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

@nazar-pc I need help here: CleanShot-2023-01-27-at-12 25 50@2x

Trying to apply this change:

--- a/rust/src/worker.rs
+++ b/rust/src/worker.rs
@@ -500,8 +500,7 @@ impl Inner {
                     Err(error) => Err(io::Error::new(
                         io::ErrorKind::Other,
                         format!(
-                            "unexpected first notification from worker [id:{}]: {:?}; error = {}",
-                            id, notification, error
+                            "unexpected first notification from worker [id:{id}]: {notification}; error = {error}"
                         ),
                     )),
                 };

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

I've tried this(as per output help):
CleanShot-2023-01-27-at-12 30 09@2x
then it complains again with:
CleanShot-2023-01-27-at-12 30 46@2x

@nazar-pc
Copy link
Collaborator

nazar-pc commented Jan 27, 2023

It was {:?}, which is print debug version of the data, not regular ({}), so it should be {notification:?}:

                        format!(
                            "unexpected first notification from worker [id:{id}]: {notification:?}; \
                            error = {error}"
                        ),

When strings are longer than configured length formatter gives up formatting other code around it sometimes, so better splitting it into multiple lines explicitly in those cases.

UPD: Error message actually suggested :?, I guess you overlooked it due to 2 other variables.

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

Thanks @nazar-pc, pushed. Should be green now.

@nazar-pc
Copy link
Collaborator

Rust is not happy with your skills today, sir. Use cargo clippy --all-targets at the root of the repo in case you run it under rust only

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

Did cargo clippy --all-targets ignore rust/tests folder because I executed it in rust folder? WHAT? How can be this language legal?

@nazar-pc
Copy link
Collaborator

I don't think that was the case, tests are covered by that command.
CI just runs that command in the root of the repo and refuses to accept any warnings.
I think you're close though.

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

CleanShot-2023-01-27-at-14 11 35@2x

CleanShot-2023-01-27-at-14 12 00@2x

Really???

@nazar-pc
Copy link
Collaborator

Only plain variables can be used, not expressions or property accesses like self.id. Let me fix those for you and push a commit into this branch.

… adding basic support to print things now in 2023
@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

All fixed in last commit.

nazar-pc and others added 2 commits January 27, 2023 15:28
…com:versatica/mediasoup into improve-simulcastconsumer-getdesiredbitrate

# Conflicts:
#	worker/build.rs
@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

@nazar-pc
Copy link
Collaborator

This error didn't show locally: https://github.com/versatica/mediasoup/actions/runs/4024845039/jobs/6917332591

Because it is platform-specific code. Unless you're on Windows, it is not compiled.

rust-toolchain.toml will make sure cargo downloads and uses desired version of Rust toolchain and components necessary for operation, including for CI to run successfully. There should be no guessing or mismatched tooling versions anymore.

@ibc
Copy link
Member Author

ibc commented Jan 27, 2023

rust-toolchain.toml will make sure cargo downloads and uses desired version of Rust toolchain and components necessary for operation, including for CI to run successfully. There should be no guessing or mismatched tooling versions anymore.

amazing

@nazar-pc
Copy link
Collaborator

All 🟢

@ibc ibc merged commit 8287299 into v3 Jan 27, 2023
@ibc ibc deleted the improve-simulcastconsumer-getdesiredbitrate branch January 27, 2023 14:21
piranna pushed a commit to dyte-in/mediasoup that referenced this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency of Sending Bitate with Available Outgoing Bitrate
3 participants