GNOME Bugzilla – Bug 652154
[mpegaudioparse] generates invalid timestamps
Last modified: 2011-07-14 21:12:11 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.
Created attachment 189508 [details] [review] Simple patch
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.
*** This bug has been marked as a duplicate of bug 652150 ***