Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTPUtils, inaccurate difference of sequence numbers #291

Open
flurbi opened this issue Mar 24, 2017 · 3 comments
Open

RTPUtils, inaccurate difference of sequence numbers #291

flurbi opened this issue Mar 24, 2017 · 3 comments
Labels

Comments

@flurbi
Copy link

flurbi commented Mar 24, 2017

org.jitsi.util.RTPUtils.sequenceNumberDiff(...)
    line 34
    incorrect: diff > 1<<15
    problem  : 1<<15 is not in the short range (-2^15 .. 2^15 - 1)
    correct  : diff >= 1<<15
@bgrozev
Copy link
Member

bgrozev commented Mar 30, 2017

Well, if the difference between two sequence numbers is exactly 2^15, then we don't know which one comes first. I think both 2^15 and -2^15 are reasonable values to return, and I don't know of a requirement for the return value to be in the short range.

@flurbi
Copy link
Author

flurbi commented Mar 31, 2017

Casted back to an short leads to the same result anyway (2^15 -> -2^15). I do not know if its matter, but since the two inputs are in (-2^15 .. 2^15 - 1), I thought it would be more consistent if the computed value lays in the same range (unique representative modulo 2^16). Note that the docu says "modulo 2^16", without specifying the range (which is not ambiguous in C with uint16_t). That the inputs are also "modulo 2^16" is not explicitly stated neither, but otherwise the code does not make much sense.

I suggest this simpler implementation, which I had in mind as I opened this ticket:

    public static short sequenceNumberDiff_loic(short a, short b) {
        return (short) (a - b);
    }

Then it is clear what "modulo 2^16" means. It is (up to the handling of 2^15) equivalent to the original code, shorter and possibly faster. I even question if a function is needed for that.

Up to you judgment to adapt the code and/or the documentation, it is not critical in my opinion.

@bgrozev
Copy link
Member

bgrozev commented Mar 31, 2017

Good point, I'll address it when I get a chance.

@bgrozev bgrozev added the bug label Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants