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

Bump Ratio to 3.0 #438

Merged
merged 7 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 9 additions & 15 deletions lib/membrane/clock.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ defmodule Membrane.Clock do
the time passed, in practice the described approach turns out to be more convenient,
as it simplifies the first update.

Basing on updates, Clock calculates the `t:ratio_t/0` of its time to the reference
Basing on updates, Clock calculates the `t:Ratio.t/0` of its time to the reference
time. The reference time can be configured with `:time_provider` option. The ratio
is broadcasted (see `t:ratio_message_t/0`) to _subscribers_ (see `subscribe/2`)
- processes willing to synchronize to the custom clock. Subscribers can adjust
Expand All @@ -34,11 +34,6 @@ defmodule Membrane.Clock do

@type t :: pid

@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t() | non_neg_integer

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this type

Suggested change
@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t() | non_neg_integer
@typedoc """
Ratio of the Clock time to the reference time.
"""
@type ratio_t :: Ratio.t()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@typedoc """
Update message received by the Clock. It should contain the time till the next
update.
Expand All @@ -48,13 +43,13 @@ defmodule Membrane.Clock do
milliseconds ::
non_neg_integer
| Ratio.t()
| {numerator :: non_neg_integer, denominator :: pos_integer}}
| {numerator :: non_neg_integer, denominator :: pos_integer()}}

@typedoc """
Ratio message sent by the Clock to all its subscribers. It contains the ratio
of the custom clock time to the reference time.
"""
@type ratio_message_t :: {:membrane_clock_ratio, clock :: pid, ratio_t}
@type ratio_message_t :: {:membrane_clock_ratio, clock :: pid, Ratio.t()}

@typedoc """
Options accepted by `start_link/2` and `start/2` functions.
Expand Down Expand Up @@ -200,7 +195,7 @@ defmodule Membrane.Clock do
defp get_proxy_options(true, _proxy_for), do: %{proxy: true, proxy_for: nil}

defp get_proxy_options(_proxy, _proxy_for),
do: %{init_time: nil, clock_time: 0, till_next: nil, proxy: false}
do: %{init_time: nil, clock_time: Ratio.new(0), till_next: nil, proxy: false}

defp handle_unsubscribe(pid, state) do
Process.demonitor(state.subscribers[pid].monitor, [:flush])
Expand All @@ -212,13 +207,13 @@ defmodule Membrane.Clock do
end

defp handle_clock_update(till_next, state) do
use Ratio
till_next = Ratio.new(till_next)

if till_next < 0 do
if Ratio.lt?(till_next, 0) do
raise "Clock update time cannot be negative, received: #{inspect(till_next)}"
end

till_next = till_next * Time.millisecond()
till_next = Ratio.mult(till_next, Ratio.new(Time.millisecond()))

case state.init_time do
nil -> %{state | init_time: state.time_provider.(), till_next: till_next}
Expand All @@ -227,10 +222,9 @@ defmodule Membrane.Clock do
end

defp do_handle_clock_update(till_next, state) do
use Ratio
%{till_next: from_previous, clock_time: clock_time} = state
clock_time = clock_time + from_previous
ratio = clock_time / (state.time_provider.() - state.init_time)
clock_time = Ratio.add(clock_time, from_previous)
ratio = Ratio.new(clock_time, state.time_provider.() - state.init_time)
Copy link
Member

Choose a reason for hiding this comment

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

div is less confusing in this case IMO

Suggested change
ratio = Ratio.new(clock_time, state.time_provider.() - state.init_time)
ratio = Ratio.div(clock_time, state.time_provider.() - state.init_time)

Copy link
Contributor Author

@varsill varsill Nov 29, 2022

Choose a reason for hiding this comment

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

done
The reason why Ratio.new was used there previously is because Ratio.new accepts both integers and Ratios as arguments, while Ratio.div enforces usage of Ratio - I need to convert state.time_provider.() - state.init_time into a Ratio by using Ratio.new anyway :D

