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

Fix manual demands on ended inputs #117

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

Karolk99
Copy link
Contributor

No description provided.

@Karolk99 Karolk99 requested a review from mat-hek as a code owner July 31, 2024 15:51
@Karolk99 Karolk99 force-pushed the fix/manual-demand-on-ended-input branch 2 times, most recently from fbd6cf7 to 98a6316 Compare July 31, 2024 15:53
@Karolk99 Karolk99 force-pushed the fix/manual-demand-on-ended-input branch from 98a6316 to 2f71e9d Compare August 1, 2024 08:48
@mat-hek mat-hek self-assigned this Aug 1, 2024
@mat-hek mat-hek added the bug Something isn't working label Aug 1, 2024
Comment on lines -406 to +408
if SamplesQueue.empty?(cache) do
if processing_finished? do
end_of_streams = generate_output_end_of_streams(ctx)
if SamplesQueue.empty?(cache) and processing_finished? do
end_of_streams = generate_output_end_of_streams(ctx)

{end_of_streams, state}
else
{[redemand: output_pad], state}
end
{end_of_streams, state}
Copy link
Member

@mat-hek mat-hek Aug 1, 2024

Choose a reason for hiding this comment

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

  • this still doesn't guarantee we don't demand on an EOSed pad
  • are you sure we never need this redemand here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't change this particular clause at all just to be clear, and yes we don't need to do redemand in this case because there is no input pad left and the SampleQueue is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this still doesn't guarantee we don't demand on an EOSed pad why is that?

Copy link
Member

Choose a reason for hiding this comment

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

When processing_finished? is false, some input pads are still left. Previously we redemanded when processing_finished? was false. Now we don't unless I'm missing something.

this still doesn't guarantee we don't demand on an EOSed pad why is that?

Redemanding means that handle_demand is executed immediately if there's still some demand on output. But if we don't redemand, handle_demand can still be executed right away - it's up to the framework. In that case, your change doesn't make any difference from the current behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generate_end_of_stream_segment sends redemand so it stays the same and also mark pad as ended by setting endtimestamp to nil so it should help with issue of demands of ended pads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when it comes to demanding withoug redemand the only situation in which we don't send redemand is when we send end of stream on output

@Karolk99 Karolk99 requested a review from mat-hek August 2, 2024 09:55
@darthez darthez merged commit df35516 into master Aug 7, 2024
3 checks passed
@darthez darthez deleted the fix/manual-demand-on-ended-input branch August 7, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants