cberner / raptorq

Rust implementation of RaptorQ (RFC6330)
Apache License 2.0
264 stars 47 forks source link

Possible inconsistency with RFC 6330: use of ISI vs ESI in `PayloadId` #164

Closed mlegner closed 5 months ago

mlegner commented 6 months ago

First of all, thank you very much for maintaining this efficient RaptorQ library, this is highly appreciated. 💯

While integrating this into one of my projects, I believe I found an inconsistency of the library's behavior with RFC 6330.

RFC 6330, Section 5.3.1 states the following (emphasis added):

For a given source block of K source symbols, for encoding and decoding purposes, the source block is augmented with K'-K additional padding symbols [...] The encoding symbol ID (ESI) is used by a sender and receiver to identify the encoding symbols of a source block, [...]. For a source block with K source symbols, the ESIs for the source symbols are 0, 1, 2, ..., K-1, and the ESIs for the repair symbols are K, K+1, K+2, .... Using the ESI for identifying encoding symbols in transport ensures that the ESI values continue consecutively between the source and repair symbols.

What I get when running the code, however, is a gap between the source and repair symbols, which should only be there for the internal symbol ID (ISI):

For purposes of encoding and decoding data, the value of K' derived from K is used as the number of source symbols of the extended source block upon which encoding and decoding operations are performed, where the K' source symbols consist of the original K source symbols and an additional K'-K padding symbols. The Internal Symbol ID (ISI) is used by the encoder and decoder to identify the symbols associated with the extended source block, i.e., for generating encoding symbols and for decoding. For a source block with K original source symbols, the ISIs for the original source symbols are 0, 1, 2, ..., K-1, the ISIs for the K'-K padding symbols are K, K+1, K+2, ..., K'-1, and the ISIs for the repair symbols are K', K'+1, K'+2, ....

AFAIU the RFC, the PayloadId should contain the ESI, but the library actually adds the ISI (in SourceBlockEncoder::repair_packets). Using the ESI instead of the ISI would also prevent potential panics in decode.

Example code:

use raptorq::{Encoder, ObjectTransmissionInformation};

fn main() {
    let encoder = Encoder::new(
        &[1, 2, 3],
        ObjectTransmissionInformation::new(3, 1, 1, 0, 1),
    );
    for packet in encoder.get_encoded_packets(3) {
        println!("{}", packet.payload_id().encoding_symbol_id());
    }
}

Example output (K'=10 for K=3):

0
1
2
10
11
12
cberner commented 6 months ago

Ah, ya that wouldn't surprise me. I haven't looked at that code in a while. Want to send a PR? (and a test reproducing the panic would be great!)

Sent from my phone

On Tue, Mar 5, 2024, 8:22 AM Markus Legner @.***> wrote:

First of all, thank you very much for maintaining this efficient RaptorQ library, this is highly appreciated. 💯

While integrating this into one of my projects, I believe I found an inconsistency of the library's behavior with RFC 6330.

RFC 6330, Section 5.3.1 https://datatracker.ietf.org/doc/html/rfc6330#section-5.3.1 states the following (emphasis added):

For a given source block of K source symbols, for encoding and decoding purposes, the source block is augmented with K'-K additional padding symbols [...] The encoding symbol ID (ESI) is used by a sender and receiver to identify the encoding symbols of a source block, [...]. For a source block with K source symbols, the ESIs for the source symbols are 0, 1, 2, ..., K-1, and the ESIs for the repair symbols are K, K+1, K+2, .... Using the ESI for identifying encoding symbols in transport ensures that the ESI values continue consecutively between the source and repair symbols.

What I get when running the code, however, is a gap between the source and repair symbols, which should only be there for the internal symbol ID (ISI):

For purposes of encoding and decoding data, the value of K' derived from K is used as the number of source symbols of the extended source block upon which encoding and decoding operations are performed, where the K' source symbols consist of the original K source symbols and an additional K'-K padding symbols. The Internal Symbol ID (ISI) is used by the encoder and decoder to identify the symbols associated with the extended source block, i.e., for generating encoding symbols and for decoding. For a source block with K original source symbols, the ISIs for the original source symbols are 0, 1, 2, ..., K-1, the ISIs for the K'-K padding symbols are K, K+1, K+2, ..., K'-1, and the ISIs for the repair symbols are K', K'+1, K'+2, ....

AFAIU the RFC, the PayloadId should contain the ESI, but the library actually adds the ISI (in SourceBlockEncoder::repair_packets https://github.com/cberner/raptorq/blob/master/src/encoder.rs#L314). Using the ESI instead of the ISI would also prevent potential panics in decode https://github.com/cberner/raptorq/blob/master/src/decoder.rs#L264.

Example code:

use raptorq::{Encoder, ObjectTransmissionInformation}; fn main() { let encoder = Encoder::new( &[1, 2, 3], ObjectTransmissionInformation::new(3, 1, 1, 0, 1), ); for packet in encoder.get_encoded_packets(3) { println!("{}", packet.payload_id().encoding_symbol_id()); }}

Example output (K'=10 for K=3):

0 1 2 10 11 12

— Reply to this email directly, view it on GitHub https://github.com/cberner/raptorq/issues/164, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGNXQAAZQU4IJT2HOUAFWDYWXWCRAVCNFSM6AAAAABEHOJET2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGE3DSNRTHE2DQMY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mlegner commented 6 months ago

Happy to create a PR for this. This would be a breaking change, however, as the new code would no longer be compatible with the old one.

cberner commented 6 months ago

That's fine. If it's not currently compliant with the spec we should fix that. I'll just release it as 2.0 to make clear the incompatibility

mlegner commented 6 months ago

I created a PR, please let me know what you think.

I didn't add a test for the panic though, as this no longer exists after the changes.