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.

Sign up for GitHub

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

Jump to bottom

SourceBin dynamic pads #95

Merged
merged 10 commits into from
Aug 22, 2024
Merged

SourceBin dynamic pads #95

merged 10 commits into from
Aug 22, 2024

Conversation

bartkrak
Copy link
Contributor

@bartkrak bartkrak commented Jul 25, 2024 •
edited
Loading

@bartkrak bartkrak requested a review from Qizot as a code owner July 25, 2024 20:14
@bartkrak bartkrak requested a review from varsill July 31, 2024 01:55
FelonEkonom
FelonEkonom requested changes Aug 12, 2024
View reviewed changes
lib/membrane_rtmp_plugin/rtmp/source/source_bin.ex Outdated
Comment on lines 45 to 133
|> child(:demuxer, Membrane.FLV.Demuxer),
child(:audio_parser, %Membrane.AAC.Parser{
out_encapsulation: :none
}),
child(:video_parser, Membrane.H264.Parser),
#
get_child(:demuxer)
|> via_out(Pad.ref(:audio, 0))
|> get_child(:audio_parser)
|> bin_output(:audio),
#
get_child(:demuxer)
|> via_out(Pad.ref(:video, 0))
|> get_child(:video_parser)
|> bin_output(:video)
|> child(:demuxer, Membrane.FLV.Demuxer)
]

{[spec: structure], %{}}
state = %{
demuxer_audio_pad_ref: nil,
demuxer_video_pad_ref: nil
}

{[spec: spec], state}
end

@impl true
def handle_pad_added(Pad.ref(:audio, _ref) = pad, _ctx, state) do
spec =
if state.demuxer_audio_pad_ref != nil do
[
get_child(:demuxer)
|> via_out(state.demuxer_audio_pad_ref)
|> child(:audio_parser, %Membrane.AAC.Parser{
out_encapsulation: :none
})
|> bin_output(pad)
]
else
[
child(:funnel_audio, Membrane.Funnel)
|> bin_output(pad)
]
end

{[spec: spec], state}
end

def handle_pad_added(Pad.ref(:video, _ref) = pad, _ctx, state) do
spec =
if state.demuxer_video_pad_ref != nil do
[
get_child(:demuxer)
|> via_out(state.demuxer_video_pad_ref)
|> child(:video_parser, Membrane.H264.Parser)
|> bin_output(pad)
]
else
[
child(:funnel_video, Membrane.Funnel)
|> bin_output(pad)
]
end

{[spec: spec], state}
end

@impl true
def handle_child_notification({:new_stream, pad_ref, :AAC}, :demuxer, ctx, state) do
audio_pad_ref = get_pad(:audio, ctx)

if audio_pad_ref != nil do
{[
spec: [
get_child(:demuxer)
|> via_out(pad_ref)
|> child(:audio_parser, %Membrane.AAC.Parser{
out_encapsulation: :none
})
|> get_child(:funnel_audio)
]
], state}
else
{[], %{state | demuxer_audio_pad_ref: pad_ref}}
end
end

def handle_child_notification({:new_stream, pad_ref, :H264}, :demuxer, ctx, state) do
video_pad_ref = get_pad(:video, ctx)

if video_pad_ref != nil do
{[
spec: [
get_child(:demuxer)
|> via_out(pad_ref)
|> child(:video_parser, Membrane.H264.Parser)
|> get_child(:funnel_video)
]
], state}
Copy link
Member

Choose a reason for hiding this comment

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

If a spec is in form of [element], it can be just element

lib/membrane_rtmp_plugin/rtmp/source/source_bin.ex Outdated
Comment on lines 61 to 97
@impl true
def handle_pad_added(Pad.ref(:audio, _ref) = pad, _ctx, state) do
spec =
if state.demuxer_audio_pad_ref != nil do
[
get_child(:demuxer)
|> via_out(state.demuxer_audio_pad_ref)
|> child(:audio_parser, %Membrane.AAC.Parser{
out_encapsulation: :none
})
|> bin_output(pad)
]
else
[
child(:funnel_audio, Membrane.Funnel)
|> bin_output(pad)
]
end

{[spec: spec], state}
end

def handle_pad_added(Pad.ref(:video, _ref) = pad, _ctx, state) do
spec =
if state.demuxer_video_pad_ref != nil do
[
get_child(:demuxer)
|> via_out(state.demuxer_video_pad_ref)
|> child(:video_parser, Membrane.H264.Parser)
|> bin_output(pad)
]
else
[
child(:funnel_video, Membrane.Funnel)
|> bin_output(pad)
]
end
Copy link
Member

Choose a reason for hiding this comment

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

Raise, if there is more than 1 audio/video pad

varsill
varsill requested changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment •
edited
Loading

Choose a reason for hiding this comment

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

General thought - I think it would be easier to understand if we would always spawn the funnels in the handle_pad_added, and then link appropriate outputs when they are ready. We could then move that "linking" to some function, like link_audio or link_video, so that not to repeat the code.
Another benefit is that we would always have the same structure of the bin, no matter what is the order in which the callbacks were invoked, which definitely would simplify debugging (for instance, if there was some bug in the funnel)

README.md Outdated
@@ -14,7 +14,7 @@ The package can be installed by adding `membrane_rtmp_plugin` to your list of de
```elixir
def deps do
[
{:membrane_rtmp_plugin, "~> 0.23.4"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure it's enough just to bump a patch, as we gonna release the changes with new RTMP server as well

@varsill varsill removed the request for review from Qizot August 12, 2024 13:29
varsill
varsill approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

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

Looks much more clear now. Perhaps you could bump version to v0.25.0 and release it after it's merged?

lib/membrane_rtmp_plugin/rtmp/source/source_bin.ex Outdated
|> Enum.count(fn pad_ref -> Pad.name_by_ref(pad_ref) == name end)

if count > 1 do
raise("Linking more than one #{name} output pad to #{__MODULE__} is not allowed")
Copy link
Contributor

Choose a reason for hiding this comment

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

[NIT] If you don't add inspect() it will print something like Elixir.Membrane.RTMP.SourceBin instead of Membrane.RTMP.SourceBin

Suggested change
raise("Linking more than one #{name} output pad to #{__MODULE__} is not allowed")
raise("Linking more than one #{name} output pad to #{inspect(__MODULE__)} is not allowed")

FelonEkonom
FelonEkonom approved these changes Aug 22, 2024
View reviewed changes
lib/membrane_rtmp_plugin/rtmp/source/source_bin.ex Outdated
|> Enum.count(fn pad_ref -> Pad.name_by_ref(pad_ref) == name end)

if count > 1 do
raise("Linking more than one #{name} output pad to #{inspect(__MODULE__)} is not allowed")
Copy link
Member

Choose a reason for hiding this comment

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

Wrap name into inspect()

@bartkrak bartkrak merged commit 3258ba2 into master Aug 22, 2024
3 checks passed
@bartkrak bartkrak deleted the source_bin_dynamic_pads branch August 22, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varsill varsill varsill approved these changes

@FelonEkonom FelonEkonom FelonEkonom approved these changes

@mat-hek mat-hek Awaiting requested review from mat-hek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bartkrak @varsill @FelonEkonom