Skip to content

Handler: allow spatial layers from 0 to N #149

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.

Sign up for GitHub

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

Closed
aconchillo wants to merge 1 commit into from
Closed

Handler: allow spatial layers from 0 to N #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/Producer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ namespace mediasoupclient
// Paused flag.
bool paused{ false };
// Video Max spatial layer.
uint8_t maxSpatialLayer{ 0 };
uint8_t maxSpatialLayer{ (uint8_t) -1 };
Copy link
Member

@ibc ibc Sep 23, 2022

Choose a reason for hiding this comment

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

Why this syntax? What is this?

Copy link
Author

@aconchillo aconchillo Sep 23, 2022

Choose a reason for hiding this comment

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

This basically means 255. It is (or was) a common way to initialize an unsigned number to its maximum value to mainly indicate it's not initialized.

Previsouly it was 0 and the allowed values were 1, 2 and 3, so this check https://github.com/versatica/libmediasoupclient/blob/v3/src/Producer.cpp#L204 would pass. Now, since we allow 0 as a value, the check would not pass if we set spatialLayer to 0. Another option would be to have a boolean to indicate SetMaxSpatialLayer has been called a first time, but not sure if that's a good option. Or make it a int8_t and don't do this weird cast.

Right now, with 255 (I doubt anyone has 255 layers) we indicate that is not initialized, like in the unit test that I updated.

I'm happy to update the PR with any suggestions. Thanks!

Copy link
Member

@jmillan jmillan Sep 28, 2022

Choose a reason for hiding this comment

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

This change should not be needed. It just represents the maximum spatial layer which at least is 0.

Copy link
Author

@aconchillo aconchillo Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @jmillan. So, if we leave it to 0 and call Producer::setMaxSpatialLayer(0) it seems that the Producer.cpp line I was pointing at above, it would actually not enable the 0 spatialLayer (because maxSpatialLayer is already initialized to 0):

		if (spatialLayer == this->maxSpatialLayer)
			return;

Before, since values were 1, 2 and 3, it will always go through (the first time). Maybe removing that if condition?

// App custom data.
nlohmann::json appData;
};
Expand Down
50 changes: 6 additions & 44 deletions src/Handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,51 +516,13 @@ namespace mediasoupclient
auto* transceiver = localIdIt->second;
auto parameters = transceiver->sender()->GetParameters();

bool hasLowEncoding{ false };
bool hasMediumEncoding{ false };
bool hasHighEncoding{ false };
webrtc::RtpEncodingParameters* lowEncoding{ nullptr };
webrtc::RtpEncodingParameters* mediumEncoding{ nullptr };
webrtc::RtpEncodingParameters* highEncoding{ nullptr };

if (!parameters.encodings.empty())
// Edit encodings. Activate only the layers below or equal to spatialLayer.
if (!parameters.encodings.empty() && (spatialLayer < parameters.encodings.size()))
{
hasLowEncoding = true;
lowEncoding = &parameters.encodings[0];
}

if (parameters.encodings.size() > 1)
{
hasMediumEncoding = true;
mediumEncoding = &parameters.encodings[1];
}

if (parameters.encodings.size() > 2)
{
hasHighEncoding = true;
highEncoding = &parameters.encodings[2];
}

// Edit encodings.
if (spatialLayer == 1u)
{
hasLowEncoding && (lowEncoding->active = true);
hasMediumEncoding && (mediumEncoding->active = false);
hasHighEncoding && (highEncoding->active = false);
}

else if (spatialLayer == 2u)
{
hasLowEncoding && (lowEncoding->active = true);
hasMediumEncoding && (mediumEncoding->active = true);
hasHighEncoding && (highEncoding->active = false);
}

else if (spatialLayer == 3u)
{
hasLowEncoding && (lowEncoding->active = true);
hasMediumEncoding && (mediumEncoding->active = true);
hasHighEncoding && (highEncoding->active = true);
for (int i = parameters.encodings.size() - 1; i >= 0; i--)
{
parameters.encodings[i].active = (i <= spatialLayer);
}
}

auto result = transceiver->sender()->SetParameters(parameters);
Expand Down
2 changes: 1 addition & 1 deletion test/src/mediasoupclient.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ TEST_CASE("mediasoupclient", "[mediasoupclient]")
REQUIRE(audioProducer->GetRtpSender() != nullptr);
REQUIRE(audioProducer->GetTrack() == audioTrack);
REQUIRE(audioProducer->IsPaused());
REQUIRE(audioProducer->GetMaxSpatialLayer() == 0);
REQUIRE(audioProducer->GetMaxSpatialLayer() == (uint8_t) -1);
REQUIRE(audioProducer->GetAppData() == appData);
REQUIRE(audioProducer->GetRtpParameters()["codecs"].size() == 1);

Expand Down