payload_type hardcoded

[zoff99]

c-toxcore/toxav/rtp.h

Line 34 in 0542ffa

rtp_TypeAudio = 192,

should this be:
PACKET_ID_LOSSLESS_RANGE_START + PACKET_ID_LOSSLESS_RANGE_SIZE
?

but seems to be hardcoded now

[zugz]

* Thursday, 2017-11-02 at 15:32 +0000 - Zoff <[email protected]>:

https://github.com/TokTok/c-toxcore/blob/0542ffa2db75fc6affd1b5c83bcb5af8e7d92c88/toxav/rtp.h#L34 should this be: PACKET_ID_LOSSLESS_RANGE_START + PACKET_ID_LOSSLESS_RANGE_SIZE

Why that? It seems to be used just as a magic number.

[zoff99]

i think its used as lossless packet type

c-toxcore/toxav/toxav.c

Line 1209 in 0542ffa

call->audio.first = rtp_new(rtp_TypeAudio, av->m, call->friend_number, call->bwc,

c-toxcore/toxav/rtp.c

Line 40 in 0542ffa

RTPSession *rtp_new(int payload_type, Messenger *m, uint32_t friendnumber,

c-toxcore/toxav/rtp.c

Line 122 in 0542ffa

rdata[0] = session->payload_type;

[zugz]

* Thursday, 2017-11-02 at 21:22 +0000 - Zoff <[email protected]>:

i think its used as lossless packet type

Aha, yes, I see. So 192 is really PACKET_ID_LOSSY_RANGE_START (defined in net_crypto.h).

[zoff99]

so it should be:

rtp_TypeAudio = (PACKET_ID_LOSSLESS_RANGE_START + PACKET_ID_LOSSLESS_RANGE_SIZE),

instead of:

rtp_TypeAudio = 192,

and why is this defined in net_crypto.h

toxcore/net_crypto.h:#define PACKET_ID_LOSSY_RANGE_START 192

and this in Messenger.h

toxcore/Messenger.h:#define PACKET_ID_LOSSLESS_RANGE_START 160
toxcore/Messenger.h:#define PACKET_ID_LOSSLESS_RANGE_SIZE 32

?

should be all in 1 place

[zugz]

* Friday, 2017-11-03 at 21:41 +0000 - Zoff <[email protected]>:

so it should be: ``` rtp_TypeAudio = (PACKET_ID_LOSSLESS_RANGE_START + PACKET_ID_LOSSLESS_RANGE_SIZE), ```

Wouldn't PACKET_ID_LOSSY_RANGE_START make more sense? It happens that they're equal, but there's no logical reason that there couldn't be another packet type between these lossless types and the lossy types.

and why is this define in net_crypto.h toxcore/net_crypto.h:#define PACKET_ID_LOSSY_RANGE_START 192 and this in Messenger.h toxcore/Messenger.h:#define PACKET_ID_LOSSLESS_RANGE_START 160 ? should be all in 1 place

Actually maybe not. They're not as similar as the names suggest. According to net_crypto.h:write_cryptpacket(), as far as net_crypto is concerned anything between CRYPTO_RESERVED_PACKETS and PACKET_ID_LOSSY_RANGE_START is lossless. So PACKET_ID_LOSSLESS_* really is specific to Messenger, while PACKET_ID_LOSSY_* is genuinely part of net_crypto. Maybe PACKET_ID_LOSSLESS_* should be renamed to PACKET_ID_OTHER_LOSSLESS_* or similar?

[zugz]

Actually, maybe it makes most sense to leave it as a hardcoded constant, and just add a comment that it must be in the PACKET_ID_LOSSY_* range? It happens to have been set as the first id in this range, but if hypothetically we wanted to decrease PACKET_ID_LOSSY_RANGE_START it would make sense to keep 192 as the audio id for backwards-compatibility.

[zoff99]

hmm. at least make if a defined constant, to make it easier to find / read.
i am trying to add lossless video packets, so thats how i stumbled upon this issue

[sudden6]

I agree with @zoff99 better make it a named constant.

[zugz]

* Wednesday, 2017-11-08 at 15:07 +0000 - sudden6 <[email protected]>:

I agree with @zoff99 better make it a named constant.

So do I. (Sorry, forgot to reply earlier.)