state = %{state | clock_time: clock_time, till_next: till_next}
broadcast_and_update_ratio(ratio, state)
end
Expand Down
12 changes: 7 additions & 5 deletions lib/membrane/core/timer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ defmodule Membrane.Core.Timer do
init_time: Time.t(),
clock: Clock.t(),
next_tick_time: Time.t(),
ratio: Clock.ratio_t(),
ratio: Ratio.t(),
timer_ref: reference() | nil
}

@enforce_keys [:interval, :clock, :init_time, :id]
defstruct @enforce_keys ++ [next_tick_time: 0, ratio: 1, timer_ref: nil]
defstruct @enforce_keys ++ [next_tick_time: 0, ratio: Ratio.new(1), timer_ref: nil]

@spec start(id_t, interval_t, Clock.t()) :: t
def start(id, interval, clock) do
Expand All @@ -37,7 +37,7 @@ defmodule Membrane.Core.Timer do
:ok
end

@spec update_ratio(t, Clock.ratio_t()) :: t
@spec update_ratio(t, Ratio.t()) :: t
def update_ratio(timer, ratio) do
%__MODULE__{timer | ratio: ratio}
end
Expand All @@ -48,7 +48,7 @@ defmodule Membrane.Core.Timer do
end

def tick(timer) do
use Ratio
use Numbers, overload_operators: true
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 be consistent and not use that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


