type inconsistency in payloads

[kekkokk]

When the payload is only one, a number is returned. when there are multiple payloads a string is returned. I think we should have a consistency to be able to safely parse those fields

See the following example:

media: [
  {
    rtp: [Array],
    fmtp: [],
    type: 'audio',
    port: 1,
    protocol: 'UDP/TLS/RTP/SAVPF',
    payloads: 111, <----------------------------------------------------
    connection: [Object],
    rtcp: [Object],
    setup: 'actpass',
    mid: 'audio',
    direction: 'sendrecv',
    iceUfrag: '1ayI',
    icePwd: 'qKHZWyBE5t576gA675kDri',
    fingerprint: [Object],
    candidates: [Array],
    endOfCandidates: 'end-of-candidates',
    ssrcs: [Array],
    rtcpMux: 'rtcp-mux'
  },
  {
    rtp: [Array],
    fmtp: [Array],
    type: 'video',
    port: 1,
    protocol: 'UDP/TLS/RTP/SAVPF',
    payloads: '96 100', <----------------------------------------------------
    connection: [Object],
    rtcp: [Object],
    rtcpFb: [Array],
    setup: 'actpass',
    mid: 'video',
    direction: 'sendrecv',
    iceUfrag: '1ayI',
    icePwd: 'qKHZWyBE5t576gA675kDri',
    fingerprint: [Object],
    candidates: [Array],
    endOfCandidates: 'end-of-candidates',
    ssrcs: [Array],
    rtcpMux: 'rtcp-mux'
  }
]

[j1elo]

I was just looking around today, checking if this had been mentioned already.

My m line is like this:

m=audio 9 UDP/TLS/RTP/SAVPF 0

and because there is only one PayloadType, the toIntIfInt function wrongly converts it into an int when called from here.

I wonder if the format grammar for the m lines, which already is %s %d %s %s (so the last one is a %s thus denoting a string), could be used to ensure that the field is parsed and stored as a string and not an int.

Current workaround: force a string on those fields:

// Parse the SDP message text into an object.
const sdpObj = SdpTransform.parse(sdpStr);

// Force "payloads" to be a string field.
for (const media of sdpObj.media) {
  media.payloads = "" + media.payloads;
}

[hyavari]

I tried to reproduce these issues, but I couldn't. Hope you fixed it or if not appreciate it if you provide the SDP sample.

[j1elo]

This is trivial to reproduce. Here's a minimal sample that shows the issue, I hope it is useful for understanding the bug:

File main.js:

"use strict";

const SdpTransform = require("sdp-transform");
const util = require("node:util");

const sdp = `
m=audio 1 UDP 11 22
m=video 1 UDP 33
`;

const sdpObj = SdpTransform.parse(sdp);

console.log("sdpObj:", util.inspect(sdpObj));
console.log()
console.log("sdpObj.media[0].payloads:", util.inspect(sdpObj.media[0].payloads));
console.log("sdpObj.media[1].payloads:", util.inspect(sdpObj.media[1].payloads));
console.log()
console.log("typeof sdpObj.media[0].payloads:", typeof sdpObj.media[0].payloads);
console.log("typeof sdpObj.media[1].payloads:", typeof sdpObj.media[1].payloads);

Run commands:

mkdir /tmp/issue-94 && cd /tmp/issue-94
npm install sdp-transform
touch main.js # Copy contents from above.
node main.js

Obtained output:

sdpObj: {
  media: [
    {
      rtp: [],
      fmtp: [],
      type: 'audio',
      port: 1,
      protocol: 'UDP',
      payloads: '11 22'
    },
    {
      rtp: [],
      fmtp: [],
      type: 'video',
      port: 1,
      protocol: 'UDP',
      payloads: 33
    }
  ]
}

sdpObj.media[0].payloads: '11 22'
sdpObj.media[1].payloads: 33

typeof sdpObj.media[0].payloads: string
typeof sdpObj.media[1].payloads: number

Expected output:

[...]

sdpObj.media[0].payloads: '11 22'
sdpObj.media[1].payloads: '33'

typeof sdpObj.media[0].payloads: string
typeof sdpObj.media[1].payloads: string

[ibc]

It's not unexpected. If a value can be converted into a number, it is converted into a number. If not, it's becomes a string. That's how all things work in the parser. Whether that's super nice or not... that's another story.

[clux]

yeah, true. it's not great, but it is what it is. i do like the above suggestion of adding an optional type vector in the grammar to let it be smarter (or something along those lines).

[j1elo]

It's not unexpected. If a value can be converted into a number, it is converted into a number. If not, it's becomes a string. That's how all things work in the parser. Whether that's super nice or not... that's another story.

I meant "Expected output" as in "expected if there were no type inconsistency as reported in this issue".

It is for sure a bit strange that the field could be of one type or another, depending on the input given. And I agree it's not super nice, and consistency would be very much desirable.

[j1elo]

yeah, true. it's not great, but it is what it is. i do like the above suggestion of adding an optional type vector in the grammar to let it be smarter (or something along those lines).

The issue seems to be the assumption that toIntIfInt can be unconditionally applied to all values, while in fact, that's not always the case. Not sure if this happens to other fields, it might?

The ideal solution might be a map of specific type-per-field, enforced for each of them. But implementing all of that just for a single field... makes me feel like the kludge of manually forcing a string type just for that field might not be too bad, and would solve the issue. (But if more fields like that one start being discovered... yeah a map of types would be better).

Can any other instances of this issue happen?

[ibc]

Can any other instances of this issue happen?

Sure. a=mid and a=rid have string values, however I'm 200% sure that sdp-transform convert those values to number in case they are a=mid:0 or a=mid:1 (as usual).

[hyavari]

Sure. a=mid and a=rid have string values, however I'm 200% sure that sdp-transform convert those values to number in case they are a=mid:0 or a=mid:1 (as usual).

Yes, as you mentioned toIntIfInt would do it for all values. But I guess for now it would be good and the lib users can handle it.