GNOME Bugzilla – Bug 780446
gst_rtp_buffer_ext_timestamp() math is incorrect for big jumps
Last modified: 2017-12-21 22:51:54 UTC
Created attachment 348572 [details] [review] gstrtpbuffer: fix math in gst_rtp_buffer_ext_timestamp() The math in gst_rtp_buffer_ext_timestamp() is not entirely correct. When doing big jumps it will produce strange results: guint64 ext = 0; gst_rtp_buffer_ext_timestamp (&ext, G_MAXINT32 + 1); -> ext should now be G_MAXINT32 + 1 -> before this change it would be G_MAXUINT32 + G_MAXINT32 + 2 guint64 ext = 1087500; gst_rtp_buffer_ext_timestamp (&ext, 24); > ext should now be G_MAXUINT32 + 1 + 24 -> before this change it would be just 24
Created attachment 348573 [details] [review] tests/libs/rtp: add unit test for gst_rtp_buffer_ext_timestamp()
Review of attachment 348573 [details] [review]: looks good
Review of attachment 348573 [details] [review]: Actually, can you add a test that does ext = -1; gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 gst_rtp_buffer_ext_timestamp(&ext, 2) == 2 gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32) == G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 + G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, 2) == 2 + G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 + G_MAXUINT32 I'm worried that by removing the G_MAXINT32 comparision with the diff, you sometimes add an extra wrap-around?
(In reply to Olivier Crête from comment #3) > gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 > gst_rtp_buffer_ext_timestamp(&ext, 2) == 2 > gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 Hmm, this means that going backwards by 1 is acceptable? The documentation says it will always be increasing, so yes, in this example I would expect the last call to equal (G_MAXUINT32 + 1) + 1 instead of just 1.
Yes, the packets can be re-ordered.. The idea is that we should catch the wrap-around, but reasonable differences should be ignored. Maybe the difference should be set at G_MAXINT/2 or /4, but I think the original code was correct.
(In reply to Olivier Crête from comment #5) > Yes, the packets can be re-ordered.. The idea is that we should catch the > wrap-around, but reasonable differences should be ignored. Did you read the doc ? * This function makes sure that the returned value is a constantly increasing * value even in the case where there is a timestamp wraparound. Reading the code of this function, clearly it was in no way meant to support consecutive backward timestamp. Those currently cause a big forward jump if I read properly. Because of the doc, I would have expected the exttimestamp to be returned, and left unmofied if timestamp is detected as going backward. That of course assumes we can always detect it. Can we hold on a bit and not rush this change right before a release ?
Comment on attachment 348573 [details] [review] tests/libs/rtp: add unit test for gst_rtp_buffer_ext_timestamp() Sorry.
After a little chat here and looking at how this is used by rtpjitterbuffer, we think that yes, it can go back by a little. To also account for the case where the first packet arrives second, but with a timestamp of 0, we probably probably need to have gst_rtp_buffer_ext_timestamp(-1, 0) == G_MAXUINT32, so that in the next time, if we have gst_rtp_buffer_ext_timestamp(G_MAXUINT32, G_MAXUINT32 - 2), it retuerns G_MAXUINT32-2 instead of something invalid. So there should be a test like: ext = -1; gst_rtp_buffer_ext_timestamp(&ext, 1) == G_MAXUINT32 + 2 gst_rtp_buffer_ext_timestamp(&ext, 2) == G_MAXUINT32 + 3 gst_rtp_buffer_ext_timestamp(&ext, 0) == G_MAXUINT32 + 1 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32) == G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 - 10 ) == G_MAXUINT32 - 10 gst_rtp_buffer_ext_timestamp(&ext, 3) == G_MAXUINT32 + 4 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 4 ) == G_MAXUINT32 + 1 + G_MAXUINT32/4 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 2 ) == G_MAXUINT32 + 1 + G_MAXUINT32/2 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 3 ) == G_MAXUINT32 + 1 + G_MAXUINT32/3 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 ) == 2*G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, 0 ) == 2*G_MAXUINT32 + 1 gst_rtp_buffer_ext_timestamp(&ext, 2 ) == 2*G_MAXUINT32 + 3 gst_rtp_buffer_ext_timestamp(&ext, 0 ) == 2*G_MAXUINT32 + 1 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 4 ) == 2*G_MAXUINT32 + 1 + G_MAXUINT32/4 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 2 ) == 2*G_MAXUINT32 + 1 + G_MAXUINT32/2 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 / 3 ) == 2*G_MAXUINT32 + 1 + G_MAXUINT32/3 gst_rtp_buffer_ext_timestamp(&ext, G_MAXUINT32 ) == 3*G_MAXUINT32 gst_rtp_buffer_ext_timestamp(&ext, 0 ) == 3*G_MAXUINT32 + 1 gst_rtp_buffer_ext_timestamp(&ext, 2 ) == 3*G_MAXUINT32 + 3 gst_rtp_buffer_ext_timestamp(&ext, 0 ) == 3*G_MAXUINT32 + 1
Yes what Nicolas says is true, and it seems to me like the function is doing exactly what it is supposed to do? What is the problem here? If there's a timestamp jump by more than half the possible range, it is considered a wraparound and handled accordingly. And timestamps are supposed to never go backwards.
The problem that was pointed to me is that the jitterbuffer seems to use that function before reordering, and do expect the results to go backward on un-ordered input. Right now, backward input timestamp can cause large forward jump. (I have not personally verified this)
The original issue that was discovered with this function is that when ext is already G_MAXUINT32 + 1 and you try to set G_MAXUINT32 for the timestamp, it awkwardly returns 3*G_MAXUINT32 + 2. This is wrong in both cases (allowing and not allowing timestamps to go backwards). If we allow them to go backwards, it should return G_MAXUINT32. If we don't allow them, it should return 2*G_MAXUINT32 + 1. For the fix, I considered what the doc says, i.e. timestamps are NOT allowed to go backwards, so my patch simply implements this behavior. Instead of comparing with G_MAXINT32 (which basically allows going backwards by at most G_MAXINT32 time units, if the timestamp is not wrapping around), my patch simply checks if the timestamp is smaller than the previous value, which means it went upwards and wrapped around.
(In reply to George Kiagiadakis from comment #11) > The original issue that was discovered with this function is that when ext > is already G_MAXUINT32 + 1 and you try to set G_MAXUINT32 for the timestamp, > it awkwardly returns 3*G_MAXUINT32 + 2. ...this is basically the same thing that I check in the very first lines of the unit test, with all variables being minus G_MAXUINT32+1: + result = gst_rtp_buffer_ext_timestamp (&exttimestamp, 0); + fail_unless_equals_uint64 (result, 0); + result = gst_rtp_buffer_ext_timestamp (&exttimestamp, + ((guint64) G_MAXINT32) + 1); + fail_unless_equals_uint64 (result, ((guint64) G_MAXINT32) + 1); If you run this without applying the patch, the last check will fail and 'result' will equal G_MAXUINT32 + G_MAXINT32 + 2.
I'm not sure why a sequence like: G_MAXUINT32 + 1 -> G_MAXUINT32 + 1 G_MAXUINT32 -> 3 * G_MAXUINT32 + 2. Is more valid then: G_MAXUINT32 + 1 -> G_MAXUINT32 + 1 G_MAXUINT32 -> 2 * G_MAXUINT32 + 2. Both are going forward, so that's compliant with the doc. In practice, there was no wraparound, so maybe returning G_MAXUINT32 + 1 would be valid. Now, the theoretical aspect of this issue has no importance. It's a completely useless to try and fix a behaviour for another just because it seems better. A study of the function usage is the only useful rational to change this function. rtpjitterbuffer is the main user, and I think studying that aspect, and verifying if it may not be using it the way the function was designed is important. From there, we will be able to construct a unit test that covers real usages, and potentially fix a real bug. The best test, would be to trigger a bug from the jitter buffer cause by the current behaviour.
Hello, it seems that George and I have come across the same issue. I opened a new one few days ago: https://bugzilla.gnome.org/show_bug.cgi?id=783443 From my point of view, I am pretty sure that we should provide an utility to deal with backward RTP timestamp to be used in places where missordered packets are handled. So, there are typically 2 alternatives: a1 - Create a new function to provide this functionality. a2 - Update gst_rtp_buffer_ext_timestamp implementation. In relation to (a2) the main problems are: p1 - doc says "This function makes sure that the returned value is a onstantly increasing value even in the case where there is a timestamp wraparound." p2 - Could this break currently usages of this function? Or just would fix them? Regarding (p1) is not really true because this happens: ext = -1; gst_rtp_buffer_ext_timestamp(&ext, 2) == 2 gst_rtp_buffer_ext_timestamp(&ext, 1) == 1 So what I think is that the doc is not properly written and I suppose that what it wants to say is something like "This function takes into account timestamp wraparound making sure that the returned value is increased." (actually this is what it does). In relation to (p2), I think that taking into account backward timestamps and performing unwrapping will fix the function. In conclusion, I would update the implementation of gst_rtp_buffer_ext_timestamp and the doc. What do you think?
I think you missed that the current implementation does not support backward wraparound (which is really important if you want to use it for disordered packet). Supporting both direction reduces the maximum distance you can tolerate between two timestamp, though I haven't referred to the spec to see what's should be tolerated (left as an exersize). (In reply to Miguel París Díaz from comment #14) > In conclusion, I would update the implementation of > gst_rtp_buffer_ext_timestamp and the doc. > What do you think? I think it's per use case. From jitterbuffer perspective, it should in my opinion reorder first, and then set timestamp. The monotonic behaviour of gst_rtp_buffer_ext_timestamp () need fixing, so if the timestamp are effectively backward after reordering, we just force them forward.
Hmm, second though, are these timestamp PTS ? If so, then yes, a function that accept backward is important (think b-frames, though completly broken atm).
Hello @stormer. gst_rtp_buffer_ext_timestamp() supports backward, because of that I said that the doc does NOT match to the implementation. The problem that I suffered is that it does NOT support unwraparound. Because of that I proposed a change in both doc and implementation in the next patch: https://bugzilla.gnome.org/review?bug=783443&attachment=353321 I have been using it and it works for me ;).
I ended up merging the patch from #783443 which I think also fixs this problem.