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 Debug.Filter and Debug.Sink #552

Merged
merged 10 commits into from
May 15, 2023
Merged

Implement Debug.Filter and Debug.Sink #552

merged 10 commits into from
May 15, 2023

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom requested a review from mat-hek as a code owner May 8, 2023 15:26
Comment on lines 5 to 8
Accepts 3 options:
- `:handle_buffer` - function with arity 1, that maps buffers handled by this filter. Defaults to `&(&1)`.
- `:handle_event` - function with arity 1, that maps events handled by this filter. Defaults to `&(&1)`.
- `:handle_stream_format` - function with arity 1, that maps stream formats handled by this filter. Defaults to `&(&1)`.
Copy link
Member

Choose a reason for hiding this comment

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

Let's move that to the options' descriptions. Defaults are automatically put there (generate the docs and verify)

Comment on lines 33 to 34
def_options handle_buffer: [spec: (Buffer.t() -> Buffer.t()), default: &__MODULE__.identity/1],
handle_event: [spec: (Event.t() -> Event.t()), default: &__MODULE__.identity/1],
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
def_options handle_buffer: [spec: (Buffer.t() -> Buffer.t()), default: &__MODULE__.identity/1],
handle_event: [spec: (Event.t() -> Event.t()), default: &__MODULE__.identity/1],
def_options handle_buffer: [spec: (Buffer.t() -> Buffer.t() | [Buffer.t()]), default: &__MODULE__.identity/1],
handle_event: [spec: (Event.t() -> Event.t() | [Event.t()]), default: &__MODULE__.identity/1],

This should already work and may be useful

Copy link
Member

@mat-hek mat-hek May 10, 2023

Choose a reason for hiding this comment

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

BTW, you can use &Function.identity/1

@@ -0,0 +1,68 @@
defmodule Membrane.Simple.Filter do
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
defmodule Membrane.Simple.Filter do
defmodule Membrane.SimpleFilter do

@@ -0,0 +1,60 @@
defmodule Membrane.Simple.Sink do
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
defmodule Membrane.Simple.Sink do
defmodule Membrane.SimpleSink do

Comment on lines 28 to 36
@spec identity(any()) :: any()
def identity(arg), do: arg

def_options handle_buffer: [spec: (Buffer.t() -> any()), default: &__MODULE__.identity/1],
handle_event: [spec: (Event.t() -> any()), default: &__MODULE__.identity/1],
handle_stream_format: [
spec: (StreamFormat.t() -> any()),
default: &__MODULE__.identity/1
]
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
@spec identity(any()) :: any()
def identity(arg), do: arg
def_options handle_buffer: [spec: (Buffer.t() -> any()), default: &__MODULE__.identity/1],
handle_event: [spec: (Event.t() -> any()), default: &__MODULE__.identity/1],
handle_stream_format: [
spec: (StreamFormat.t() -> any()),
default: &__MODULE__.identity/1
]
@spec noop(any()) :: any()
def noop(_arg), do: nil
def_options handle_buffer: [spec: (Buffer.t() -> any()), default: &__MODULE__.noop/1],
handle_event: [spec: (Event.t() -> any()), default: &__MODULE__.noop/1],
handle_stream_format: [
spec: (StreamFormat.t() -> any()),
default: &__MODULE__.noop/1
]

Comment on lines 5 to 8
Accepts 3 options:
- `:handle_buffer` - function with arity 1, that will be called with all buffers handled by this sink
- `:handle_event` - function with arity 1, that will be called with all events handled by this sink
- `:handle_stream_format` - function with arity 1, that will be called with all stream formats handled by this sink
Copy link
Member

Choose a reason for hiding this comment

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

like in the filter, and mention that the returned values are ignored

Comment on lines 11 to 34
defmodule HelperSource do
use Membrane.Source

def_output_pad :output, flow_control: :push, accepted_format: _any

defmodule StreamFormat do
defstruct []
end

@impl true
def handle_playing(_ctx, state) do
{[stream_format: {:output, %StreamFormat{}}], state}
end

@impl true
def handle_parent_notification({:send_buffers, number}, _ctx, state) do
buffers =
Enum.map(1..number, fn i ->
%Buffer{payload: inspect(i)}
end)

{[buffer: {:output, buffers}], state}
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Does this do anything beyond the testing source?

Copy link
Member Author

Choose a reason for hiding this comment

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

This source works in push, while Testing Source works in pull

Copy link
Member

Choose a reason for hiding this comment

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

does it matter in this use case?

Copy link
Member Author

Choose a reason for hiding this comment

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

While LambaSink work in auto pull, number of produced and consumed buffers would be tremendous, if we used Testing.Sink

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't be more than you provide via the output option

@FelonEkonom FelonEkonom changed the title Implement Simple.Filter and Simple.Sink Implement Lambda.Filter and Lambda.Sink May 10, 2023
@FelonEkonom FelonEkonom requested a review from mat-hek May 10, 2023 14:13
@@ -1,16 +1,11 @@
defmodule Membrane.Simple.Filter do
defmodule Membrane.LambdaFilter do
Copy link
Member

Choose a reason for hiding this comment

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

❤️

default: &Function.identity/1,
description: """
Function with arity 1, that maps events handled by this filter.
Result of this function is passed to the `:event` action on the opposite pad to
Copy link
Member

Choose a reason for hiding this comment

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

hmm, this way we cannot know whether the event is going downstream or upstream, which is pretty important

@FelonEkonom FelonEkonom changed the title Implement Lambda.Filter and Lambda.Sink Implement Debug.Filter and Debug.Sink May 12, 2023
@FelonEkonom FelonEkonom requested a review from mat-hek May 12, 2023 09:46
Comment on lines 70 to 74
opposite_pad =
case pad do
:input -> :output
:output -> :input
end
Copy link
Member

Choose a reason for hiding this comment

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

you can use the forward action instead

@@ -0,0 +1,84 @@
defmodule Membrane.Debug.Filter do
@moduledoc """
Membrane Filter, that can be used to create a child that will be used to debug data flowing thouth pipeline.
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 Filter, that can be used to create a child that will be used to debug data flowing thouth pipeline.
Membrane Filter, that can be used to create a child that will be used to debug data flowing thouth pipeline.

@FelonEkonom FelonEkonom merged commit fade9a0 into master May 15, 2023
@FelonEkonom FelonEkonom deleted the simple-elements branch May 15, 2023 12:38
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