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

[RTC-333] Limit HLS components to 1 per room #72

Merged
merged 2 commits into from Aug 23, 2023
Merged

Conversation

sgfn
Copy link
Member

@sgfn sgfn commented Aug 23, 2023

No description provided.

@sgfn sgfn requested a review from Karolk99 August 23, 2023 13:08
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #72 (11a2cf4) into main (a16b081) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   83.59%   83.78%   +0.19%     
==========================================
  Files          36       36              
  Lines         512      518       +6     
==========================================
+ Hits          428      434       +6     
  Misses         84       84              
Files Changed Coverage Δ
lib/jellyfish/room.ex 83.76% <100.00%> (+0.72%) ⬆️
.../jellyfish_web/controllers/component_controller.ex 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a16b081...11a2cf4. Read the comment docs.

Comment on lines 394 to 408
defp check_component_allowed(Component.HLS, %{
config: %{video_codec: video_codec},
components: components
}) do
cond do
video_codec != :h264 ->
{:error, :incompatible_codec}

Map.values(components) |> Enum.any?(&(&1.type == Component.HLS)) ->
{:error, :reached_components_limit}

true ->
:ok
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

What about:

Suggested change
defp check_component_allowed(Component.HLS, %{
config: %{video_codec: video_codec},
components: components
}) do
cond do
video_codec != :h264 ->
{:error, :incompatible_codec}
Map.values(components) |> Enum.any?(&(&1.type == Component.HLS)) ->
{:error, :reached_components_limit}
true ->
:ok
end
end
defp check_component_allowed(Component.HLS, %{config: %{video_codec: video_codec}, components: components}) do
hls_components_present = components_includes_HLS?(components)
case {video_codec, hls_components_present} do
{:h264, false} -> :ok
{:h264, true} -> {:error, :reached_components_limit}
_ -> {:error, :incompatible_codec}
end
end
defp components_includes_HLS?(components) do
components
|> Map.values()
|> Enum.any?(&(&1.type == Component.HLS))
end

@sgfn sgfn merged commit a970633 into main Aug 23, 2023
6 checks passed
@sgfn sgfn deleted the sgfn/RTC-333-limit-hls branch August 23, 2023 18:43
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.

None yet

2 participants