libsdptransform icon indicating copy to clipboard operation
libsdptransform copied to clipboard

Allow ptime SDP element to accept float type values

Open Mitchell-Roy opened this issue 5 years ago • 8 comments

Mitchell-Roy avatar Mar 31 '20 17:03 Mitchell-Roy

@ibc Here is a proposed change for ptime element in sdp. Thanks!

Mitchell-Roy avatar Mar 31 '20 17:03 Mitchell-Roy

Thanks :)

BTW have you run tests? may be you could modify some existing test SDP by adding a ptime with float value and then check in in parse.test.cpp?

ibc avatar Mar 31 '20 17:03 ibc

Yes, haven't got to that yet, will try to add this in as soon as I get a chance :)

Mitchell-Roy avatar Mar 31 '20 17:03 Mitchell-Roy

Note tha Travis CI is failing due to this change: https://travis-ci.com/github/ibc/libsdptransform/jobs/310698468#L691

Somehow I expected this. The problem is that this PR forces ptime to be printed as float even if it was an integer. Example: a=ptime:20.0.

IMHO it's critical that we represent it as an integer ( a=ptime:20) if it does not have decimals. I'm not sure all RTP stacks will like ptime with a float number so let's avoid float representation when it's not needed. I don't know right now how to do that.

BTW: why this change?

- "ptime:%d"
+ "ptime:%s"

I understand it cannot be %d if it contains a float, but why %s?

ibc avatar Mar 31 '20 17:03 ibc

Yeah, I noticed the writer will force a decimal place as well. I wanted to make it something similar to the "framerate" element as it handles similar inputs so I used that as a point of reference..

And yes, ideally we'd want to not have a floating point representation if not needed. I'm not quite sure how one would go about ensuring that atm :)

Mitchell-Roy avatar Mar 31 '20 18:03 Mitchell-Roy

You are right, sorry. Maybe the same issue happens in a=framerate. In fact, the tests just have decimal framerates, that's why it works :)

ibc avatar Mar 31 '20 18:03 ibc

Note that, instead of format you can define a formatFunc (there are some attributes in grammar.cpp using that).

ibc avatar Mar 31 '20 18:03 ibc

In such a formatFunc you may use some helper to check if it's a float or just an integer. In parser.cpp there we already use:

#include <sstream>   // std::istringstream
#include <ios>       // std::noskipws

bool isFloat(const std::string& str)
{
	std::istringstream iss(str);
	float f;

	iss >> std::noskipws >> f;

	return iss.eof() && !iss.fail();
}

ibc avatar Mar 31 '20 18:03 ibc