Make demand action accept timestamps & write manual timestamps guide#1079
Make demand action accept timestamps & write manual timestamps guide#1079FelonEkonom merged 53 commits intomasterfrom
Conversation
FelonEkonom
changed the title
FelonEkonom
changed the title
| [%Buffer{}] | [], | ||
| non_neg_integer | Membrane.Time.t(), | ||
| first_consumed_buffer :: Buffer.t() | nil, | ||
| last_consumed_buffer :: Buffer.t() | nil | ||
| ) :: {[%Buffer{}] | [], [%Buffer{}] | []} |
There was a problem hiding this comment.
| [%Buffer{}] | [], | |
| non_neg_integer | Membrane.Time.t(), | |
| first_consumed_buffer :: Buffer.t() | nil, | |
| last_consumed_buffer :: Buffer.t() | nil | |
| ) :: {[%Buffer{}] | [], [%Buffer{}] | []} | |
| [%Buffer{}], | |
| non_neg_integer | Membrane.Time.t(), | |
| first_consumed_buffer :: Buffer.t() | nil, | |
| last_consumed_buffer :: Buffer.t() | nil | |
| ) :: {[%Buffer{}], [%Buffer{}]} |
| [first | rest] = buffers | ||
|
|
||
| _last = | ||
| Enum.reduce(rest, first, fn |
There was a problem hiding this comment.
Iterating over all buffers multiple times may introduce some overhead. We do it here and in assert_non_nil_timestamps and split_timestamp_buffers. I'd do that once if possible.
There was a problem hiding this comment.
Is it a copy of Membrane.Core.Element.ManualFlowController.BufferMetric?
There was a problem hiding this comment.
Yeah, it was a leftover I missed
varsill
left a comment
varsill
left a comment
There was a problem hiding this comment.
[Comments concerning just the guide]
| the demanded value. | ||
|
|
||
| Timestamp demand units are only applicable to **input pads with manual flow | ||
| control**. Output pads do not support them. If an input pad uses a timestamp |
There was a problem hiding this comment.
I mean that currently it just describes the scenario when output pad's demand unit in unspecified - what if output pad's demand unit is explicitly set as :bytes (or :buffers)?
|
|
||
| The canonical pattern for a filter with both pads using manual flow control is: | ||
|
|
||
| - **[`handle_demand/5`](`c:Membrane.Element.WithOutputPads.handle_demand/5`)** — propagate the demand upstream by returning a [`:demand`](`t:Membrane.Element.Action.demand/0`) action on the input pad. |
There was a problem hiding this comment.
Let's mention that demand might be modified (just as it's multiplied by 2 in the example below)
| ### Redemand | ||
|
|
||
| Sometimes producing all demanded buffers at once is not possible or not | ||
| desired — for example when the element generates one buffer at a time. In that |
There was a problem hiding this comment.
"when the element generates one buffer at a time" - it's a little bit obscure to me, do you mean: "the previous element"?
There was a problem hiding this comment.
TBH in my mind the element generates one buffer at a time is the same as the one that receives demand, you can think about Membrane.Source scenario
There was a problem hiding this comment.
I have rephrased the sentence so maybe now it will sound better
| desired — for example when the element generates one buffer at a time. In that | ||
| case, return the [`:redemand`](`t:Membrane.Element.Action.redemand/0`) action | ||
| alongside the buffer. Membrane will then invoke | ||
| [`handle_demand/5`](`c:Membrane.Element.WithOutputPads.handle_demand/5`) again | ||
| with the demand reduced by the size of what was just sent. |
There was a problem hiding this comment.
How about this one?
I think that concerning "we shouldn't return :redemand from handle_demand in Filters" - you can add a tiny note at the end of the "## Filters with manual flow control" section
|
@varsill IDK why but for some reason I cannot reply to some of your comments
There is already
I tried to add this information to this paragrapgh with Claude, but it is hard to me to figure out how to mention that information in this place without interrupting the paragragraph flow. If you have any specific commit suggestion, you can tell me |
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Private helpers | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
I don't like the fact that we dispatch do_split_buffers to do_split_bytes and do_split_timestamp_buffers - these functions split buffers based on demand unit, so I would call them: do_split_buffers_by_bytesize and do_split_buffers_by_timestamp
| Demanding a #{timestamp_name(unit)} that is not greater than the elapsed one \ | ||
| won't result in handling any further buffers, until the element demands a #{timestamp_name(unit)} \ | ||
| greater than the elapsed one. \ |
There was a problem hiding this comment.
I think the same information is repeated twice in this sentence
| This may lead to unexpected behavior in elements that have input pad with flow \ | ||
| control set to `:manual` and demand unit set to `:timestamp`, `{:timestamp, :dts}` \ | ||
| `{:timestamp, :pts}` or `{:timestamp, :dts_or_pts}`. | ||
| Pad reference: #{inspect(pad_ref)} |
There was a problem hiding this comment.
Does it make sense to say that it might lead to unexpacted behaviour in "input pad with flow
control set to :manual and demand unit set to :timestamp, {:timestamp, :dts}
{:timestamp, :pts} or {:timestamp, :dts_or_pts}. " since we are already talking about a very specific pad with such timestamp demand unit?
| accepted_formats_str: [], | ||
| flow_control: flow_control_spec(direction, component), | ||
| flow_control: flow_control_spec_constraints(direction, component), | ||
| demand_unit: demand_unit_spec(direction, component), |
There was a problem hiding this comment.
same for others i.e. demand_unit_spec_constraints and max_instances_spec_constraints
3 participants
Closes #1072
Closes #1081