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

Refactor demand mechanism #783

Merged
merged 19 commits into from
Apr 3, 2024
Merged

Refactor demand mechanism #783

merged 19 commits into from
Apr 3, 2024

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom commented Mar 26, 2024

Closes #706

@FelonEkonom FelonEkonom requested a review from mat-hek as a code owner March 26, 2024 12:54
@FelonEkonom FelonEkonom mentioned this pull request Mar 26, 2024
@FelonEkonom FelonEkonom force-pushed the refactor-demand-mechanism branch from 58e7f3c to 0f7b0ca Compare March 26, 2024 13:41
@FelonEkonom FelonEkonom force-pushed the refactor-demand-mechanism branch from 93bdfb4 to 9c309b0 Compare March 26, 2024 16:34
@FelonEkonom FelonEkonom added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Mar 27, 2024
Comment on lines 48 to 53
# Match in the function below is caused by a fact, that handle_spec_started is the only callback, that
# might be executed in between handling actions returned from other callbacks.
# This callback has been deprecated and should be removed in v2.0.0, along with the if statement below.

@impl CallbackHandler
def handle_end_of_actions(state) do
def handle_end_of_actions(:handle_spec_started, state), do: state
Copy link
Member

Choose a reason for hiding this comment

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

element doesn't have this callback

@spec handle_buffer(Pad.ref(), [Buffer.t()] | Buffer.t(), State.t()) :: State.t()
def handle_buffer(pad_ref, buffers, state) do
@spec handle_ingoing_buffers(Pad.ref(), [Buffer.t()] | Buffer.t(), State.t()) :: State.t()
def handle_ingoing_buffers(pad_ref, buffers, state) do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def handle_ingoing_buffers(pad_ref, buffers, state) do
def handle_incoming_buffers(pad_ref, buffers, state) do

if pad_data.direction == :input,
do: raise("cannot snapshot atomic counter in input pad")
if pad_data.direction == :input do
raise("cannot snapshot atomic counter in input pad")
Copy link
Member

Choose a reason for hiding this comment

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

add pad ref

@FelonEkonom FelonEkonom requested a review from mat-hek April 2, 2024 16:17
@@ -30,7 +30,7 @@ defmodule Membrane.Core.Element.DemandController do
with {:ok, pad_data} when not pad_data.end_of_stream? <- PadModel.get_data(state, pad_ref),
%State{playback: :playing} <- state do
if pad_data.direction == :input do
raise("cannot snapshot atomic counter in input pad")
raise("cannot snapshot atomic counter in input pad #{inspect(pad_ref)}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise("cannot snapshot atomic counter in input pad #{inspect(pad_ref)}")
raise Membrane.ElementError, "Cannot snapshot atomic counter in input pad #{inspect(pad_ref)}"

@mat-hek
Copy link
Member

mat-hek commented Apr 3, 2024

@FelonEkonom BTW I recall you wanted to refactor InputQueue as well

@FelonEkonom FelonEkonom force-pushed the refactor-demand-mechanism branch from 94f6dbf to ad5612a Compare April 3, 2024 10:14
@FelonEkonom FelonEkonom force-pushed the refactor-demand-mechanism branch from ad5612a to 1430ecc Compare April 3, 2024 10:19
@FelonEkonom
Copy link
Member Author

FelonEkonom commented Apr 3, 2024

@mat-hek I know, but then I decided that I would rather focus more on the code related to DemandHandler, because it was more messy and some changes, that I wanted to make in InputQueue (like deleting from it direct dependency to AtomicDemand) turned to be bad ideas

@FelonEkonom FelonEkonom merged commit fb10564 into master Apr 3, 2024
5 of 6 checks passed
@FelonEkonom FelonEkonom deleted the refactor-demand-mechanism branch April 3, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor InputQueue
2 participants