After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 610265 - gstrtpmp4gdepay issuing a SIGFPE crashing an application
gstrtpmp4gdepay issuing a SIGFPE crashing an application
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.16
Other Linux
: Normal normal
: 0.10.19
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-02-17 14:46 UTC by Anders Skargren
Modified: 2010-02-24 10:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for gstrtpmp4gdepay to avoid a SIGFPE. (1.90 KB, application/octet-stream)
2010-02-17 14:46 UTC, Anders Skargren
  Details
proposed patch (1.22 KB, patch)
2010-02-22 11:27 UTC, Wim Taymans
none Details | Review

Description Anders Skargren 2010-02-17 14:46:54 UTC
Created attachment 154039 [details]
Patch for gstrtpmp4gdepay to avoid a SIGFPE.

Sometimes(very rarely, but it does happen) the RTP timestamp on the first 2 packets received can be the same, causing the 

/* get the number of packets by dividing with the duration */
diff /= rtpmp4gdepay->constantDuration;

to issue a SIGFPE.

Attaching an example fix that also recalculates the constantDuration for every received packet. It unfortunately doesnt seem like a good idea to base this value on only the first 2 packets due to that the delta can actually vary alot.

This patch needs work, but can at least highlight the issue.
Comment 1 Anders Skargren 2010-02-22 09:37:40 UTC
The fix is actually broken, it now has moved the FPE to when the difference of timestamps is 1 instead of 0. 

Can resend a better patch if you want.
Comment 2 Wim Taymans 2010-02-22 11:27:32 UTC
Created attachment 154376 [details] [review]
proposed patch

something like this patch then?
Comment 3 Sebastian Dröge (slomo) 2010-02-22 20:00:29 UTC
Looks like something that should be included in this release too
Comment 4 Anders Skargren 2010-02-23 08:01:08 UTC
(In reply to comment #2)
> something like this patch then?

Yes, it should solve the SIGFPE case.

That the constantDuration ends up being wrong doesnt seem to break anything; just make it do alot of correction.
But, shouldnt the constantDuration calculation be modified to be a bit better too?
Comment 5 Wim Taymans 2010-02-23 11:48:42 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > something like this patch then?
> 
> Yes, it should solve the SIGFPE case.
> 
> That the constantDuration ends up being wrong doesnt seem to break anything;
> just make it do alot of correction.
> But, shouldnt the constantDuration calculation be modified to be a bit better
> too?

I don't know what more you would add there. The constantDuration parameter should be given in caps. If there is no constantDuration, we create one by looking at the timestamp diff and the amount of packets we have. If all that fails, constantDuration remains 0 until some good rtp timestamp diff comes up. What other way of calculating the constantDuration do you have in mind?
Comment 6 Wim Taymans 2010-02-23 11:58:42 UTC
commit 3a09d334a09b98f3f0601b71ffaec7f7ba8290aa
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Feb 22 12:24:14 2010 +0100

    rtpmp4gdepay: avoid division by 0
    
    Avoid a division by 0 when no constantDuration was specified and when out two
    timestamps are equal.
    
    Fixes #610265
Comment 7 Anders Skargren 2010-02-24 10:33:08 UTC
(In reply to comment #5)

> I don't know what more you would add there. The constantDuration parameter
> should be given in caps. If there is no constantDuration, we create one by
> looking at the timestamp diff and the amount of packets we have. If all that
> fails, constantDuration remains 0 until some good rtp timestamp diff comes up.
> What other way of calculating the constantDuration do you have in mind?

In my tests the delta between the rtp timestamps differ alot. From 0 to at least 3000+.

In current code once you find a delta that is > 0 you use that as the constantDuration for the rest of the session, regardless of how bad that duration really reflects the true duration of the packets once the timestamps have stabilized.

Was just a possible improvement that this value might be updated as you go along to make it reflect the true duration better.