Skip to content

Conversation

@FelonEkonom
Copy link
Member

No description provided.

@FelonEkonom FelonEkonom requested a review from Copilot September 26, 2025 10:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for live HLS (HTTP Live Streaming) ingress functionality to the Boombox streaming library. The implementation includes comprehensive test coverage and updates the dependency to support HLS live streaming operations.

Key changes:

  • Added support for live HLS mode with proper handling in tests and core functionality
  • Updated dependencies to enable HLS streaming capabilities
  • Enhanced test infrastructure to handle live streaming scenarios with appropriate timeouts

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
test/boombox_test.exs Added comprehensive test cases for live and VOD HLS modes, updated endpoint handling logic, and improved test timeout configuration
mix.exs Updated HLS plugin dependency and added ex_hls package for HLS functionality
lib/boombox/internal_bin.ex Enhanced HLS input detection to support local files and removed live mode detection logic
lib/boombox.ex Simplified HLS input type definitions by removing mode options

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 211 to 214
# defp sort_endpoint_pairs(endpoint_pairs) do
# chunks = Enum.split
# end

Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

Remove commented-out dead code. This incomplete function definition serves no purpose and reduces code clarity.

Suggested change
# defp sort_endpoint_pairs(endpoint_pairs) do
# chunks = Enum.split
# end

Copilot uses AI. Check for mistakes.
@FelonEkonom FelonEkonom marked this pull request as ready for review September 26, 2025 13:23
@FelonEkonom FelonEkonom requested a review from varsill September 26, 2025 13:23
mix.exs Outdated
{:membrane_realtimer_plugin, "~> 0.9.0"},
{:membrane_http_adaptive_stream_plugin, "~> 0.19.0"},
{:membrane_http_adaptive_stream_plugin, "~> 0.20.1"},
{:ex_hls, "~> 0.1.2"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need that

Comment on lines 15 to 18
Logger.info("""
Option :mode is deprecated for HLS input. Its value will be ignored.
It was set to #{inspect(maybe_hls_mode)}.
""")
Copy link
Contributor

@varsill varsill Sep 29, 2025

Choose a reason for hiding this comment

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

Let's make it a warning

do:
{:srt, "srt://127.0.0.1:9710",
[stream_id: "some_stream_id", password: "some_password"]}
do: {:srt, "srt://127.0.0.1", [stream_id: "some_stream_id", password: "some_password"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if I like the fact that now the endpoints we use in tests definition are different than Boombox endpoints itself (i.e. in Boombox we have srt://<host>:<port> input, and in tests we use srt://<host>)

@FelonEkonom FelonEkonom requested a review from varsill September 30, 2025 12:16
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

🥇

@FelonEkonom FelonEkonom merged commit 3884f21 into master Sep 30, 2025
2 of 3 checks passed
@FelonEkonom FelonEkonom deleted the live-hls-ingress-v2 branch September 30, 2025 13:03
@FelonEkonom FelonEkonom mentioned this pull request Sep 30, 2025
@FelonEkonom FelonEkonom moved this to Done in Smackore Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants