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 516811 - [mp3parse] immediate EOS when playing back AVIs
[mp3parse] immediate EOS when playing back AVIs
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal blocker
: 0.10.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-02-16 08:10 UTC by Tim-Philipp Müller
Modified: 2008-02-18 10:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mp3parse-timestamps.diff (1.38 KB, patch)
2008-02-16 11:44 UTC, Sebastian Dröge (slomo)
none Details | Review
mp3parse-timestamps.diff (2.76 KB, patch)
2008-02-17 04:59 UTC, Sebastian Dröge (slomo)
none Details | Review
mp3parse-timestamps.diff (2.76 KB, patch)
2008-02-17 09:12 UTC, Sebastian Dröge (slomo)
none Details | Review
mp3parse-timestamps.diff (3.42 KB, patch)
2008-02-17 09:13 UTC, Sebastian Dröge (slomo)
none Details | Review
mp3parse-invalid-timestamps.diff (1.73 KB, patch)
2008-02-18 05:12 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Tim-Philipp Müller 2008-02-16 08:10:33 UTC
This commit:

 2008-02-14  Sebastian Dröge  <slomo@circular-chaos.org>

        * gst/mpegaudioparse/gstmpegaudioparse.c:
        (gst_mp3parse_emit_frame):
        Return GST_FLOW_UNEXPECTED if we get data that is after our
        configured segment. This makes upstream go EOS immediately instead
        of sending us the complete stream. Also improve debugging a bit.

breaks movie playback (AVI) for me. The films more or less EOS immediately (it seems to preroll, but then stops right away after a blip). Reverting the commit makes things work again.
Comment 1 Sebastian Dröge (slomo) 2008-02-16 11:29:39 UTC
Weird, could you attach a debug log with GST_DEBUG=mp3parse:5 ? The only reason why this could happen IMHO are either wrong timestamps by avidemux or a wrong NEWSEGMENT by avidemux.
Comment 2 Sebastian Dröge (slomo) 2008-02-16 11:42:30 UTC
Ok, I can reproduce it... it's caused by the output buffer having an invalid timestamp (i.e. -1). Should get some checks where the UNEXPECTED is returned.

Patch will come later if nobody is faster than me ;)
Comment 3 Sebastian Dröge (slomo) 2008-02-16 11:44:43 UTC
Created attachment 105375 [details] [review]
mp3parse-timestamps.diff

Ok, there it is already... any thoughts? Fixes it for me but why are the timestamps -1 at all? Is this another bug?
Comment 4 Sebastian Dröge (slomo) 2008-02-16 11:54:34 UTC
Also there seem to be some buffers missing in the beginning... well, tonight I've some time for this maybe :)
Comment 5 Sebastian Dröge (slomo) 2008-02-16 11:55:10 UTC
It's about ~0.04 seconds btw... nothing too bad. Just 2 or 3 mp3 frames.
Comment 6 Tim-Philipp Müller 2008-02-16 12:03:22 UTC
Or maybe we should just back this out for the release? It was only an optimisation anyway, wasn't it? The fact that no one has run into this even though the patch has been in CVS for almost 48 hours makes me slightly uncomfortable and makes me want to stick to the previous revision in the hope that it's slightly better tested. (But then I don't have a lot of faith in mpegaudioparse doing the right thing in general anyway, so feel free to ignore me.)


> Ok, there it is already... any thoughts? Fixes it for me

Well, it fixes the immediate-EOS problem for me, but I still have problems when seeking: no sound, things stop after a few seconds of video without sound after a seek; some seeks make it work again though. Setting mpegaudioparse to RANK_NONE fixes these issues for me.


> timestamps -1 at all? Is this another bug?

I don't think so. Not every packet needs to be timestamped. Decoders are expected to interpolate.
Comment 7 Sebastian Dröge (slomo) 2008-02-16 13:47:03 UTC
Does seeking work again if you revert the UNEXPECTED patch? Or do you suggest to give mp3parse RANK_NONE for the release instead?

I'll look into the seeking bug and everything later but for the release we should probably revert.
Comment 8 Tim-Philipp Müller 2008-02-16 14:10:44 UTC
> Does seeking work again if you revert the UNEXPECTED patch?

The seeking problem seems to only occur with the patch in comment #3, but not with gstmpegaudioparse.c rev. 1.86 or rev. 1.85 (I only did some very quick tests though; the problem happens when seeking backwards).

> Or do you suggest to give mp3parse RANK_NONE for the release instead?

Well, that would be my preferred option, but would probably upset people who need accurate seeking, segment seeks and things like that. Tough call.
Comment 9 Jan Schmidt 2008-02-16 14:21:54 UTC
I say revert the patch.
Comment 10 Sebastian Dröge (slomo) 2008-02-16 15:37:42 UTC
Me too, RANK_NONE is not necessary IMHO. We use mpegaudioparse since 17 Nov 2007 with the version from that day in Debian without problems. And 0.10.6.3 is in Ubuntu hardy now without any bugreports too.
Comment 11 Sebastian Dröge (slomo) 2008-02-17 04:11:19 UTC
Hm, also just reverting this patch might not be good enough. It will simply drop all buffers that have no timestamp, not good either ;)

