-
Notifications
You must be signed in to change notification settings - Fork 67
ECN support #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ECN support #365
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| // SPDX-FileCopyrightText: 2025 The Pion community <https://pion.ly> | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package transport | ||
|
|
||
| import ( | ||
| "net" | ||
| "time" | ||
| ) | ||
|
|
||
| type NetConnSocket interface { | ||
| net.Conn | ||
|
|
||
| ReadWithAttributes(p []byte, attr *PacketAttributes) (n int, err error) | ||
| } | ||
|
|
||
| type PacketConnSocket interface { | ||
| net.PacketConn | ||
|
|
||
| ReadFromWithAttributes(p []byte, attr *PacketAttributes) (n int, addr net.Addr, err error) | ||
| } | ||
|
|
||
| // NetConnToNetConnSocket wraps a net.Conn and implements the PacketStream interface by delegating | ||
| // calls to the underlying connection. ReadWithAttributes delegates to Read and | ||
| // ignores the provided PacketAttributes. | ||
| type NetConnToNetConnSocket struct { | ||
| conn net.Conn | ||
| } | ||
|
|
||
| // NewNetConnToNetConnSocket returns a new Proxy that wraps the provided net.Conn. | ||
| func NewNetConnToNetConnSocket(conn net.Conn) *NetConnToNetConnSocket { | ||
| return &NetConnToNetConnSocket{conn: conn} | ||
| } | ||
|
|
||
| // ReadWithAttributes reads from the underlying connection and ignores attributes. | ||
| func (p *NetConnToNetConnSocket) ReadWithAttributes(b []byte, _ *PacketAttributes) (int, error) { | ||
| return p.conn.Read(b) | ||
| } | ||
|
|
||
| // Delegate net.Conn methods to the underlying connection. | ||
| func (p *NetConnToNetConnSocket) Read(b []byte) (int, error) { return p.conn.Read(b) } | ||
| func (p *NetConnToNetConnSocket) Write(b []byte) (int, error) { return p.conn.Write(b) } | ||
| func (p *NetConnToNetConnSocket) Close() error { return p.conn.Close() } | ||
| func (p *NetConnToNetConnSocket) LocalAddr() net.Addr { return p.conn.LocalAddr() } | ||
| func (p *NetConnToNetConnSocket) RemoteAddr() net.Addr { return p.conn.RemoteAddr() } | ||
| func (p *NetConnToNetConnSocket) SetDeadline(t time.Time) error { return p.conn.SetDeadline(t) } | ||
| func (p *NetConnToNetConnSocket) SetReadDeadline(t time.Time) error { return p.conn.SetReadDeadline(t) } | ||
| func (p *NetConnToNetConnSocket) SetWriteDeadline(t time.Time) error { | ||
| return p.conn.SetWriteDeadline(t) | ||
| } | ||
|
|
||
| type PacketConnToPacketConnSocket struct { | ||
| conn net.PacketConn | ||
| } | ||
|
|
||
| func NewPacketConnToPacketConnSocket(conn net.PacketConn) *PacketConnToPacketConnSocket { | ||
| return &PacketConnToPacketConnSocket{ | ||
| conn: conn, | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you please explain why we need these two structs and why we need to make them public?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The answer is not in transport but in other repos like srtp and webrtc. The issue is, the socket is abstracted with either net.Conn or net.PacketConn in the code. For example take mux. mux implements net.Conn so it has a method Read or Write (just like a socket) but I needed to tell my mux to ReadWithAttributes. This is why these two structs are defined.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this just be a helper-cast function that simply returns an interface, err without exposing a new public struct?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean something like? var conn net.Conn
if s, ok := conn.(*NetConnSocket); ok != nil {
// some code
}yes this is doable. I'm not much experienced in Go, but I just learned this for the changes I made, so initially I didn't think of this. |
||
|
|
||
| func (u *PacketConnToPacketConnSocket) ReadFromWithAttributes( | ||
| p []byte, _ *PacketAttributes) (n int, addr net.Addr, err error) { | ||
| return u.conn.ReadFrom(p) | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) ReadFrom(p []byte) (n int, addr net.Addr, err error) { | ||
| return u.conn.ReadFrom(p) | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) WriteTo(b []byte, addr net.Addr) (int, error) { | ||
| n, err := u.conn.WriteTo(b, addr) | ||
|
|
||
| return n, err | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) Close() error { | ||
| return u.conn.Close() | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) LocalAddr() net.Addr { | ||
| return u.conn.LocalAddr() | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) SetDeadline(t time.Time) error { | ||
| return u.conn.SetDeadline(t) | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) SetReadDeadline(t time.Time) error { | ||
| return u.conn.SetReadDeadline(t) | ||
| } | ||
|
|
||
| func (u *PacketConnToPacketConnSocket) SetWriteDeadline(t time.Time) error { | ||
| return u.conn.SetWriteDeadline(t) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // SPDX-FileCopyrightText: 2025 The Pion community <https://pion.ly> | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package transport | ||
|
|
||
| const attrMaxLen = 1 | ||
|
|
||
| type ECN uint8 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be better if transport's packet-attributes stayed generic, leaving ECN and other metadata details to higher-level application logic. Because the way i see it is that ECN is a semantics not mechanism, and transport is just a thin layer / helper library, and it shouldn't care about details. this comment also extends to ECN and WithECN.. |
||
|
|
||
| type PacketAttributesBuffer interface { | ||
| // for serializing | ||
| Marshal() []byte | ||
|
|
||
| // for de-serializing. The bytes will be copied into the returned buffer | ||
| GetBuffer() []byte | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe marshal/Unmarshal or Bytes/Scratch or any common serialize/de-serialize naming convention that is common in Go.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes yes I'm familiar with that. My idea was, GetBuffer returns the attributes buffer so bytes can be directly copied into the PacketAttributes' buffer instead of copying first into a temp buffer and then calling UnMarshall. The reason is, through a packet's journey, a packet is written/read to the packet buffer multiple times. If you think there's a better option or UnMarshall is a good enough name, I'm fine with that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like marshal/unmarshall personally, but generally it's better to follow the naming conventions. as long as you also have marshal. |
||
| } | ||
|
|
||
| type PacketAttributes struct { | ||
| buffer [attrMaxLen]byte | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should make the attribute size dynamic and add a byte to determine how many attributes are there so we don't have to make attributesv2 in the future.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No we don't really need to make attributesv2 in the future. If we were to add more fields to the PacketAttributes we can just do it. The packet buffer is agnostic of PacketAttributes' buffer length and it can work with any variable sized PacketAttributes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment was more about that we don't need to touch pion/transport to add new attributes, we should make it generic, IMO: no |
||
| } | ||
|
|
||
| func NewPacketAttributes() *PacketAttributes { | ||
| p := &PacketAttributes{} | ||
| p.Reset() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to call reset here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. You're right! |
||
|
|
||
| return p | ||
| } | ||
|
|
||
| func (p *PacketAttributes) Reset() { | ||
| p.WithECN(0) | ||
| } | ||
|
|
||
| func (p *PacketAttributes) GetECN() ECN { | ||
| return ECN(p.buffer[0]) | ||
| } | ||
|
|
||
| func (p *PacketAttributes) WithECN(e ECN) *PacketAttributes { | ||
| p.buffer[0] = byte(e) | ||
|
|
||
| return p | ||
| } | ||
|
|
||
| // Marshal returns the internal buffer as-is. | ||
| func (p *PacketAttributes) Marshal() []byte { | ||
| return p.buffer[:] | ||
| } | ||
|
|
||
| func (p *PacketAttributes) GetBuffer() []byte { | ||
| return p.buffer[:] | ||
| } | ||
|
|
||
| func (p *PacketAttributes) Clone() *PacketAttributes { | ||
| clone := &PacketAttributes{} | ||
| clone.buffer[0] = p.buffer[0] | ||
|
|
||
| return clone | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we unify attribute types in the APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having a generic packet attributes type with variable buffer length would address this comment right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah a generic packet attributes will be cool, good idea.