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

Implement soft corcking #586

Merged
merged 13 commits into from
Aug 25, 2023
Merged

Implement soft corcking #586

merged 13 commits into from
Aug 25, 2023

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom force-pushed the soft-corking branch 3 times, most recently from e4614a5 to d763bb5 Compare July 24, 2023 15:24
@FelonEkonom FelonEkonom marked this pull request as ready for review July 25, 2023 15:25
@FelonEkonom FelonEkonom requested a review from varsill July 25, 2023 15:25
lib/membrane/element/pad_data.ex Outdated Show resolved Hide resolved
lib/membrane/element/action.ex Show resolved Hide resolved
test/membrane/integration/demands_test.exs Outdated Show resolved Hide resolved
test/membrane/integration/demands_test.exs Outdated Show resolved Hide resolved
@FelonEkonom FelonEkonom requested a review from varsill July 27, 2023 15:09
test/membrane/integration/demands_test.exs Show resolved Hide resolved
lib/membrane/core/element/action_handler.ex Outdated Show resolved Hide resolved
lib/membrane/core/element/action_handler.ex Outdated Show resolved Hide resolved
test/membrane/integration/demands_test.exs Show resolved Hide resolved
lib/membrane/element/pad_data.ex Outdated Show resolved Hide resolved
lib/membrane/element/action.ex Outdated Show resolved Hide resolved
lib/membrane/element/action.ex Outdated Show resolved Hide resolved
lib/membrane/element/action.ex Outdated Show resolved Hide resolved
@FelonEkonom FelonEkonom changed the base branch from master to add_children_groups July 28, 2023 14:43
@FelonEkonom FelonEkonom changed the base branch from add_children_groups to master July 28, 2023 14:43
Process.sleep(100 * RedemandingSource.sleep_time())

Testing.Pipeline.execute_actions(pipeline, notify_child: {:sink, :pause_auto_demand})

assert_pipeline_notified(pipeline, :sink, {:buff_no, buff_no})
# sink should receive aropund 100 buffers, but the boundary is set to 70, in case of eg.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# sink should receive aropund 100 buffers, but the boundary is set to 70, in case of eg.
# sink should receive around 100 buffers, but the boundary is set to 70, in case of eg.

def handle_end_of_stream(_pad, ctx, state) do
actions =
Map.values(ctx.pads)
|> Enum.filter(&(&1.direction == :output and not &1.end_of_stream?))
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, there is only one pad with direction:output, something might be wrong with that action generation.

Comment on lines 254 to 257
pad_upperbound = Map.get(state.pads_upperbounds, pad, :infinity)

actions =
if pad_counter > pad_upperbound and not ctx.pads[pad].auto_demand_paused? do
Copy link
Contributor

@varsill varsill Aug 24, 2023

Choose a reason for hiding this comment

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

that pad_counter > pad_upperbound comparison is quite tricky (since we are using :infinity atom) :D

Copy link
Member

@mat-hek mat-hek left a comment

Choose a reason for hiding this comment

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

Looks great!

if old_value == paused? do
operation = if paused?, do: "pause", else: "resume"

Membrane.Logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Membrane.Logger.debug(
Membrane.Logger.debug_verbose(

Comment on lines +268 to +272
if ctx.pads.output.end_of_stream? do
{[], state}
else
{[end_of_stream: :output], 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.

If it's ok that the eos will be send on funnel's output right after first eos from input is received, we can leave it that way

@FelonEkonom FelonEkonom merged commit eb8e8ef into master Aug 25, 2023
1 check passed
@FelonEkonom FelonEkonom deleted the soft-corking branch August 25, 2023 12:29
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.

3 participants