%__MODULE__{
id: id,
Expand All @@ -62,7 +62,9 @@ defmodule Membrane.Core.Timer do

# Next tick time converted to BEAM clock time
beam_next_tick_time =
(init_time + next_tick_time / ratio) |> Ratio.floor() |> Time.round_to_milliseconds()
(init_time + Ratio.new(next_tick_time, ratio))
|> Ratio.floor()
|> Time.round_to_milliseconds()

timer_ref =
Process.send_after(self(), Message.new(:timer_tick, id), beam_next_tick_time, abs: true)
Expand Down
2 changes: 1 addition & 1 deletion lib/membrane/core/timer_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ defmodule Membrane.Core.TimerController do
state
end

@spec handle_clock_update(Timer.id_t(), Clock.ratio_t(), Component.state_t()) ::
@spec handle_clock_update(Timer.id_t(), Ratio.t(), Component.state_t()) ::
Component.state_t()
def handle_clock_update(clock, ratio, state) do
update_in(
Expand Down
23 changes: 7 additions & 16 deletions lib/membrane/time.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ defmodule Membrane.Time do
that do not touch hardware clock, you should use Membrane units for consistency.
"""

require Ratio

@compile {:inline,
native_units: 1, native_unit: 0, nanoseconds: 1, nanosecond: 0, second: 0, seconds: 1}

Expand All @@ -30,7 +32,7 @@ defmodule Membrane.Time do
# Difference between 01.01.1900 (start of NTP epoch) and 01.01.1970 (start of Unix epoch) in seconds
@ntp_unix_epoch_diff 2_208_988_800

@two_to_pow_32 Ratio.pow(2, 32)
@two_to_pow_32 Ratio.pow(Ratio.new(2, 1), 32) |> Ratio.trunc()

@doc """
Checks whether a value is `Membrane.Time.t`.
Expand Down Expand Up @@ -260,12 +262,10 @@ defmodule Membrane.Time do
end

# credo:disable-for-next-line Credo.Check.Readability.Specs
def unquote(unit.plural)(number) do
if not Ratio.is_rational?(number) do
raise "Only integers and rationals can be converted with Membrane.Time.#{unquote(unit.plural)}"
end
def unquote(unit.plural)(number) when Ratio.is_rational(number) do
use Numbers, overload_operators: true
Copy link
Member

Choose a reason for hiding this comment

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

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here and also in clock_test.exs


Ratio.*(number, unquote(unit.duration))
(number * unquote(unit.duration))
|> round_rational()
end

Expand All @@ -288,7 +288,7 @@ defmodule Membrane.Time do
@spec unquote(as_fun_name)(t) :: integer | Ratio.t()
# credo:disable-for-next-line Credo.Check.Readability.Specs
def unquote(as_fun_name)(time) when is_time(time) do
Ratio./(time, unquote(unit.duration))
Ratio.new(time, unquote(unit.duration))
end
end)

Expand All @@ -298,7 +298,6 @@ defmodule Membrane.Time do
end

defp round_rational(ratio) do
ratio = make_rational(ratio)
trunced = Ratio.trunc(ratio)

if 2 * sign_of_rational(ratio) *
Expand All @@ -308,14 +307,6 @@ defmodule Membrane.Time do
else: trunced
end

defp make_rational(number) do
if Ratio.is_rational?(number) do
number
else
%Ratio{numerator: number, denominator: 1}
end
end

defp sign_of_rational(ratio) do
if ratio.numerator == 0, do: 0, else: Ratio.sign(ratio)
end
Expand Down
2 changes: 1 addition & 1 deletion mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ defmodule Membrane.Mixfile do
{:qex, "~> 0.3"},
{:telemetry, "~> 1.0"},
{:bunch, "~> 1.5"},
{:ratio, "~> 2.0"},
{:ratio, "~> 3.0"},
# Development
{:ex_doc, "~> 0.28", only: :dev, runtime: false},
{:makeup_diff, "~> 0.1", only: :dev, runtime: false},
Expand Down
4 changes: 2 additions & 2 deletions mix.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"dialyxir": {:hex, :dialyxir, "1.2.0", "58344b3e87c2e7095304c81a9ae65cb68b613e28340690dfe1a5597fd08dec37", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "61072136427a851674cab81762be4dbeae7679f85b1272b6d25c3a839aff8463"},
"earmark_parser": {:hex, :earmark_parser, "1.4.29", "149d50dcb3a93d9f3d6f3ecf18c918fb5a2d3c001b5d3305c926cddfbd33355b", [:mix], [], "hexpm", "4902af1b3eb139016aed210888748db8070b8125c2342ce3dcae4f38dcc63503"},
"erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"},
"ex_doc": {:hex, :ex_doc, "0.29.0", "4a1cb903ce746aceef9c1f9ae8a6c12b742a5461e6959b9d3b24d813ffbea146", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "f096adb8bbca677d35d278223361c7792d496b3fc0d0224c9d4bc2f651af5db1"},
"ex_doc": {:hex, :ex_doc, "0.29.1", "b1c652fa5f92ee9cf15c75271168027f92039b3877094290a75abcaac82a9f77", [:mix], [{:earmark_parser, "~> 1.4.19", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "b7745fa6374a36daf484e2a2012274950e084815b936b1319aeebcf7809574f6"},
"excoveralls": {:hex, :excoveralls, "0.15.0", "ac941bf85f9f201a9626cc42b2232b251ad8738da993cf406a4290cacf562ea4", [:mix], [{:hackney, "~> 1.16", [hex: :hackney, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "9631912006b27eca30a2f3c93562bc7ae15980afb014ceb8147dc5cdd8f376f1"},
"file_system": {:hex, :file_system, "0.2.10", "fb082005a9cd1711c05b5248710f8826b02d7d1784e7c3451f9c1231d4fc162d", [:mix], [], "hexpm", "41195edbfb562a593726eda3b3e8b103a309b733ad25f3d642ba49696bf715dc"},
"hackney": {:hex, :hackney, "1.18.1", "f48bf88f521f2a229fc7bae88cf4f85adc9cd9bcf23b5dc8eb6a1788c662c4f6", [:rebar3], [{:certifi, "~>2.9.0", [hex: :certifi, repo: "hexpm", optional: false]}, {:idna, "~>6.1.0", [hex: :idna, repo: "hexpm", optional: false]}, {:metrics, "~>1.0.0", [hex: :metrics, repo: "hexpm", optional: false]}, {:mimerl, "~>1.1", [hex: :mimerl, repo: "hexpm", optional: false]}, {:parse_trans, "3.3.1", [hex: :parse_trans, repo: "hexpm", optional: false]}, {:ssl_verify_fun, "~>1.1.0", [hex: :ssl_verify_fun, repo: "hexpm", optional: false]}, {:unicode_util_compat, "~>0.7.0", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "a4ecdaff44297e9b5894ae499e9a070ea1888c84afdd1fd9b7b2bc384950128e"},
Expand All @@ -25,7 +25,7 @@
"numbers": {:hex, :numbers, "5.2.4", "f123d5bb7f6acc366f8f445e10a32bd403c8469bdbce8ce049e1f0972b607080", [:mix], [{:coerce, "~> 1.0", [hex: :coerce, repo: "hexpm", optional: false]}, {:decimal, "~> 1.9 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "eeccf5c61d5f4922198395bf87a465b6f980b8b862dd22d28198c5e6fab38582"},
"parse_trans": {:hex, :parse_trans, "3.3.1", "16328ab840cc09919bd10dab29e431da3af9e9e7e7e6f0089dd5a2d2820011d8", [:rebar3], [], "hexpm", "07cd9577885f56362d414e8c4c4e6bdf10d43a8767abb92d24cbe8b24c54888b"},
"qex": {:hex, :qex, "0.5.1", "0d82c0f008551d24fffb99d97f8299afcb8ea9cf99582b770bd004ed5af63fd6", [:mix], [], "hexpm", "935a39fdaf2445834b95951456559e9dc2063d0a055742c558a99987b38d6bab"},
"ratio": {:hex, :ratio, "2.4.2", "c8518f3536d49b1b00d88dd20d49f8b11abb7819638093314a6348139f14f9f9", [:mix], [{:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}, {:numbers, "~> 5.2.0", [hex: :numbers, repo: "hexpm", optional: false]}], "hexpm", "441ef6f73172a3503de65ccf1769030997b0d533b1039422f1e5e0e0b4cbf89e"},
"ratio": {:hex, :ratio, "3.0.2", "60a5976872a4dc3d873ecc57eed1738589e99d1094834b9c935b118231297cfb", [:mix], [{:decimal, "~> 1.6 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}, {:numbers, "~> 5.2.0", [hex: :numbers, repo: "hexpm", optional: false]}], "hexpm", "3a13ed5a30ad0bfd7e4a86bf86d93d2b5a06f5904417d38d3f3ea6406cdfc7bb"},
"ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.6", "cf344f5692c82d2cd7554f5ec8fd961548d4fd09e7d22f5b62482e5aeaebd4b0", [:make, :mix, :rebar3], [], "hexpm", "bdb0d2471f453c88ff3908e7686f86f9be327d065cc1ec16fa4540197ea04680"},
"telemetry": {:hex, :telemetry, "1.1.0", "a589817034a27eab11144ad24d5c0f9fab1f58173274b1e9bae7074af9cbee51", [:rebar3], [], "hexpm", "b727b2a1f75614774cff2d7565b64d0dfa5bd52ba517f16543e6fc7efcc0df48"},
"unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"},
Expand Down
8 changes: 4 additions & 4 deletions test/membrane/clock_test.exs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
defmodule Membrane.ClockTest do
use ExUnit.Case
use Numbers, overload_operators: true

@module Membrane.Clock

Expand All @@ -16,16 +17,15 @@ defmodule Membrane.ClockTest do
refute_receive {:membrane_clock_ratio, ^clock, _ratio}
send(clock, {:membrane_clock_update, 2})
send(clock, time: 13)
assert_receive {:membrane_clock_ratio, ^clock, 2}
two = Ratio.new(2, 1)
assert_receive {:membrane_clock_ratio, ^clock, ^two}
send(clock, {:membrane_clock_update, random_time()})
send(clock, time: 33)
ratio = Ratio.new(20 + 2, 33 - 3)
assert_receive {:membrane_clock_ratio, ^clock, ^ratio}
end

test "should handle different ratio formats" do
use Ratio

{:ok, clock} =
@module.start_link(time_provider: fn -> receive do: (time: t -> ms_to_ns(t)) end)

Expand All @@ -38,7 +38,7 @@ defmodule Membrane.ClockTest do
send(clock, {:membrane_clock_update, 5})
send(clock, time: 20)
@module.subscribe(clock)
ratio = (5 + Ratio.new(1, 3) * 2) / (20 - 5)
ratio = Ratio.div(5 + Ratio.new(1, 3) * 2, Ratio.new(20 - 5))
assert_receive {:membrane_clock_ratio, ^clock, ^ratio}
end

Expand Down