Make demand action accept timestamps & write manual timestamps guide#1079
Make demand action accept timestamps & write manual timestamps guide#1079FelonEkonom wants to merge 28 commits intomasterfrom
Conversation
FelonEkonom
changed the title
FelonEkonom
changed the title
FelonEkonom
changed the title
varsill
left a comment
varsill
left a comment
There was a problem hiding this comment.
Just a guide review, I will come back with code review later
| element whose output pad with manual flow control is connected to that | ||
| downstream. The callback receives the pad name, the demanded amount, and the | ||
| demand unit. The element is expected to produce and send that amount of data, | ||
| or less if the stream ends. |
There was a problem hiding this comment.
Add some note that of course the element does not need to fulfill the whole demand all at once, in a single callback's invocation
| flow control — the output pad inherits `:buffers`. | ||
|
|
||
| So the output pad can explicitly control the unit it receives demand in, but | ||
| timestamp units are not available on output pads. |
There was a problem hiding this comment.
I don't understand this "timestamp units are not available on output pads." part 🤔
| 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.
Let's rephrase it slightly so that to emphasize that the output pad connected to an input pad with timestamps demand unit receives demand in :bytes or :buffers, as specified by output pad's demand unit (defaulting to :buffers)
| end | ||
| ``` | ||
|
|
||
| > #### Do not use redemand in a filter's `handle_demand` {: .warning} |
There was a problem hiding this comment.
Something is wrong with this {: .warning}
| on `:input`, so each demand naturally covers the next one-second slice of the | ||
| stream. | ||
|
|
||
| ## Filters with manual flow control |
There was a problem hiding this comment.
I think using :redemand in filters in also quite "canonical" and it's less intuitive than using it in sources - how about providing an example for it as well?
lib/membrane/buffer/metric.ex
Outdated
| For count/byte metrics, subtracts `consumed_size` from `demand`. | ||
| For timestamp metrics, the demand is a duration that does not change as buffers are consumed — |
There was a problem hiding this comment.
[NIT] I am not sure which should describe how do behaviour implementations work in the behaviour's callback definition
| defp delay_supplying_demand(pad_ref, 0, state) do | ||
| Membrane.Logger.debug_verbose("Ignoring demand of size of 0 on pad #{inspect(pad_ref)}") | ||
| state | ||
| end | ||
|
|
There was a problem hiding this comment.
Because demand 0 is not ignored when unit is timestamp
| def decrease_demand_by_outgoing_buffers(pad_ref, buffers, state) do | ||
| pad_data = PadModel.get_data!(state, pad_ref) | ||
| buffers_size = Buffer.Metric.from_unit(pad_data.demand_unit).buffers_size(buffers) | ||
| {:ok, buffers_size} = Buffer.Metric.from_unit(pad_data.demand_unit).buffers_size(buffers) |
There was a problem hiding this comment.
It will cause a nasty match error when buffers_size returns {:error, ...} - shouldn't we propagate the error further or explicitly raise with a meaningfull error message?
|
|
||
| When the pad's `demand_unit` is `:timestamp`, `{:timestamp, :pts}`, | ||
| `{:timestamp, :dts}`, or `{:timestamp, :dts_or_pts}`, the demand size is a | ||
| `t:Membrane.Time.t/0` duration (in nanoseconds). The queue will deliver |
There was a problem hiding this comment.
| `t:Membrane.Time.t/0` duration (in nanoseconds). The queue will deliver | |
| `t:Membrane.Time.t/0` duration (expressed internally in nanoseconds). The queue will deliver |
There was a problem hiding this comment.
I will remove this nanoseconds mention
There was a problem hiding this comment.
As discussed, please put the metrics into separate modules.
|
|
||
| defp maybe_assert_timestamps_present!(buffers, outbound_metric, pad_ref) do | ||
| Enum.each(buffers, fn buffer -> | ||
| if outbound_metric.nil_timestamp?(buffer) do |
There was a problem hiding this comment.
I think it would be better to make get_timestamp() a public callback and use it here to compare it with nil - this way we could get rid of nil_timestamp?() callback
There was a problem hiding this comment.
As discussed - let's:
- add
is_timestamp_metricguard and use it in InputQueue - unify
generate_metric_specific_warning()andnil_timestamp?assertion into a single callback inTimestampMetricbehaviour
Co-authored-by: Łukasz Kita <lukasz.kita0@gmail.com>
2 participants
Closes #1072
Closes #1081