Extract ex_moq - #7
Conversation
fa962f8 to
375add5
Compare
varsill
left a comment
varsill
left a comment
There was a problem hiding this comment.
[NIT] Remember to remove Rust's target/ from .gitignore ;)
| {:membrane_aac_format, "~> 0.8.0"}, | ||
| {:membrane_opus_format, "~> 0.3.0"}, | ||
| # TODO: change to released version once it's public | ||
| {:ex_moq, github: "membraneframework/ex_moq", branch: "kidq330/group_loss_fixed"}, |
There was a problem hiding this comment.
Hmm shouldn't it point now to kidq330/initial?
There was a problem hiding this comment.
yup that's a leftover from debugging CI
| Membrane.Logger.debug("MoQ subscriber closed while connecting: #{inspect(reason)}") | ||
|
|
||
| {[setup: :complete, notify_parent: {:disconnected, reason}], | ||
| %{state | status: :disconnect_pending}} |
There was a problem hiding this comment.
I think you need to update moduledoc and describe that :disconnected notification is also sent when MoQ subscriber closes while connecting
There was a problem hiding this comment.
Currently it is documented like this:
* `{:disconnected, reason :: String.t() | ExMoQ.Native.close_reason()}`
when the broadcast goes away or the session drops.
The source sends `:end_of_stream` to all active pads.
I added this clause because there's a window between :moq_connected (session with relay established, otherwise the source crashes due to invalid url etc) and :moq_broadcast_ready where the session can disconnect, sending :moq_disconnected. status: :connecting can be misleading but it just means the element is still trying to connect to the particular broadcast.
The :disconnected notif is always sent after setup finishes, so I don't see a reason for the moduledoc to explicitly differentiate if the disconnect happened before or after :moq_broadcast_ready, it shouldn't matter to the parent IMO.
| {:ex_moq, github: "membraneframework/ex_moq", branch: "kidq330/group_loss_fixed"}, | ||
| {:muontrap, ">= 0.0.0", only: :test}, | ||
| {:membrane_aac_plugin, "~> 0.19", only: :test}, | ||
| {:muontrap, "~> 1.8", only: :test}, |
| @spec update(t(), [{Native.track(), Native.track_format()}]) :: {diff(), t()} | ||
| def update(catalog, renditions) do | ||
| new = Map.new(renditions) | ||
| @spec update(t(), %{Native.track() => Native.track_format()}) :: {diff(), t()} |
There was a problem hiding this comment.
Hmm is it always Native.track_format()? Cannot it be :unrecognized?
There was a problem hiding this comment.
Yes, I guess we could add a type alias for this map to ex_moq but it's not used anywhere, only as a message contract
There was a problem hiding this comment.
Shouldn't we configure it to install moq relay binary here just as you did in https://github.com/membraneframework/ex_moq/blob/f51d9d10af953f6703af34b01ad67b6acdd7630c/.github/workflows/ci.yml
?
There was a problem hiding this comment.
We should once we enable integration tests on CI, right now it would be dead weight, and I'm leaving it for a follow-up PR as discussed in DMs
2 participants
No description provided.