GNOME Bugzilla – Bug 753037
rtpopusdepay: timestamp handling regression
Last modified: 2015-08-16 13:36:46 UTC
I'm getting Received invalid RTP payload warning and refcount error when trying to prase and depayload the following pcap file https://drive.google.com/file/d/0B12AhxvnYHrAQUpvWmozMm9mZG8 The same command is working without errors on gstreamer 1.2.4 and I can transcode to mp4 audio file. gst-launch-1.0 -v filesrc location=audio-video.pcap ! pcapparse src-port=1 ! "application/x-rtp, payload=111, encoding-name=OPUS" ! rtpopusdepay ! fakesink 0:00:00.138585766 18691 0x1d37f20 WARN rtpbasedepayload gstrtpbasedepayload.c:474:gst_rtp_base_depayload_handle_buffer:<rtpopusdepay0> warning: Received invalid RTP payload, dropping WARNING: from element /GstPipeline:pipeline0/GstRTPOpusDepay:rtpopusdepay0: Could not decode stream. Additional debug info: gstrtpbasedepayload.c(474): gst_rtp_base_depayload_handle_buffer (): /GstPipeline:pipeline0/GstRTPOpusDepay:rtpopusdepay0: Received invalid RTP payload, dropping (gst-launch-1.0:18691): GStreamer-CRITICAL **: gst_mini_object_unref: assertion 'mini_object->refcount > 0' failed 0:00:00.326023228 18691 0x1d37f20 WARN rtpbasedepayload gstrtpbasedepayload.c:474:gst_rtp_base_depayload_handle_buffer:<rtpopusdepay0> warning: Received invalid RTP payload, dropping WARNING: from element /GstPipeline:pipeline0/GstRTPOpusDepay:rtpopusdepay0: Could not decode stream. Additional debug info: gstrtpbasedepayload.c(474): gst_rtp_base_depayload_handle_buffer (): /GstPipeline:pipeline0/GstRTPOpusDepay:rtpopusdepay0: Received invalid RTP payload, dropping
I tried the exact same pipeline and can't reproduce the described issue with git master.
Did you try it with the attached pcap file? Are there differences between master and 1.5.2 that can explain why you can't reproduce it? I've found another report from someone trying to do the same: https://groups.google.com/forum/#!searchin/discuss-webrtc/rtpdump/discuss-webrtc/hJ-sOXl3z6M/kkjBOHhyMs8J
I upgraded to git master. Now I'm not getting the error I got with 1.5.2 but there is still a problem. When running this with gstreamer 1.2.4 I'm getting a 5.4MB mp4 audio file: gst-launch-1.0 -v filesrc location=audio-video.pcap ! pcapparse src-port=1 ! "application/x-rtp, payload=111" ! rtpopusdepay ! opusdec ! audioconvert ! audioresample ! voaacenc ! aacparse ! mp4mux ! filesink location=test.mp4 When running this with git master I'm getting a 1.8KB mp4 audio file: gst-launch-1.0 -v filesrc location=audio-video.pcap ! pcapparse src-port=1 ! "application/x-rtp, payload=111, encoding-name=OPUS" ! rtpopusdepay ! opusdec ! audioconvert ! audioresample ! voaacenc ! aacparse ! mp4mux ! filesink location=test.mp4 Why does git master give me a short audio file?
Looks like an issue in rtpopusdepay, it doesn't pass through the input timestamps properly, but just makes up 0-based timestamps, and opusdec then just clips the incoming buffers because they're before the segment start: gst-launch-1.0 -v filesrc location=audio-video.pcap ! pcapparse src-port=1 ! "application/x-rtp, payload=111, encoding-name=X-GST-OPUS-DRAFT-SPITTKA-00" ! identity silent=false ! rtpopusdepay ! fakesink silent=false -v
Could also be in the base class of course.
(In reply to Tim-Philipp Müller from comment #4) > Looks like an issue in rtpopusdepay, it doesn't pass through the input > timestamps properly, but just makes up 0-based timestamps, and opusdec then > just clips the incoming buffers because they're before the segment start: > Interesting, i thought I had address this issue through (that's one of the difference between 1.5.2 and master I think). Maybe it needed more work. commit 7c638e06ff7e26de8b429966e80058f98cb73073 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Fri Jul 10 12:49:01 2015 -0400 depayloader: Use input segment start When there is no clock_base provided, the start position is set to 0 instead of the original segment start value. This would break synchronization if start was not 0. https://bugzilla.gnome.org/show_bug.cgi?id=752228
VP8 has a similar issue. With git master I'm getting a 600 Bytes video file, with 1.2.4 I'm getting a 6.8MB video file. gst-launch-1.0 -v filesrc location=audio-video.pcap ! pcapparse src-port=3 ! "application/x-rtp, payload=100, encoding-name=VP8" ! rtpvp8depay ! vp8dec ! videoconvert ! x264enc ! h264parse ! mp4mux ! filesink location=test.mp4
Ok, pcapparse create a segment with segment start=399493:48:27.120205000, but pushes first buffer with timestamp 399493:48:27.049440000. This leads to first few buffer being out of segment and having TS set to none.
So I've been looking deeper, and in fact there is a series of issues. The one I expose in comment 8 is a bug introduce with buffer list. We use the last TS from the list rather then the first to create the segment. Though, a bigger issue I found, and it has been this way for a long time, is that rtph264depay sets timestamp base on the input, while the rtpbasedepayload set timestamp to running-time. This bug has been around for a long time, and never been an issues until now. Show story, from base class stand point "depayloader: Use input segment start" is wrong, but from h264depay standpoint it's right. Another subtle bugs is the duration is copied as is, while it fact it should have been scaled with the rate to be coherant with the running time. Normally, we'd calculate a end position, convert it to running time and compute back the duration. In fact, there is no clear reason why we waste CPU cycle converting these timestamp to running time except that stats property sends running time (which we could convert on demand). This property is also not thread safe, it should obviously take the the lock before reading all those values from the private class structure. A lot of issues in one that this bug raise up.
Why is rtph264depay doing magic things with the timestamps? So should we just revert your patch for 1.6 or do you want to try fixing all these problems together? :)
(In reply to Sebastian Dröge (slomo) from comment #10) > Why is rtph264depay doing magic things with the timestamps? > > > So should we just revert your patch for 1.6 or do you want to try fixing all > these problems together? :) We'll address these problem one by one I guess. Reverting is just flipping the coin as simple pipeline like this will stop working: ... ! x264enc ! rtph264pay ! rtph264depay ! ... My general opinion is that re-timestamping is wrong, and H264 brehaviour is what we should keep. And the fact duration has not been scaled is an additional sign that re-timestamping is wrong default behavior.
First issue (the wrong pcap segment) was trivial. commit 3757e507f37ce99cf67bfb746516bd67325182a2 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Aug 5 14:52:12 2015 -0400 pcapparse: Segment should start at base ts Instead we would use cur_ts which matches the last TS store in the buffer list. https://bugzilla.gnome.org/show_bug.cgi?id=753037
Created attachment 308812 [details] [review] basedepayloader: Don't re-timestamp with running-time There was a confusion, six depayloaders where passing through the timestamp while the base class was re-timestamping to running time. This inconstancy has been unnoticed has in most use cases the incoming segment is [0, inifnity] in which case timestamps are the same as running time. With DTS/PTS shifting added (to avoid negative values) and pcapparse sending a different segment this started being an issue.
With these two patches playing from pcap works again.
I think this makes sense but there might've been a reason why we convert buffer timestamps to running time. It seems wrong to do that though.
Attachment 308812 [details] pushed as 6ddab69 - basedepayloader: Don't re-timestamp with running-time
As it make sense, and fixes the regression, I'm merging it now. I'll still review all this during the hackfest and we can take actions accordingly.