roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.02k stars 203 forks source link

Add return code to IFrameWriter::write() #685

Closed gavv closed 2 months ago

gavv commented 5 months ago

audio::IFrameWriter is an interface implemented by chained elements of processing pipeline. In the beginning of the pipeline, you write a frame to the first element, it performs some processing, writes processed frame to the next element, and so on.

Currently it's assumed that write() can never fail:

    //! Write audio frame.
    virtual void write(Frame& frame) = 0;

As the first step of a bigger task (#183, #614, #615), we need to add return codes to the write() method, to be able to report errors from pipeline to the upper level. It should become:

    //! Write audio frame.
    virtual ROC_ATTR_NODISCARD status::StatusCode write(Frame& frame) = 0;

Currently there is ~10 implementations of IFrameWriter + some mocks in tests. They all need to be adjusted to forward status codes.

Most of them just need to forward status from underlying writer. If nested write() call returns error (any status except StatusOK), we should stop processing frame and forward that error. If nested write() returns StatusOK, we do everything we need to do and in the end also return StatusOK.

TylerHullWork commented 4 months ago

Hi, I would like to help with this issue. However, I am a first-time open source contributor. Could you please give me some advice for contributing here? Also, after looking at some of the documentation, I am wondering if this project can be compiled on windows or only linux/mac? Thanks.

gavv commented 4 months ago

Hi! Thanks and welcome.

Please take a look at this page: https://roc-streaming.org/toolkit/docs/development/contribution_guidelines.html

Feel free to ask questions here or in matrix chat (link in readme).

We don't have windows support yet (btw we also have help-wanted issue for it). I know that some people are building roc in wsl.

TylerHullWork commented 4 months ago

to clarify, when it is stated that there are ~10 implementations of iframe_writer, that is referring to the channel_mapper_writer, null_writer, profiling_writer, etc?

Also, how exactly is an error code forwarded in this context? Is there a specific file I can look to for an example on that? Thanks.

gavv commented 4 months ago

to clarify, when it is stated that there are ~10 implementations of iframe_writer, that is referring to the channel_mapper_writer, null_writer, profiling_writer, etc?

Yes.

Also, how exactly is an error code forwarded in this context? Is there a specific file I can look to for an example on that? Thanks.

Here is an example: https://github.com/roc-streaming/roc-toolkit/blob/69d68bd3eea3e0fca7ad6764c6dd2011e889c438/src/internal_modules/roc_packet/delayed_reader.cpp#L42-L49

(it's for packet reader, not frame writer, but idea is the same).

For example, in ChannelMapperWriter, if outputwriter.write() returns any status different from StatusOK, it should interrupt loop and return the same status.

In other words, if everything returns StatusOK, everything continues to work same as currently. If anything returns not-OK, then everything returns early and forwards the returned error to the upper level.

TylerHullWork commented 4 months ago

https://github.com/roc-streaming/roc-toolkit/pull/699

Hi again, I submitted a pull request in order to be reviewed. This request is just a temporary request in order to share the code changes here, apologies if that was not the right way to go about that. I am wondering if the logic behind the changes I made to these write codes are accurate as to what you are looking for. Particularly the resampler_writer.cpp file changes. If this logic is correct, then it seems like it is probably simple to apply this to all of the other derived classes from iFrameWriter.

Still being new to open source contributions, and in addition having to set up a linux environment, I have been unable to compile the code to test it (with these changes it would definitely not run at the moment for obvious reasons, but I have not been able to run even an unmodified version of the code), so if you have any advice on how to do that I would greatly appreciate it.

gavv commented 4 months ago

I don't get what you're trying to achieve/show with that patch? All those classes don't have fetchpackets method.

For build instructions, take a look at https://roc-streaming.org/toolkit/docs/building.html

gavv commented 2 months ago

Closing with respect to #614.