Implemented explicit flushing of buffered samples from sink to file #763
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
work in progress
jeshwanthreddy13
changed the title
github-actions
bot
added
ready for review
gavv
reviewed
Jul 25, 2024
Comment on lines
112
to
119
| //! Flush buffered samples to disk | ||
| virtual ROC_ATTR_NODISCARD status::StatusCode flush(); |
There was a problem hiding this comment.
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.
| return code; | ||
| return sink_.flush(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also:
- please move flush call before "exiting main loop" message
- if flush fails, please log error and status (see status::code_to_str)
Comment on lines
220
to
240
| status::StatusCode SoxSink::flush() { | ||
| if (fflush((FILE*)output_->fp) == 0) { | ||
| return status::StatusOK; | ||
| } | ||
|
|
||
| return status::StatusErrFile; | ||
| } |
There was a problem hiding this comment.
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..)output_->fpcan't be used if SoxSink is device. We should use it only ifdriver_type_ == DriverType_File, otherwise flush should be no-op.- Let's also check that
output_->fpis non-NULL. If it's null, let's make flush a no-op too. - nit: Usually early returns are used for errors and the very last return for success.
There was a problem hiding this comment.
Done, also implemented a fix in write() where buffer will write to sox only if buffer_pos > 0
gavv
added
needs revision
github-actions
bot
added
the
needs rebase
|
🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts. |
github-actions
bot
removed
the
needs rebase
jeshwanthreddy13
force-pushed
the
develop
branch
from
b357541
to
d6d64c0
Compare
|
🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts. |
github-actions
bot
added
the
needs rebase
github-actions
bot
added
work in progress
jeshwanthreddy13
force-pushed
the
develop
branch
from
d6d64c0
to
06f05ae
Compare
github-actions
bot
removed
the
needs rebase
github-actions
bot
added
ready for review
github-actions
bot
added
work in progress
github-actions
bot
added
ready for review
…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
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #703
virtual ROC_ATTR_NODISCARD status::StatusCode flush()=0pure virtual function insndio::ISink.flush()when it exits fromrun().