Add max_instances option for dynamic pads #876
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mat-hek
reviewed
Sep 30, 2024
lib/membrane/bin/pad_data.ex
Outdated
| @@ -23,6 +24,7 @@ defmodule Membrane.Bin.PadData do | |||
| availability: Membrane.Pad.availability(), | |||
| direction: Membrane.Pad.direction(), | |||
| name: Membrane.Pad.name(), | |||
| max_cardinality: Membrane.Pad.max_cardinality() | nil, | |||
mat-hek
reviewed
Sep 30, 2024
lib/membrane/pad.ex
Outdated
| @@ -102,6 +102,12 @@ defmodule Membrane.Pad do | |||
| """ | |||
| @type accepted_format :: module() | (pattern :: term()) | |||
|
|
|||
| @typedoc """ | |||
| Describes maximal number of instances of dynamic pad (`availability: :on_request`) that | |||
| can occur within single component. | |||
There was a problem hiding this comment.
Suggested change
| can occur within single component. | |
| can occur simultaneously within a single component. |
varsill
requested changes
Oct 1, 2024
lib/membrane/bin/pad_data.ex
Outdated
| @@ -8,6 +8,7 @@ defmodule Membrane.Bin.PadData do | |||
| - `:name` - see `t:Membrane.Pad.name/0`. Do not mistake with `:ref` | |||
| - `:options` - options passed in `Membrane.ChildrenSpec` when linking pad | |||
| - `:ref` - see `t:Membrane.Pad.ref/0` | |||
| - `max_cardinality` - specyfies maximal possible number of instances of a dynamic pads that can occur within single element. `nil` for pads with `availability: :always`. | |||
There was a problem hiding this comment.
Suggested change
| - `max_cardinality` - specyfies maximal possible number of instances of a dynamic pads that can occur within single element. `nil` for pads with `availability: :always`. | |
| - `max_cardinality` - specifies maximal possible number of instances of a dynamic pad that can exist within single element. Equals `nil` for pads with `availability: :always`. |
There was a problem hiding this comment.
I would apply similar changes for the option in bin and in the typedoc in Pad module
| @@ -37,6 +37,26 @@ defmodule Membrane.Core.Child.PadController do | |||
| :ok | |||
| end | |||
|
|
|||
| @spec validate_pad_cardinality!(Pad.name(), state()) :: :ok | |||
There was a problem hiding this comment.
Suggested change
| @spec validate_pad_cardinality!(Pad.name(), state()) :: :ok | |
| @spec validate_pad_cardinality!(Pad.name(), state()) :: :ok | no_return |
Comment on lines
50
to
52
| Pad #{inspect(pad_name)} can occur only #{max_cardinality} times within a single component, while it \ | ||
| attempted to occur #{current_number + 1} times. Set `:max_cardinality` option to a different value, | ||
| to change this boundary. |
There was a problem hiding this comment.
How about something like this:
Suggested change
| Pad #{inspect(pad_name)} can occur only #{max_cardinality} times within a single component, while it \ | |
| attempted to occur #{current_number + 1} times. Set `:max_cardinality` option to a different value, | |
| to change this boundary. | |
| Only #{max_cardinality} instances of pad #{inspect(pad_name)} can exist within this component, while an attempt to create #{current_number + 1} instances was made. Set `:max_cardinality` option to a different value, | |
| to change this boundary. |
?
lib/membrane/pad.ex
Outdated
| @@ -126,7 +135,8 @@ defmodule Membrane.Pad do | |||
| accepted_format: accepted_format(), | |||
| flow_control: flow_control(), | |||
| options: Keyword.t(), | |||
| demand_unit: Buffer.Metric.unit()} | |||
| demand_unit: Buffer.Metric.unit(), | |||
| max_cardinality: max_cardinality()} | |||
There was a problem hiding this comment.
It's probably not the scope of this PR, but is this element_spec a proper typespec? I think that it doesn't highlight the fact that some of these fields are mandatory, while the others are optional
There was a problem hiding this comment.
BTW. - Perhaps it would be good to write somewhere that max_cardinality defaults to :infinity for dynamic pads? I don't think there is a way to tell that without checking the code.
mat-hek
approved these changes
Oct 24, 2024
FelonEkonom
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #870