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 544509 - [qtdemux] Regression in segment.stop
[qtdemux] Regression in segment.stop
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.9
Other Linux
: Normal blocker
: 0.10.9
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-07-24 09:15 UTC by Edward Hervey
Modified: 2008-07-27 12:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
alternative patch (18.41 KB, patch)
2008-07-25 09:14 UTC, Wim Taymans
none Details | Review
Fix regression (3.97 KB, patch)
2008-07-25 13:23 UTC, Edward Hervey
committed Details | Review

Description Edward Hervey 2008-07-24 09:15:01 UTC
There is a regression in pre-release qtdemux whereby the 'stop' value of the newsegment outputted by qtdemux will not correspond to the stop value of the requested segment seek.

This worked perfectly in gst-plugins-good-0.10.8.

Still don't know exactly which commit broke it. Will figure this out.
Comment 1 Wim Taymans 2008-07-25 09:14:05 UTC
Created attachment 115233 [details] [review]
alternative patch

an alternative patch to implement reverse playback in qtdemux.
Comment 2 Wim Taymans 2008-07-25 09:14:42 UTC
probably broken with the reverse playback patch. Attached is an alternative patch for reverse playback. Last time I checked, it had some bugs but it uses a lot less special casing.
Comment 3 Edward Hervey 2008-07-25 10:01:22 UTC
That patch fixes this issue, but still leaves us with the original problem that was fixed by julien's patch:

On the following file, if you pause, seek to a position, then play ... it will stop the video playback before the end of the file.

The part that breaks it around line 1273 (in _activate_segment() the part that calculates the stop position of the segment).

http://dolphy-tech.net/files/iron_man.mov
Comment 4 Edward Hervey 2008-07-25 13:23:59 UTC
Created attachment 115250 [details] [review]
Fix regression

The issue was kind of stupid. The original stop code (from before julien's code) was correct... except for the part where it was using the stop value directly from GstSegment which is in stream-time and not in buffer-time. This converts it so that the outputted newsegment event is correct.

I also added at the top the documentation regarding quicktime segments from wim's proposal.
Comment 5 Wim Taymans 2008-07-25 13:40:46 UTC
looks correct
Comment 6 Jan Schmidt 2008-07-25 13:47:48 UTC
Awesome - pop it in
Comment 7 Edward Hervey 2008-07-25 14:50:53 UTC
2008-07-25  Edward Hervey  <edward.hervey@collabora.co.uk>

	* gst/qtdemux/qtdemux.c: (gst_qtdemux_activate_segment):
	Fix segment-stop regression.
	Add documentation regarding segments in quicktime files by Wim Taymans.
	Fixes #544509

Comment 8 Edward Hervey 2008-07-27 09:31:21 UTC
Alas, while fixing the vast majority of the regressions... it has brought in a new  but similar regression. This time it's both the start and stop values of the newsegment which are wrong.

By looking at the results, when asking for a segment seek of [start:A, stop:B] it's returning a newsegment of [start:A-(B-A), stop:A] (ergo : segment is of the proper duration, but shifted back in time by (B-A)).

Most likely, this is another value in the code for which the time-realm (track time vs segment time) is unclear.

Will figure this out and attach a patch soon. Sorry for re-opening this, but running the QA tests takes a long time :)
Comment 9 Edward Hervey 2008-07-27 12:17:29 UTC
my bad, this isn't a bug in qtdemux. Just a bad assumption in the segment-seek tests.

The assumption is that if I do an accurate segment seek for [start:A, stop:B], the resulting newsegment would have [start:A, stop:B, position:A]... which is wrong.

The only thing that can be assumed is:
* newsegment.position WILL be equal to seek.start
* the sum of all the newsegments duration will be equal to ABS(seek.stop - seek.start). (if there's only one, then ABS(newsegment.stop - newsegment.start) == ABS(seek.stop - seek.start))

I was adding to that the (not always correct) assumptions that:
* newsegment.start would be equal to seek.start
* newsegment.stop would be equal to seek.stop

The two assumptions are correct for most demuxer/decoders... but with quicktime's EDL system... it can be wrong.

closing this bug again. Sorry for the noise, better be sure than regret it later :)