Wallacoloo / serde_osc

Serialization and Deserialization to/from Open Sound Control packets using Serde
Apache License 2.0
7 stars 2 forks source link

UDP transport does not work as is. The spec appears to only require the length at the start of a packet for streaming protocols #3

Open apps4uco opened 3 years ago

apps4uco commented 3 years ago

The spec http://opensoundcontrol.org/spec-1_0 says

"An OSC packet can be naturally represented by a datagram by a network protocol such as UDP. In a stream-based protocol such as TCP, the stream should begin with an int32 giving the size of the first packet, followed by the contents of the first packet, followed by the size of the second packet, etc."

So my understanding is that if the transport is UDP then the packets/messages should not be prefixed with the length, This is also how Ardour interprets the spec.

However in

msg_serializer.rs and bundle_serializer.rs

There is no flag to optionally include the size if it is TCP

Actual:

// Write the packet length output.osc_write_i32(payload_size.try_into()?)?;

Suggestion:

if stream_based_connection //eg tcp { output.osc_write_i32(payload_size.try_into()?)?; }

Would this be possible?

Wallacoloo commented 3 years ago

Thanks for writing! This should be possible. You already identified where the framing happens (MsgSerializer::to_write and BundleSerializer::to_write). I would just remove the lines from those functions which do the framing, and push that upstream.

The simplest place to move it is all the way to the top-level function here. Right now, it looks like this:

/// Serialize `value` into an OSC packet, and write the contents into `write`.
/// Note that serialization of structs is done only based on the ordering
/// of fields; their names are not preserved in the output.
pub fn to_write<S: ?Sized, W: Write>(write: &mut W, value: &S) -> ResultE<()>
    where W: Write, S: serde::ser::Serialize
{
    let mut ser = Serializer::new(write.by_ref());
    value.serialize(&mut ser)
}

It could look something like:

pub fn to_write<S: ?Sized, W: Write>(write: &mut W, value: &S, framing: Framing) -> ResultE<()>
    where W: Write, S: serde::ser::Serialize
{
    match framing {
        Framing::Unframed => {
            let mut ser = Serializer::new(write);
            value.serialize(&mut ser)
        },
        Framing::Framed => {
            let mut buffer = Cursor::new(Vec::new());
            let mut ser = Serializer::new(&mut buffer);
            value.serialize(&mut ser)?;
            write.osc_write_i32(buffer.len().try_into()?)?;
            write.write(buffer)
        }
    }
}

After adding some Framing enum. The to_vec method would need to be updated to accept that Framing argument as well. If you go this route, I suspect you'll need to change this line in the BundleSerializer so that it frames each of its packets (i.e. have it call into to_write, passing Framing::Framed) -- since I think we still need framing there regardless of the transport layer.

I'm not terribly concerned about perf. The buffering might add some overhead, but right now the MsgSerializer does its own buffering in order to do its own framing, so it might actually not have any perf impact if someone else comes by and removes the extra buffer there after doing this work.

I don't have the resources to tackle that right now, but if you or anyone reading this wants to take a stab at it, I'm happy to field any PR's and help get them merged, and I'll take care of pushing a new version to crates.io with the change.