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 572854 - rtph264pay failed to incorporate PPS for sprop-parameters-set
rtph264pay failed to incorporate PPS for sprop-parameters-set
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.11
Other All
: Normal minor
: 0.10.14
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-02-23 14:11 UTC by Wai-Ming Ho
Modified: 2009-02-23 14:45 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Wai-Ming Ho 2009-02-23 14:11:06 UTC
Please describe the problem:
When a h264 PPS NAL is sent after a SPS NAL(both NALs are sent as individual discret buffers), the payloader failed to incorporate the PPS into the caps' sprop-parameters-set value. This is due to an extra loop inside gst_rtp_h264_pay_decode_nal (Looping is not necessary because this is already taken care of by handle_buffer()). The data being passed to gst_rtp_h264_pay_decode_nal is already one complete NAL unit. All that is necessary is to test the first byte for a NAL type 7 or 8 to identify SPS/PPS NALs.

Steps to reproduce:
1. Send SPS, PPS and h264 coded slice NAL units as individual buffers
2. Observe GST_DEBUG traces. The PPS is never parsed.
3. Get the negotiated caps. the sprop-parameters-set does not contain the PPS part (no comma (,) separating two base64 strings. Only one base64 string that is the SPS).


Actual results:
Please refer to the GST_DEBUG traces emitted or do a gst_pad_get_negotiated_caps() on the payloader pad after sending both SPS and PPS.

Expected results:
The sprop-parameters-set value in the caps should contain 2 base64 strings, separated by a comma (,)

Does this happen every time?
Yes

Other information:
The patch for the bug is to replace gst_rtp_h264_pay_decode_nal() completely by the function below:

static void
gst_rtp_h264_pay_decode_nal (GstRtpH264Pay * payloader,
    guint8 * data, guint size, gboolean * updated)
{
  guint8 *sps = NULL, *pps = NULL;
  guint sps_len = 0, pps_len = 0;
  guint8 header, type;
  guint len;

  /* default is no update */
  *updated = FALSE;

  GST_DEBUG ("NAL payload len=%u", size);
    
  len = size;
  header = data[0];
  type = header & 0x1f;

  /* keep sps & pps separately so that we can update either one 
   * independently */
  if (SPS_TYPE_ID == type) {
    /* encode the entire SPS NAL in base64 */
    GST_DEBUG ("Found SPS %x %x %x Len=%u", (header >> 7),
	       (header >> 5) & 3, type, len);
    
    sps = data;
    sps_len = len;
  } else if (PPS_TYPE_ID == type) {
    /* encoder the entire PPS NAL in base64 */
    GST_DEBUG ("Found PPS %x %x %x Len = %u",
	       (header >> 7), (header >> 5) & 3, type, len);
    
    pps = data;
    pps_len = len;
  } else {
    GST_DEBUG ("NAL: %x %x %x Len = %u", (header >> 7),
	       (header >> 5) & 3, type, len);
  }
  

  /* If we encountered an SPS and/or a PPS, check if it's the
   * same as the one we have. If not, update our version and
   * set *updated to TRUE
   */
  if (sps_len > 0) {
    if ((payloader->sps_len != sps_len)
	|| !is_nal_equal (payloader->sps, sps, sps_len)) {
      payloader->profile = (sps[1] << 16) + (sps[2] << 8) + sps[3];
      
      GST_DEBUG ("Profile level IDC = %06x", payloader->profile);
      
      if (payloader->sps_len)
	g_free (payloader->sps);
      
      payloader->sps = sps_len ? g_new (guint8, sps_len) : NULL;
      memcpy (payloader->sps, sps, sps_len);
      payloader->sps_len = sps_len;
      *updated = TRUE;
    }
  }
  
  if (pps_len > 0) {
    if ((payloader->pps_len != pps_len)
	|| !is_nal_equal (payloader->pps, pps, pps_len)) {
      if (payloader->pps_len)
	g_free (payloader->pps);
      
      payloader->pps = pps_len ? g_new (guint8, pps_len) : NULL;
      memcpy (payloader->pps, pps, pps_len);
      payloader->pps_len = pps_len;
      *updated = TRUE;
    }
  }
}
Comment 1 Wim Taymans 2009-02-23 14:45:42 UTC
commit aeee52be051d7d73cd565e7eaf9b49b84a00c8a2
Author: Wai-Ming Ho <waiming at ailuropoda dot net>
Date:   Mon Feb 23 15:43:51 2009 +0100

    Always add PPS to the sprop-parameters-set
    
    Rework the parsing code that under certain circumstances dropped the PPS from
    the sprop-parameters-set.
    Fixes #572854.