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 652154 - [mpegaudioparse] generates invalid timestamps
[mpegaudioparse] generates invalid timestamps
Status: RESOLVED DUPLICATE of bug 652150
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other All
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-06-08 23:48 UTC by Matej Knopp
Modified: 2011-07-14 21:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Simple patch (566 bytes, patch)
2011-06-08 23:49 UTC, Matej Knopp
none Details | Review

Description Matej Knopp 2011-06-08 23:48:38 UTC
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.
Comment 1 Matej Knopp 2011-06-08 23:49:50 UTC
Created attachment 189508 [details] [review]
Simple patch
Comment 2 David Schleef 2011-07-04 19:27:25 UTC
It looks like lamemp3enc is pushing buffers with the wrong timestamps.  The buffer timestamps should correspond to the beginning of the first frame in the buffer.  It appears we've been doing this consistently wrong, so it might be necessary to have backward compatibility rather than Doing It Right.  In any case, the correct solution is to make lamemp3enc properly frame output data, which eliminates any confusion.
Comment 3 David Schleef 2011-07-14 21:12:11 UTC

*** This bug has been marked as a duplicate of bug 652150 ***