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

Implemented explicit flushing of buffered samples from sink to file #763

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

jeshwanthreddy13
Copy link

@jeshwanthreddy13 jeshwanthreddy13 commented Jul 21, 2024

Fixes #703

  • Added virtual ROC_ATTR_NODISCARD status::StatusCode flush()=0 pure virtual function in sndio::ISink.
  • Added this method to all ISink implementations.
  • Implementations that don't use buffereing are no-op.
  • For SoxSink, flush() sends the buffered samples to disk.
  • sndio::Pump invokes flush() when it exits from run().

@github-actions github-actions bot added the work in progress Pull request is still in progress and changing label Jul 21, 2024
@jeshwanthreddy13 jeshwanthreddy13 changed the title Implemented explicit flushing of buffered samples from sink to file #703 Implemented explicit flushing of buffered samples from sink to file Jul 21, 2024
@gavv gavv added the contribution A pull-request by someone else except maintainers label Jul 21, 2024
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as ready for review July 21, 2024 17:48
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Jul 21, 2024
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for PR! A few comments.

Comment on lines 112 to 119
//! Flush buffered samples to disk
virtual ROC_ATTR_NODISCARD status::StatusCode flush();
Copy link
Member

Choose a reason for hiding this comment

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

Here and in other headers: it's not necessarily flushing to disk because e.g. there are pipeline sinks that will later implement flushing to network queue.

We can reword it like:

Flush buffered data, if any.

Copy link
Author

Choose a reason for hiding this comment

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

Done

return code;
return sink_.flush();
Copy link
Member

Choose a reason for hiding this comment

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

If code was not StatusOK before flushing, we should return original code. Only if there were no previous errors before flush and code was StatusOK, we should return code from flush.

(It is a good practice to return earliest error happened, because later errors can be caused by the very first one).

Currently, we just drop previous code, which is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Also:

  • please move flush call before "exiting main loop" message
  • if flush fails, please log error and status (see status::code_to_str)

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 220 to 240
status::StatusCode SoxSink::flush() {
if (fflush((FILE*)output_->fp) == 0) {
return status::StatusOK;
}

return status::StatusErrFile;
}
Copy link
Member

Choose a reason for hiding this comment

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

  1. output_ may be NULL, when sink is paused. I think flush should be no-op in this case. (BTW we need to address this in write() too..)
  2. output_->fp can't be used if SoxSink is device. We should use it only if driver_type_ == DriverType_File, otherwise flush should be no-op.
  3. Let's also check that output_->fp is non-NULL. If it's null, let's make flush a no-op too.
  4. nit: Usually early returns are used for errors and the very last return for success.

Copy link
Author

Choose a reason for hiding this comment

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

Done, also implemented a fix in write() where buffer will write to sox only if buffer_pos > 0

@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Jul 25, 2024
@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Jul 28, 2024
Copy link

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot removed the needs rebase Pull request has conflicts and should be rebased label Aug 3, 2024
Copy link

github-actions bot commented Aug 6, 2024

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Aug 6, 2024
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 12:53
@github-actions github-actions bot added work in progress Pull request is still in progress and changing and removed needs revision Pull request should be revised by its author labels Aug 6, 2024
@github-actions github-actions bot removed the needs rebase Pull request has conflicts and should be rebased label Aug 6, 2024
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as ready for review August 6, 2024 14:20
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Aug 6, 2024
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as draft August 6, 2024 14:22
@github-actions github-actions bot added work in progress Pull request is still in progress and changing and removed ready for review Pull request can be reviewed labels Aug 6, 2024
@jeshwanthreddy13 jeshwanthreddy13 marked this pull request as ready for review August 6, 2024 14:47
@jeshwanthreddy13 jeshwanthreddy13 requested a review from gavv August 6, 2024 14:47
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Aug 6, 2024
@gavv gavv added this to the next milestone Aug 7, 2024
…s from sink to file

- Added `virtual ROC_ATTR_NODISCARD status::StatusCode flush()=0` pure virtual function in `sndio::ISink`.
- Added this method to all ISink implementations.
- Implementations that don't use buffereing are no-op.
- For SoxSink, flush() sends the buffered samples to disk.
- sndio::Pump invokes `flush()` when it exits from `run()`.
- Made SoxSink::write to write only when buffer_pos is greater than zero
@gavv gavv merged commit ae5fe2e into roc-streaming:develop Aug 7, 2024
1 check passed
@gavv
Copy link
Member

gavv commented Aug 7, 2024

Thank you!

@github-actions github-actions bot removed the ready for review Pull request can be reviewed label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants