Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Live compositor #291

Merged
merged 20 commits into from
Jul 11, 2023
Merged

Live compositor #291

merged 20 commits into from
Jul 11, 2023

Conversation

Karolk99
Copy link
Contributor

@Karolk99 Karolk99 commented Jul 4, 2023

No description provided.

Comment on lines 56 to 63
@impl true
def handle_inputs_change(
inputs,
_ctx,
%{screenShare?: true} = state
) do
{inputs, state}
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be modified? Because if I understand correctly, it won't even work, because inputs aren't a Scene struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should have been removed but I missed it

Comment on lines +90 to +139
defp get_desired_stream_format(1, output_stream_format) do
output_stream_format
end

defp get_desired_stream_format(2, %{width: width, height: height}) do
%{
width: round(1 / 2 * width) - @padding * 2,
height: height - @padding * 2
}
end

defp get_desired_stream_format(3, %{width: width, height: height}) do
%{
width: round(1 / 2 * width) - @padding * 2,
height: round(1 / 2 * height) - @padding * 2
}
end

defp get_placement(desired_stream_format, 1, 0, input_stream_format, _output_stream_format) do
%BaseVideoPlacement{
position: {0, 0},
size: scale(desired_stream_format, input_stream_format),
z_value: @z_value
}
end

defp get_placement(desired_stream_format, 2, index, input_stream_format, %{width: width}) do
%BaseVideoPlacement{
position: {round(1 / 2 * width) * index + @padding, @padding},
size: scale(desired_stream_format, input_stream_format),
z_value: @z_value
}
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Whay do you think about moving these functions on the end of the file?

Comment on lines 167 to 201
defp get_cropping(desired_stream_format, scaled_stream_format),
do: %Cropping{
crop_top_left_corner: get_cropping_position(desired_stream_format, scaled_stream_format),
crop_size: get_cropping_size(desired_stream_format, scaled_stream_format),
cropped_video_position: :input_position
}

defp get_cropping_position(desired_stream_format, {width, height}) do
if desired_stream_format.width == width,
do: {0.0, (height - desired_stream_format.height) / (2 * height)},
else: {(width - desired_stream_format.width) / (2 * width), 0.0}
end

defp get_cropping_size(desired_stream_format, {width, height}) do
if desired_stream_format.width == width,
do: {1.0, desired_stream_format.height / height},
else: {desired_stream_format.width / width, 1.0}
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 do you think about creating a utils module where these functions could be moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is around 200 lines so I don't think that we have to split it into smaller parts.

Copy link
Contributor

@Rados13 Rados13 left a comment

Choose a reason for hiding this comment

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

Also, one test fails.

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Merging #291 (481bb6d) into master (7da831e) will increase coverage by 0.20%.
The diff coverage is 84.44%.

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   62.91%   63.11%   +0.20%     
==========================================
  Files          44       40       -4     
  Lines        2130     2055      -75     
==========================================
- Hits         1340     1297      -43     
+ Misses        790      758      -32     
Impacted Files Coverage Δ
lib/membrane_rtc_engine/engine.ex 77.08% <ø> (-0.42%) ⬇️
lib/membrane_rtc_engine/track.ex 88.88% <ø> (ø)
lib/membrane_rtc_engine/utils.ex 14.28% <ø> (ø)
test/support/file_endpoint.ex 96.00% <ø> (ø)
...ane_rtc_engine/endpoints/hls/compositor_handler.ex 81.08% <81.08%> (ø)
lib/membrane_rtc_engine/endpoints/hls_endpoint.ex 79.20% <100.00%> (-0.18%) ⬇️

... and 2 files with indirect coverage changes


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 7da831e...481bb6d. Read the comment docs.

@Karolk99 Karolk99 requested a review from Rados13 July 10, 2023 13:23
@Karolk99 Karolk99 force-pushed the live-compositor branch 2 times, most recently from 42abd54 to 1827c5a Compare July 10, 2023 15:53
@Karolk99 Karolk99 force-pushed the live-compositor branch 2 times, most recently from d8e4de7 to 481bb6d Compare July 11, 2023 10:15
@Karolk99 Karolk99 marked this pull request as ready for review July 11, 2023 10:35
@Karolk99 Karolk99 merged commit ce94edc into master Jul 11, 2023
@Karolk99 Karolk99 deleted the live-compositor branch July 11, 2023 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants