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

Add Source Dispatcher #17

Merged
merged 44 commits into from
May 24, 2024
Merged

Add Source Dispatcher #17

merged 44 commits into from
May 24, 2024

Conversation

varsill
Copy link
Collaborator

@varsill varsill commented Apr 5, 2024

This PR:

  • adds a Membrane.Agora.Dispatcher capable of dispatching the stream output by the Membrane.Agora.Source based on the user_id field from the buffers metadata
  • adds ORB configuration to run tests on CI

@varsill varsill changed the base branch from master to fix_bug_with_conditional_compilation April 5, 2024 13:43
@varsill varsill requested a review from mat-hek April 9, 2024 09:42
Comment on lines 9 to 18
def_input_pad(:input,
flow_control: :auto,
accepted_format: _any
)

def_output_pad(:output,
flow_control: :auto,
accepted_format: _any,
availability: :on_request
)
Copy link
Member

Choose a reason for hiding this comment

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

  • auto is the default
  • add accepted format

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

if user_id in Map.keys(state.output_pads) do
{[], state}
else
add_pad(state, user_id)
Copy link
Member

Choose a reason for hiding this comment

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

why do we send the add_pad notification from handle_pad_added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've fixed that

@@ -0,0 +1,91 @@
defmodule Membrane.Agora.Dispatcher do
@moduledoc false
Copy link
Member

Choose a reason for hiding this comment

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

This should be either used somewhere or documented for public use ;) Also, tests are missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added the documentation and the tests, but as you know there are problems with running these integration tests :/

@varsill varsill changed the base branch from fix_bug_with_conditional_compilation to master May 17, 2024 10:57
@varsill varsill requested a review from mat-hek May 24, 2024 15:09
@varsill varsill merged commit 5eb91d8 into master May 24, 2024
3 checks passed
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.

2 participants