Change Attributes from map interface key to use a typed key

[OrlandoCo]

Summary

This issue is to discuss the possibility to change the attr map from interface key to a typed key. As maybe I'm missing a use case for an interface key, we should discuss if it's neccesary.

Motivation

Usually attributes list should not be big enough to take advantage of a Map, from a performance perspective a slice should perform far better, if we decide to stick to a map, we should consider using a typed key.

An interface key may perfrom 10x worst because it needs to perform the following steps:

• If both "sides" of the cmp are of same types.
• If that type has '==' defined. ([]slices do not, for example)
• If the values are equal or not, but only using a dynamically dispatched "comparator"
  source
  

cc: @Sean-Der @masterada @tarrencev @jech

[jech]

It is my understanding that attributes are built and discarded for each packet, so they're likely to be fairly small. I would tend to agree with you that using a slice is likely to be more efficient.

Slices have better memory locality, and they can be searched by pointer equality:

type AttributeKey *int
func NewAttributeKey() AttributeKey {
   return new(*int)
}
type Attribute struct {
   key AttributeKey
   value interface{}
}
type Attributes []Attribute
func (as *Attributes) Get(key AttributeKey) interface{} {
    for _, a := range as {
        if k == a.key {
          return a.value
        }
    } 
    return nil
}

[Sean-Der]

That is a great point! I am all for going with slices if that makes life easier for everyone :)

If we had a few helpers for Attributes as well it should feel pretty much the same for the user.

[jech]

Perhaps we're not ready to stabilise the interceptor API yet? Perhaps the v3 release should indicate that the interceptor API is still likely to change?

[masterada]

My original considerations for attributes:

Purpose of the attributes

The purpose of attributes is to pass metadata belonging to the packet between interceptors, and ideally between interceptor and the user (developer). Some idea for the usage:

• attaching arrival time to packets in the first interceptor (because some interceptors, like a jitter buffer will delay packets, so calling time.Now after those will not return the correct arrival time)
• calculating an unwrapped sequence number once, and passing it
• adding a send time time.Time attribute to packets, for audio/video sync (this can be done using the NTP time from incoming rtcp SenderReports) - this would be most useful if the user could read the attributes too, not just the interceptors
• debugging

Passing attributes with rtp packets

I'm really not happy with how attributes are passed right now. I think it would be best to attach them to the RTP packet somehow, I just haven't found a good solution to that. Some things I considered:

• adding Attributes field to rtp.Packet
  • this would modify the public api
• adding attributes unexported field to rtp.Packet, and adding a GetAttribute(p *Packet, key interface{}) interface{}, and a similar SetAttribute method to the rtp packet
  • this is ugly
• use a separate global map for attributes (eg: key is ssrc+sequence number, value is attributes)
  • it's hard to know when it's safe to remove attributes for a packet
  • also very ugly
• modifying rtp.Packet to be an interface, and creating a wrapper for this in interceptor packet(see below)
  • this would require a LOT of modification

type PacketWithAttribute struct {
    rtp.Packet
    attributes map[interface{}]interface{}
}

func GetPacketAttribute(p rtp.Packet, key interface{}) interface{} {
    pa, ok := p.(*PacketWithAttribute)
    if !ok {
        return nil
    }
    return pa.attributes[key]
} 

Any idea is welcome for this!

Attributes type and key type

The key type is interface{} instead of string, because I planned to use attributes with unexported struct keys, similarly to how context.Context usage is recommended (this would also mean generic SetUint32 and similar methods would be unnecesary, as every attribute would have it's own setter/getter):

    type fooKey struct{}

    func SetFoo(attributes map[interface{}]interface{}, value string) {
        attributes[fooKey{}] = value
    }

    func GetFoo(attributes map[interface{}]interface{}) string {
        str, _ := attributes[fooKey{}].(string)
        return str
    }

I'm all for using a dedicated `type Attribtes map[interface{}]interface{} though. We can simply use string keys as well if you guys think this is unnecessarily complicated.

[masterada]

All that said, I don't have a strong feeling about either form, if you think the slice would be better then let's change to slice.

[OrlandoCo]

All that said, I don't have a strong feeling about either form, if you think the slice would be better then let's change to slice.

Interceptor will run on hot paths in Pion, with streams running upto 2.5 mbps, that's a lot of reads per second, map usage only benefits when n is big, so if there is no usage of a key interface or a map at all, we should fallback to slice for better peformance.

If there is no special requirement to use a map I think that slice would work great here @Sean-Der @masterada

[masterada]

Just a note: if slice is used for Attributes, instead of Set we should use With method that returns the modified slice.

[jech]

Thanks, masterada, this is helpful.

Looking at your examples, it looks like there are two requirements that are not met by the current interface:

• allowing the final user to get an attribute (e.g. the original reception time or the network congestion status at the time the packet was received);
• allowing the orginal producer to attach attributes to the orginal packet (e.g. a track taht comes from a non-RTP device that provides accurate NTP timestamps with each packet, or a transport-layer protocol that provides explicit congestion notification).

[jech]

Possibly stupid idea — what if both Attributes and AttributeKey are opaque types? Would that enable us to change the type in the future?

I.e. we say something like

type Attributes *attributes
type AttributeKey attributeKey
func NewAttributeKey() AttributeKey
func (a Attributes) Set(k attributeKey, v interface{})
func (a Attributes) Get(k attributeKey) interface()

and never export the underlying types?