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

Generate warnings related to pads with :auto flow control #900

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
11 changes: 11 additions & 0 deletions lib/membrane/bin.ex
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,9 @@ defmodule Membrane.Bin do
Options:
- `:bring_spec?` - if true (default) imports and aliases `Membrane.ChildrenSpec`
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""
defmacro __using__(options) do
bring_spec =
Expand All @@ -346,12 +349,20 @@ defmodule Membrane.Bin do
end
end

Module.put_attribute(
__CALLER__.module,
:__membrane_flow_control_related_warnings__,
Keyword.get(options, :flow_control_related_warnings?, true)
)

quote location: :keep do
alias unquote(__MODULE__)
@behaviour unquote(__MODULE__)
@before_compile unquote(__MODULE__)
@after_compile {Membrane.Core.Parent, :check_deprecated_callbacks}

@membrane_component_type unquote(__MODULE__)

unquote(bring_spec)
unquote(bring_pad)

Expand Down
72 changes: 72 additions & 0 deletions lib/membrane/core/child/pads_specs.ex
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ defmodule Membrane.Core.Child.PadsSpecs do
pads = Module.get_attribute(env.module, :membrane_pads, []) |> Enum.reverse()
:ok = validate_pads!(pads, env)

if Module.get_attribute(env.module, :__membrane_flow_control_related_warnings__) do
:ok = generate_flow_control_related_warnings(pads, env)
end

alias Membrane.Pad

quote do
Expand All @@ -150,6 +154,74 @@ defmodule Membrane.Core.Child.PadsSpecs do
end
end

@spec generate_flow_control_related_warnings(
pads :: [{Pad.name(), Pad.description()}],
env :: Macro.Env.t()
) :: :ok
defp generate_flow_control_related_warnings(pads, env) do
used_module =
env.module
|> Module.get_attribute(:__membrane_element_type__, :bin)
|> case do
:bin -> Membrane.Bin
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this apply to elements only?

:source -> Membrane.Source
:filter -> Membrane.Filter
:sink -> Membrane.Sink
:endpoint -> Membrane.Endpoint
end

{auto_pads, push_pads} =
pads
|> Enum.filter(fn {_pad, info} -> info[:flow_control] in [:auto, :push] end)
|> Enum.split_with(fn {_pad, info} -> info.flow_control == :auto end)

if auto_pads != [] and push_pads != [] do
IO.warn("""
#{inspect(env.module)} defines pads with `flow_control: :auto` and pads with `flow_control: :push` \
at the same time. Please, consider if this what you want to do - flow control of these pads won't be \
integrated. Setting `flow_control` to `:auto` in the places where it has been set to `:push` might be \
a good idea.
Pads with `flow_control: :auto`: #{auto_pads |> Keyword.keys() |> Enum.join(", ")}.
Pads with `flow_control: :push`: #{push_pads |> Keyword.keys() |> Enum.join(", ")}.

If you want get rid of this warning, pass `flow_control_related_warnings?: false` option to \
`use #{inspect(used_module)}`.
""")
end

{auto_input_pads, auto_output_pads} =
auto_pads
|> Enum.split_with(fn {_pad, info} -> info.direction == :input end)

[auto_input_pads_possible_number, auto_output_pads_possible_number] =
[auto_input_pads, auto_output_pads]
|> Enum.map(fn pads ->
pads
|> Enum.map(fn
{_pad, %{availability: :always}} -> 1
{_pad, %{max_instances: number}} when is_number(number) -> number
{_pad, %{max_instances: :infinity}} -> 2
end)
|> Enum.sum()
end)

if auto_input_pads_possible_number >= 2 and auto_output_pads_possible_number >= 2 do
Membrane.Logger.warning("""
#{inspect(env.module)} might have multiple input pads with `flow_control: :auto` and multiple output \
pads with `flow_control: :auto` at the same time. Notice, that lack of demand on any of output pads with \
`flow_control: :auto` will cause stoping demand on every input pad with `flow_control: :auto`.
Input pads with `flow_control: :auto`: #{auto_input_pads |> Keyword.keys() |> inspect()}.
Output pads with `flow_control: :auto`: #{auto_output_pads |> Keyword.keys() |> inspect()}.

If you want get rid of this warning, pass `flow_control_related_warnings?: false` option to \
`use #{inspect(used_module)}` or add upperbound on the number of possible instances of pads with \
`availability: :on_request` using `max_instances: non_neg_number` option.
""")
end

:ok
end

@spec parse_pad_specs!(
specs :: Pad.spec(),
direction :: Pad.direction(),
Expand Down
9 changes: 9 additions & 0 deletions lib/membrane/element/base.ex
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,9 @@ defmodule Membrane.Element.Base do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""
defmacro __using__(options) do
bring_pad =
Expand All @@ -247,6 +250,12 @@ defmodule Membrane.Element.Base do
end
end

Module.put_attribute(
__CALLER__.module,
:__membrane_flow_control_related_warnings__,
Keyword.get(options, :flow_control_related_warnings?, true)
)

quote location: :keep do
@behaviour unquote(__MODULE__)

Expand Down
3 changes: 3 additions & 0 deletions lib/membrane/endpoint.ex
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ defmodule Membrane.Endpoint do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
Copy link
Member

Choose a reason for hiding this comment

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

What about

Suggested change
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
- `:flow_control_hints?` - if true (default) generates compile-time warnings \

if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""
defmacro __using__(options) do
Module.put_attribute(__CALLER__.module, :__membrane_element_type__, :endpoint)
Expand Down
3 changes: 3 additions & 0 deletions lib/membrane/filter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ defmodule Membrane.Filter do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""

defmacro __using__(options) do
Expand Down
3 changes: 3 additions & 0 deletions lib/membrane/sink.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ defmodule Membrane.Sink do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""
defmacro __using__(options) do
Module.put_attribute(__CALLER__.module, :__membrane_element_type__, :sink)
Expand Down
3 changes: 3 additions & 0 deletions lib/membrane/source.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ defmodule Membrane.Source do

Options:
- `:bring_pad?` - if true (default) requires and aliases `Membrane.Pad`
- `:flow_control_related_warnings?` - if true (default) generates compile-time warnings \
if the number, direction, and type of flow control of pads are likely to cause unintended \
behaviours.
"""
alias Membrane.Core.DocsHelper

Expand Down
2 changes: 1 addition & 1 deletion test/support/child_removal_test/filter_to_be_removed.ex
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
defmodule Membrane.Support.ChildRemovalTest.FilterToBeRemoved do
@moduledoc false
use Membrane.Filter
use Membrane.Filter, flow_control_related_warnings?: false

def_input_pad :input, accepted_format: _any, flow_control: :auto, availability: :on_request
def_output_pad :output, accepted_format: _any, flow_control: :auto, availability: :on_request
Expand Down
2 changes: 1 addition & 1 deletion test/support/dynamic_filter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ defmodule Membrane.Support.Element.DynamicFilter do
"""

use Bunch
use Membrane.Filter
use Membrane.Filter, flow_control_related_warnings?: false

def_input_pad :input, accepted_format: _any, availability: :on_request, flow_control: :auto
def_output_pad :output, accepted_format: _any, availability: :on_request, flow_control: :auto
Expand Down