Support error codes and partial reads and writes in audio readers and writers

[gavv]

Problem

Here is how audio::IReader and audio::IWriter interfaces look like currently:

class IReader {
public:
    // can't fail
    // returns false on EOF; not used by most implementations
    virtual bool read(Frame& frame) = 0;
};

class IWriter {
public:
    // can't fail
    virtual void write(Frame& frame) = 0;
};

We need two improvements here:

• Allow read() and write() operations to fail and report error to the upper code. This is needed to handle run-time error gracefully.

• Allow read() and write() operations to process lesser samples than requested and report how much samples were actually processed. This is needed to implement PLC.

Solution

Convert the above interfaces to the following:

class IReader {
public:
    virtual ssize_t read(Frame& frame) = 0;
};

class IWriter {
public:
    virtual ssize_t write(Frame& frame) = 0;
};

(ssize_t is provided by roc_core/stddefs.h)

In particular:

• On success, read() and write() will return zero or positive number of samples read or written, in range [0; frame.size()).

• On failure, read() and write() will return a negative error code defined in roc_error module (roc_error/error_code.h).

• On EOF, read() should return error::EOF. This error may be returned by various implementations of sndio::ISource (which inherits audio::IReader). Currently EOF is reported as returning false.

After changing the interfaces, we should also find and fix all audio::IReader and audio::IWriter implementations:

• Fix function signature.

• Fix invocation of nested audio readers and writers. Two fixes are needed:

  • if nested reader / writer returns a negative value, we should break and return that value to the upper level;

  • if nested reader / writer returns lesser samples than requested, we should either also break and return lesser samples to the upper samples, OR repeat the read / write operation until desired number of samples reached. What behavior we need depends on the specific implementation:

  • the following components should repeat read / write: audio::ResamplerReader, audio::ResamplerWriter, audio::Mixer, pipeline::Receiver, pipeline::Sender;

    (why? just because it'd be hard to handle partial reads / writes in these components and, on the other hand, we either don't need it now)

  • the rest components can just return the number of samples returned from nested elements.

• Fix unit tests for the modified components and add a few tests for the new behavior (test that error codes are forwarded and partial reads / writes are handled as expected).

Notes

Since currently read() and write() can not fail, after this change all implementations will actually always report that the frame was successfully and completely read or write, and error and partial read/write handling code will never trigger, except unit tests.

However, after finishing this task we will be able to add error reporting and partial reads to some components. This will be done separately.

[aj-thomas-8]

Hey I can work on this if no one else is already doing so

[gavv]

@aj-thomas-8 You're welcome!

[aj-thomas-8]

In the case of nested readers, if a read operation returns an EOF error, is it still supposed to break and return the EOF error?

[gavv]

I think yes. EOF should indicate that the frame was not read and there are no more frames to read. BTW probably better name it ErrEOS (end of stream) or maybe even ErrEndOfStream.

[aj-thomas-8]

Should calls to the panic macros in write/read operations be replaced by breaking and returning the appropriate error code instead? For instance, the write function of the Packetizer class calls a panic macro if the encoder doesn't encode the expected number of samples. So would this be replaced by the write function returning an error code instead?

[gavv]

No, panics are used like assertions. They cover internal contracts / invariants and triggering a panic means a bug in the code to be fixed, but not a valid situation to be handled.

In particular, encoder may write lesser samples than requested only when the buffer is too small. However, packetizer takes care to calculate the proper buffer size, so this panic can't happen unless there is a bug in packetizer.

[aj-thomas-8]

That makes sense. Thanks!

[aj-thomas-8]

In the write function of the packetizer class, will only frame.size() / num_channels samples be encoded and written to a packet for each frame? Do the rest of the samples in the frame just get ignored? Sorry if I completely misunderstood how the packetizer works

[gavv]

The trick is that frame.size() is the number of samples total for all channels, and IFrameEncoder::write() expects the number of samples per channel, plus a channel mask. (This is mentioned in the corresponding method comments, hopefully).

Say, we have a frame with 2 channels (stereo) and 100 samples for each channel, then we'll have:

frame.size() == 200
num_channels_ == 2
buffer_samples == 100

We'll pass n_samples = 100 and channel_mask = left | righ to payload_encoder_.write(), and, assuming there is enough space in the buffer, it will return 100 as well (actual_ns), which would mean that it processed all 200 samples, 100 per each channel.

If you will be modifying the packetizer code, feel free to add a comment to make this more clear for the reader. This is really far from obvious.

[aj-thomas-8]

That makes things a lot clearer. Thanks! And I'll add a comment to the packetizer code as well

[aj-thomas-8]

Just to clarify, in the solution description, when you said the nested read/write calls in pipeline::Sender and pipeline::Receiver should repeat till the expected number of packets are read/written, you meant pipeline::SenderSink and pipeline::ReceiverSource right?

[gavv]

Yeah, they were renamed recently.

[aj-thomas-8]

In the case of Fanout, wouldn't the expected number of samples written be frame.size() * number of output writers? Because if Fanout::write(Frame frame) were to return the number of samples in the range [0: frame.size()] (like other write functions), that wouldn't account for potential partial writes by the nested output writers.

[gavv]

In the case of Fanout, wouldn't the expected number of samples written be frame.size() * number of output writers?

Hm, no, the returned value reports how much of input samples were processed. And the caller wont expect that write(N) will return, say, N*2 samples, it will break things.

Because if Fanout::write(Frame frame) were to return the number of samples in the range [0: frame.size()] (like other write functions), that wouldn't account for potential partial writes by the nested output writers.

Let's keep it simple and add a loop to Fanout, so that in case of a partial write it will repeat and write the remaining samples until the full frame is written. Just like it's suggested to do in Mixer and some other components.

It wont prevent us for implementing PLC (for which we need partial reads/writes), so this would be a good start.

[aj-thomas-8]

Awesome thanks! I'll do that

[gavv]

@aj-thomas-8 Hi, do you still have plans on this?

[aj-thomas-8]

Sorry, I’ve had a bunch of things come up so I won’t be able to work on this unfortunately

[gavv]

@aj-thomas-8 No problem! Unassigning this issue so someone else could pick it up. Feel free to come back to the project if you'll have time and interest in future.

[bkayes]

can I work on this issue?

[gavv]

@bkayes You're welcome!

[gavv]

@bkayes Hi, do you have plans on this?

[samarthagali]

@gavv Hi, I'm interested in working on the issue!

[gavv]

@samarthagali You're welcome, thanks! Let me know if you'll have any questions.

I suggest to handle readers and writers in separate PRs, for easier and faster review.

Related issue: https://github.com/roc-streaming/roc-toolkit/issues/303

[gavv]

@samarthagali Hi!

@dshil currently works on #303 (see #570). We're discussing a redesign of error / status codes.

Are you working on the task and what are your plans?

[gavv]

Moved to #614 and #615.