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 724213 - rtph264pay: shouldn't update time for sending SPS and PPS if we failed to send SPS or PPS
rtph264pay: shouldn't update time for sending SPS and PPS if we failed to sen...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-02-12 08:39 UTC by Göran Jönsson
Modified: 2014-02-25 11:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.91 KB, patch)
2014-02-12 08:39 UTC, Göran Jönsson
none Details | Review
New version patch (1.96 KB, patch)
2014-02-17 08:39 UTC, Göran Jönsson
committed Details | Review

Description Göran Jönsson 2014-02-12 08:39:34 UTC
Created attachment 268885 [details] [review]
patch

In the file gstrtph264pay.c  func  

static GstFlowReturn
gst_rtp_h264_pay_send_sps_pps (GstRTPBasePayload * basepayload,
    GstRtpH264Pay * rtph264pay, GstClockTime dts, GstClockTime pts)


This function will update the time for last sent sps and pps even if fail to send 
sps or pps.

Attached patch.
Comment 1 Tim-Philipp Müller 2014-02-14 13:17:54 UTC
Looks sensible IMHO, although I'm not sure I understand the explanation provided in the commit message. In what scenario do you get a non-OK flow return here and why and how is it related to rtsp-server sending the config out of band?

Some style nitpicks (sorry):

 - typo: sucsess -> success (but maybe rename variable to "sent_all_sps_pps")

 - move the variable declaration above guint i; (nicer alignment)
Comment 2 Göran Jönsson 2014-02-17 08:39:06 UTC
Created attachment 269362 [details] [review]
New version patch
Comment 3 Göran Jönsson 2014-02-17 08:40:35 UTC
What happens without this patch is that payloader start working already when receiving DESCRIBE but there is no transports so it trys to send sps and pps but that fail (GST_FLOW_FLUSHING) . 
But the time for last sent sps and pps will be set.

So when PLAY arrives and the first intraframe is to be sent there is no sps and pps sent due to that time since last sps pps is less than spspps_interval .
Comment 4 Tim-Philipp Müller 2014-02-25 10:59:42 UTC
Thanks, pushed this. I think it makes sense irrespective of whether one might want to do things differently in gst-rtsp-server too.

commit 53ffd9e1cab086f334f94597973fdb0fcb175adc
Author: Göran Jönsson <goranjn@axis.com>
Date:   Tue Feb 11 12:41:29 2014 +0100

    rtph264pay: only update last_spspps time if all sps/pps got sent successfully
    
    This fixes an issue with gst-rtsp-server where no sps and pps are
    sent for the first intra frame, because the payloader starts working
    already when receiving DESCRIBE but there is no transports so it tries
    to send sps and pps, but that fails with a FLUSHING flow. But the time
    for last sent sps and pps would still be set, so when PLAY arrives and
    the first intra frame is to be sent there is no sps and pps sent due to
    that time since last sps pps is less than spspps_interval.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=724213

----

This whole config-interval thing is a bit weird anyway though - why don't we just pack SPS/PPS in front of every I-frame we send out and be done with it? It's just a couple of bytes..