So what about reverting and then, if a buffer has no valid timestamp or duration push it in any case?
Comment 12 Sebastian Dröge (slomo) 2008-02-17 04:59:40 UTC
Created attachment 105416 [details] [review]
mp3parse-timestamps.diff

Ok, some news :)

First of all, the timestamp is invalid because of a bug in mp3parse, it should instead be the pending_ts in this case. Timestamps can still be invalid if upstream did not supply timestamps though...

And then, no buffers get lost, I just misread the log.

Attached patch fixes all issues for me it seems... I get proper timestamps, no UNEXPECTED unless it's correct and seeking works fine too.

Tim?
Comment 13 Sebastian Dröge (slomo) 2008-02-17 09:12:05 UTC
Created attachment 105420 [details] [review]
mp3parse-timestamps.diff

One of the problems that was there was, that, when resync==TRUE, we don't/can't use the timestamp of upstream in several situations.

This is, because we want the header of the second mpeg frame there, which gave us a new timestamp by upstream. So we had no timestamp for the first frame after a newsegment and the first frame at all if we can't query upstream (because upstream gave us no offset).

This patch probably fixes all problems now... if not I'd like to see a GST_DEBUG=mp3parse:5 debug log and an explanation what failed (no sound after the n-th seek, etc) :)
Comment 14 Sebastian Dröge (slomo) 2008-02-17 09:13:00 UTC
Created attachment 105421 [details] [review]
mp3parse-timestamps.diff

doh, now the patch for real.
Comment 15 Jan Schmidt 2008-02-17 13:10:24 UTC
I'm really uncomfortable with approving this patch 1 day before the release, but I'm not sure what else to do. The patch is too complex to be obviously correct.

I'm still more inclined to just revert the 'optimisation' - do you have any examples where the 'buffer dropped because no timestamp' situation is actually a problem? It won't happen with plain mp3 files, because those have an offset, right?

The alternative is to apply this patch and everyone has to test it thoroughly within the next 24 hours... but that sort of assumes that such testing won't actually encounter any bugs. If it does, we won't have time to fix them without delaying the release.


Comment 16 Sebastian Dröge (slomo) 2008-02-17 17:42:50 UTC
Ok, what about reverting and adding the checks for invalid timestamps, duration, offset for the release and the other stuff afterwards?
Comment 17 Sebastian Dröge (slomo) 2008-02-17 17:47:04 UTC
Without the checks we add invalid entries to the seek table and drop buffers without valid timestamp btw... so that should definitely be done.
Comment 18 Jan Schmidt 2008-02-17 18:49:17 UTC
OK, reverted to revision 1.85 in CVS. Please give me a patch just adding the checks.
Comment 19 Sebastian Dröge (slomo) 2008-02-18 05:12:09 UTC
Created attachment 105476 [details] [review]
mp3parse-invalid-timestamps.diff
Comment 20 Sebastian Dröge (slomo) 2008-02-18 05:12:53 UTC
I'll commit the other changes from the first patch after release.
Comment 21 Tim-Philipp Müller 2008-02-18 10:17:42 UTC
CVS+patch seems to work fine at first glance. I get neither the immediate-eos problem nor the audio-screwed-up-when-seeking-backwards problem with this patch (only did some quick tests though).
Comment 22 Sebastian Dröge (slomo) 2008-02-18 10:25:04 UTC
2008-02-18  Sebastian Dröge  <slomo@circular-chaos.org>

	* gst/mpegaudioparse/gstmpegaudioparse.c:
	(gst_mp3parse_emit_frame):
	Handler buffers without valid timestamp more correctly: Don't drop
	them and don't use the invalid timestamp to calculate the next
	timestamp. Fixes bug #516811.