Add how_much_to_skip option to the Source element
#116
Conversation
…Add start_at option to the source element.
test/membrane_http_adaptive_stream/integration_test/source_test.exs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to start HLS stream playback from a specific time offset by introducing a start_at option to the Source element. The implementation involves updating the ex_hls dependency to support start time parameters and propagating the start time through the source element and client components.
Key changes:
- Updates ex_hls dependency to a version that supports
start_at_msparameter - Adds
start_atoption to the Source element with proper documentation and default value - Extends the client GenServer to pass start time to the underlying HLS client
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mix.exs | Updates ex_hls dependency to newer commit and temporarily switches to GitHub ref for membrane_mp4_plugin |
| lib/membrane_http_adaptive_stream/source.ex | Adds start_at option definition and passes it to ClientGenServer |
| lib/membrane_http_adaptive_stream/source/client_genserver.ex | Extends GenServer to accept and use start_at parameter when creating HLS client |
| test/membrane_http_adaptive_stream/integration_test/source_test.exs | Adds integration test for start_at functionality with reference files |
Comments suppressed due to low confidence (1)
test/membrane_http_adaptive_stream/integration_test/source_test.exs:97
- Using a fixed sleep duration in tests can lead to flaky tests. Consider using a more deterministic approach like waiting for specific pipeline events or using Testing.Pipeline.assert_end_of_stream/2 to ensure the test completes reliably.
Process.sleep(10_000)
Co-authored-by: Copilot <[email protected]>
| hls_to_file_pipeline_spec( | ||
| @mpegts_url, | ||
| %Membrane.Transcoder{ | ||
| assumed_input_stream_format: %Membrane.AAC{ | ||
| encapsulation: :ADTS | ||
| }, | ||
| output_stream_format: Membrane.AAC | ||
| }, | ||
| audio_result_file, | ||
| video_result_file, | ||
| start_at | ||
| ) |
There was a problem hiding this comment.
I forgot to fix it, but in the audio spec branch we should have AAC parser with :out_encapsulation set to :ADTS instead of transcoder with :assumed_input_stream_format in this test and in the test above
There was a problem hiding this comment.
The transcoder there remains unchanged 😢
There was a problem hiding this comment.
I've removed it right now ;)
…nitial discontinuity generation
| end | ||
|
|
||
| @spec how_much_skipped(pid()) :: Membrane.Time.t() | ||
| def how_much_skipped(client_genserver) do |
There was a problem hiding this comment.
please rename it to sth like how_much_already_skipped to distinguish it more from how_much_to_skip
There was a problem hiding this comment.
I don't think "already" fits there, but I think how_much_truly_skipped sounds OK
| state = | ||
| put_in( | ||
| state.how_much_skipped, | ||
| state.client.base_timestamp_ms |> round() |> Membrane.Time.milliseconds() |
There was a problem hiding this comment.
is base_timestamp_ms a part of public API? IMO it would be better to access it via a public function with docs and spec
There was a problem hiding this comment.
and I don't get why we set how_much_skipped to base_timestamp_ms - the name of the second variable doesn't sound like a value describing how much of a stream we have already skipped
There was a problem hiding this comment.
Yeah, it's a hacky solution (I didn't expect that I would need to send a Discontinuity event with information about how much of the stream was skipped).
how_much_skipped is set to base_timestamp_ms as the client sets base_timestamp_ms to the timestamp of the first non-discarded sample.
There was a problem hiding this comment.
As discussed, I've added a helper function in ex_hls
| defp get_discontinuity_events(%{initial_discontinuity_event_sent?: false} = state) do | ||
| how_much_skipped = ClientGenServer.how_much_skipped(state.client_genserver) | ||
|
|
||
| get_pads(state) | ||
| |> Enum.flat_map(fn pad_ref -> | ||
| [event: {pad_ref, %Membrane.Event.Discontinuity{duration: how_much_skipped}}] | ||
| end) | ||
| end | ||
|
|
||
| defp get_discontinuity_events(_state) do | ||
| [] | ||
| end |
There was a problem hiding this comment.
discontinuity events are supposed to be sent up to once on every pad, am I right?
| hls_to_file_pipeline_spec( | ||
| @mpegts_url, | ||
| %Membrane.Transcoder{ | ||
| assumed_input_stream_format: %Membrane.AAC{ | ||
| encapsulation: :ADTS | ||
| }, | ||
| output_stream_format: Membrane.AAC | ||
| }, | ||
| audio_result_file, | ||
| video_result_file, | ||
| start_at | ||
| ) |
There was a problem hiding this comment.
The transcoder there remains unchanged 😢
| defp hls_to_file_pipeline_spec( | ||
| url, | ||
| audio_transcoder, | ||
| audio_processor, |
There was a problem hiding this comment.
| audio_processor, | |
| audio_parser, |
There was a problem hiding this comment.
in one case we pass Transcoder as this "audio_processor" so I would leave it this way
varsill
changed the title
start_at option to the Source elementhow_much_to_skip option to the Source element
This PR:
ex_hlsto a version which acceptshow_much_to_skip_msas a parameterhow_much_to_skipoption to the source elementDiscontinuity event on each output pad of the Source with duration field set to the duration of skipped part of the stream (it might differ from the requestedhow_much_to_skipdue to the fact that it needs to be aligned with segments)Related PR: membraneframework/ex_hls#5
closes membraneframework/membrane_core#992