j0r1 / JRTPLIB

RTP Library
MIT License
581 stars 220 forks source link

rtp argument of RTPRawPacket complicates things #16

Closed DavidNemeskey closed 5 years ago

DavidNemeskey commented 6 years ago

I use JRTPLIB to parse RTP / RTCP packages, and I was wondering if the rtp argument of the RTPRawPacket contructor makes any sense. Normally, this is how I would go about parsing a packet:

RTPRawPacket raw_packet(data);
RTPPacket rtp_packet(raw_packet);
if (rtp_packet.GetCreationError()) {
    RTCPCompoundPacket rtcp_packet(raw_packet);
    ...
} else {
    ...
}

However, this code fails when data represents an RTCP packet: in order to parse raw_packet as an RTPPacket, I have to instantiate it with rtp=true; but then the creation of rtcp_packet will fail by default. So effectively if rtp_packet could not be created, I need to create another raw packet, this time with rtp=false, and call the RTCPCompoundPacket constructor on that.

Not only is the process more cumbersome that it should be, with parsing in mind, the parameter doesn't make much sense, because it forces me to make a blind guess about the contents of the packet.

j0r1 commented 6 years ago

Originally, in RFC 3550, there was a port for the RTP packets and a different port for the RTCP packets, and the flag indicates on which port this packet was received. Now, RTP and RTCP packets can optionally be multiplexed, and to figure out if it's an RTP or RTCP packet one needs to check the second byte: if its value lies between 200 and 204, it's considered to be an RTCP packet, otherwise an RTP packet.

Perhaps I should change that boolean to an enum which has 'RTP', 'RTCP' and 'Guess' options, where the first two cause the current behaviour and the third looks at that second byte?

DavidNemeskey commented 6 years ago

That would be perfect.

j0r1 commented 6 years ago

After some thought it seemed easier to just add a different constructor which omits that boolean parameter. I've added it in a branch rawpacketguess, does that work for you?

DavidNemeskey commented 5 years ago

I've just checked it, works perfectly.

j0r1 commented 5 years ago

Ok great, thanks for testing. I've added it to master