[Bug 652154] New: [mpegaudioparse] generates invalid timestamps

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=652154
  GStreamer | gst-plugins-ugly | git

           Summary: [mpegaudioparse] generates invalid timestamps
    Classification: Platform
           Product: GStreamer
           Version: git
        OS/Version: All
            Status: UNCONFIRMED
          Severity: normal
          Priority: Normal
         Component: gst-plugins-ugly
        AssignedTo: gstreamer-bugs at lists.freedesktop.org
        ReportedBy: matej.knopp at gmail.com
         QAContact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---


I'm feeding gstmpegaudioparse (from plugins ugly) output of lamep3enc (with
patch from 652150, 128kbit/s 48000hz) because I need the output aligned to MP3
frames. It goes as follows:
   ACTION    TS             DURATION        BUFFER SIZE
>> Receive   0.000000000    0.019750000     316
>> Receive   0.019750000    0.022875000     366
>> Receive   0.042625000    0.022187500     355
-- Push      0.000000000    0.024000000     384
-- Push      0.024000000    0.024000000     384
>> Receive   0.064812500    0.045437500     727
-- Push      0.048000000    0.024000000     384
-- Push      0.072000000    0.024000000     384
>> Receive   0.110250000    0.023625000     378
-- Push      0.096000000    0.024000000     384
>> Receive   0.133875000    0.022875000     366
-- Push      0.120000000    0.024000000     384
>> Receive   0.156750000    0.046562500     745
-- Push      0.144000000    0.024000000     384
-- Push      0.168000000    0.024000000     384
>> Receive   0.203312500    0.023250000     372
-- Push      0.192000000    0.024000000     384
>> Receive   0.226562500    0.023687500     379
-- Push      0.216000000    0.024000000     384
>> Receive:  0.250250000    0.046500000     744
-- Push      0.240000000    0.024000000     384
-- Push      0.250250000    0.024000000     384
            ~~~~~~~~~~~~~
Note the last line - mpegaudioparse generates incorrect timestamp resulting in
the entire audiostream garbled. The following code snippet causes that

  /* Check if we have a pending timestamp from an incoming buffer to apply
   * here */
  if (GST_CLOCK_TIME_IS_VALID (mp3parse->pending_ts)) {
    if (mp3parse->tracked_offset >= mp3parse->pending_offset) {
      /* If the incoming timestamp differs from our expected by more than 
       * half a frame, then take it instead of our calculated timestamp.
       * This avoids creating imperfect streams just because of 
       * quantization in the container timestamping */
      GstClockTimeDiff diff = mp3parse->next_ts - mp3parse->pending_ts;
      GstClockTimeDiff thresh = GST_BUFFER_DURATION (outbuf) / 2;

      if (diff < -thresh || diff > thresh) {
        GST_DEBUG_OBJECT (mp3parse, "Updating next_ts from %" GST_TIME_FORMAT
            " to pending ts %" GST_TIME_FORMAT
            " at offset %" G_GINT64_FORMAT " (pending offset was %"
            G_GINT64_FORMAT ")", GST_TIME_ARGS (mp3parse->next_ts),
            GST_TIME_ARGS (mp3parse->pending_ts), mp3parse->tracked_offset,
            mp3parse->pending_offset);
        mp3parse->next_ts = mp3parse->pending_ts;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      }
      mp3parse->pending_ts = GST_CLOCK_TIME_NONE;        
    }      
  }

IMHO the output timestamp should only be updated to pending_ts if no buffer has
been pushed in this iteration yet. i.e.

  /* Check if we have a pending timestamp from an incoming buffer to apply
   * here */
  if (GST_CLOCK_TIME_IS_VALID (mp3parse->pending_ts)) {
    if (mp3parse->tracked_offset >= mp3parse->pending_offset) {
      /* If the incoming timestamp differs from our expected by more than 
       * half a frame, then take it instead of our calculated timestamp.
       * This avoids creating imperfect streams just because of 
       * quantization in the container timestamping */
      GstClockTimeDiff diff = mp3parse->next_ts - mp3parse->pending_ts;
      GstClockTimeDiff thresh = GST_BUFFER_DURATION (outbuf) / 2;

      if (diff < -thresh || diff > thresh) {
        GST_DEBUG_OBJECT (mp3parse, "Updating next_ts from %" GST_TIME_FORMAT
            " to pending ts %" GST_TIME_FORMAT
            " at offset %" G_GINT64_FORMAT " (pending offset was %"
            G_GINT64_FORMAT ")", GST_TIME_ARGS (mp3parse->next_ts),
            GST_TIME_ARGS (mp3parse->pending_ts), mp3parse->tracked_offset,
            mp3parse->pending_offset);
        mp3parse->next_ts = mp3parse->pending_ts;
      }
    }      
    mp3parse->pending_ts = GST_CLOCK_TIME_NONE;        
  }

(mp3parse->pending_ts = GST_CLOCK_TIME_NONE; moved outside the inner if)

This seems to be working for me but there might be something I'm overlooking.
Patch attached.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.