uploading the file ASAP
would be there soon : http://www.leroutier.net/345830.mov

gstreamer-0.10.8
gst-plugins-base-0.10.8
gst-plugins-bad-0.10.3
gst-plugins-good-0.10.3
gst-plugins-ugly-0.10.3

Starting program: /usr/local/bin/gst-launch-0.10 playbin uri=file:///home/tpm/samples/video/quicktime/345830-lecteur.mov -v
[Thread debugging using libthread_db enabled]
[New Thread -1212377408 (LWP 16506)]
Setting pipeline to PAUSED ...
/playbin0/decoder.sink: caps = NULL
/playbin0/decoder/typefind.src: caps = video/quicktime
[New Thread -1222538320 (LWP 16510)]
Pipeline is PREROLLING ...

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread -1222538320 (LWP 16510)]
0xb7a1ad9c in qtdemux_parse_trak (qtdemux=0x8129008, trak=0x8133480) at qtdemux.c:3257
3257            stream->samples_per_frame = (stream->bytes_per_frame /
(gdb) print stream
$1 = (QtDemuxStream *) 0x8114520
(gdb) print stream->bytes_per_frame
$2 = 0
(gdb) print stream->bytes_per_packet
$3 = 0
(gdb)

this is a heavily edited file. I'm not sure how it should be parsed... A fix to avoid the segfault was commited.

        * gst/qtdemux/qtdemux.c: (qtdemux_parse_trak):
        Don't crash on twos/sowt/raw audio. #345830.

Fixed in the development version. The fix will be available in the next major release. Thank you for your bug report.

hey, ho!

The file seems to be no longer available. Do we have another one? Is this bug still valid? There is a patch for edit lists in https://bugzilla.gnome.org/show_bug.cgi?id=586848 (for handling empty entries).

Created attachment 153081 [details] [review]
qtdemux: EOS only on the last segment

Only trigger EOS if on the last sample of the last
segment.

Created attachment 153083 [details] [review]
qtdemux: Handle empty edts entries

Empty segments in edit lists in qtff files means
a segment without media for that track. This patch
makes qtdemux consider and handle empty segments
by skipping their duration in the track time position,
preventing it from being selected for buffer pushing
until the empty segment ends.

Fixes #345830

Those two patches improve edit lists handling. This one
makes qtdemux handle empty edit lists entries.

I'd appreciate some review, I'm not so sure about the
'need_preroll' for avoiding stalls due to the first
edts entry being an empty one, maybe someone out there
has a better idea. You can find a sample in #586848 and
in #607471

Thiago, I tried to apply the patch to current git and had 2 hunks rejected line 2797 and line 2806.  Those 2 were just a GST_LOG_OBJECT that was moved, so I tried to test anyway.  I tested 2 mp4 files (m4v/aac), both alternate 10 second empty and non-empty segments, but one starts with an empty segment and the other does not (See links to files below).

Your previous patch in #586848 resulted in successful playback of the video that starts with a non-empty edit.  The one that starts with an empty edit does not play at all (seek and position problems still apply).

Using this patch, neither file plays properly.  The mp4 that starts with a non-empty edit only has the initial audio segment played (and no other). The mp4 that starts with an empty edit doesn't play at all.

Note: Using quicktime, only the mp4 that starts with a non-empty edit plays properly. The other one has a sync diff of about 2 seconds. Looking at the dumpster output it seems its mediatime duration is shorter than expected, so only use as test for starting with empty edit.. don't expect good sync.

FYI: The mp4's were created using the initial patch in #601576, with an updated qtmuxedits test app (will likely submit updated patch at some point).

AFGM-edits-10sec-empty-first.mp4:
https://docs.google.com/leaf?id=0B60y2gZqzs4hZDc0YTQ0NjMtOTg4Ny00NTI3LWJmYjYtYmQyMWI1OWJlYTZi&hl=en

AFGM-edits-10sec.mp4
https://docs.google.com/leaf?id=0B60y2gZqzs4hZjFlYTM2OTUtMGJhOC00YWI1LWE5NDctZGRmNDBkMmY4Mjcy&hl=en

dumpster output for both:
http://docs.google.com/leaf?id=0B60y2gZqzs4hYzZmNzY2ZmUtMzZlNC00MTQ2LWFkN2MtZjkxMjNiMGU1YmI5&hl=en

These checks in the segment activation function where preventing the new segment event from being pushed. Removing these checks makes the second sample play in sync.

But now I need to understand why they were there in the first place. The first one is still out of sync.


==
   if (G_UNLIKELY (offset < segment->time)) {
     GST_WARNING_OBJECT (qtdemux, "offset < segment->time %" G_GUINT64_FORMAT,
         segment->time);
     return FALSE;
   }
 
   /* segment lies beyond total indicated duration */
   if (G_UNLIKELY (segment->duration != -1 && segment->time > segment->duration)
     GST_WARNING_OBJECT (qtdemux, "segment->duration %" G_GUINT64_FORMAT
         " < segment->time %" G_GUINT64_FORMAT, segment->duration,
         segment->time);
     return FALSE;
   }
==

I got the later one working. The first one seem to have sync problems even in quicktime. Are you sure that file is correct?

Yes, the video that starts with an empty edit has a sync problem .. about ~2secs off.  If you look at the dumpster screen shot, the first empty edit is 8589 when it should be about 10000.

Created attachment 155066 [details] [review]
qtdemux: Handle empty edts entries

Empty segments in edit lists in qtff files means
a segment without media for that track. This patch
makes qtdemux consider and handle empty segments
by skipping their duration in the track time position,
preventing it from being selected for buffer pushing
until the empty segment ends.

Fixes #345830

Created attachment 155067 [details] [review]
qtdemux: EOS only on the last segment

These patches fix it for me.

One thing I noticed, though, is that the seek bar (on Totem) 'skips' forward when an empty audio entry is activated. Dunno if this is relevant, but it might be something we'd like to further investigate before pushing upstream.

I reviewed both last 2 patches, and found the following in addition to pos bug:

* Seeking, pausing or playrate change can lead to a pipeline freeze.  I tried to determine if it happens specifically with an empty or non-empty seg.. but it didn't seem to matter.. Its arbitrary.

* Seeking into an empty segment (if pipeline does not freeze), results in an audio/video sync difference totalling the remainder of the empty segment.

* Was not able to get reverse playback to work, freeze every time.

I'm working on supporting empty edits in qtdemux from an older code base (gstreamer-0.10.23 and gst-plugins-good-0.10.14).  I've have things working quite well so far, but still need to resolve a reverse play rate issue, and do some more testing. I'll post patches when I'm done, but in the meantime hear are a few observations that will likely help you with the above mentioned issues:

* alsasink 'async' property needs to be set FALSE, otherwise seek to an empty segtment will cause the pipeline to freeze during preroll

* basesink gst_base_sink_get_position reports wrong position if an empty segment is followed by a non-empty segment.. it reports the new segment's start time up until buffers are being proccessed.  My fix is to return clocktime - base + latency until clocktime has caught up with the segment's accum time.

* ringbuffer gst_ring_buffer_set_flushing is pausing the ringbuffer when flushing (called by baseaudiosink upon receiving FLUSH start event, after seek).  If seeking to an empty segment, the ringbuffer remains paused and therefore alsasink's provided clock is out of sync by the time the next non-empty segment starts to play.  I also tried calling gst_audioringbuffer_start from baseaudiosink's gst_base_audio_sink_change_state, from PLAYING -> PLAYING transition.. I found not pausing the ringbuffer in the first place worked better since clock timestamps are produced uninterrupted.

* qtdemux - I worked off your original patch from bug "586848", and added a few other fixes you probably have in the current git version. I would just note that your last patch prevents reverse playback.

I used mc to correct the length of the first empty edit in the above posted mp4:

AFGM-edits-10sec-empty-first-fixed.mp4:
http://docs.google.com/leaf?id=0B60y2gZqzs4hMzdiMDEzOTUtYWM0Mi00ZDAyLTkwNzktYjA3N2EyOTA0NDFi&hl=en

Any progress on this?

Created attachment 265246 [details] [review]
qtdemux: do not ignore empty segments

Make sure empty segments are used and pushed with a gap event
to represent its data (or lack of it)

Each QtSegment is mapped into a GstSegment with the corresponding
media range. For empty QtSegments a gap event is pushed instead
of GstBuffers and it advances to the next QtSegment.

To make this work with seeks, need to keep track of the starting
'base' to make sure it remains consistently increasing when
pushing new segment events.
For example: if a seek makes qtdemux start from 5s, the first
segment will have a base=0. When the next segment is activated,
its base time will be QtSegment.time - qtdemux.segment_base so
that it doesn't include the first 5s that weren't played and
shouldn't be accounted on the running time

After fixing https://bugzilla.gnome.org/show_bug.cgi?id=721666 backwards playback will be working and dash and mss still work.

The only issue remaining before pushing this patch is that it will 'play' empty edit list entries at the beginning of the file instead of skipping those as fixed in https://bugzilla.gnome.org/show_bug.cgi?id=700264

So, what do we want to do here? Do we want to add gaps as the file requests or do we skip this initial empty data?

Also, in the sample from #700264 we skip a initial track instead of displaying it, so if we could display that, the gap should be respected.

I think we should "play" the initial gaps if the file requests that.

commit 90a5565229f39b181e8644c4b5482c614dbcb0fd
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri Jan 3 10:59:35 2014 -0300

    qtdemux: do not ignore empty segments
    
    Make sure empty segments are used and pushed with a gap event
    to represent its data (or lack of it)
    
    Each QtSegment is mapped into a GstSegment with the corresponding
    media range. For empty QtSegments a gap event is pushed instead
    of GstBuffers and it advances to the next QtSegment.
    
    To make this work with seeks, need to keep track of the starting
    'base' to make sure it remains consistently increasing when
    pushing new segment events.
    For example: if a seek makes qtdemux start from 5s, the first
    segment will have a base=0. When the next segment is activated,
    its base time will be QtSegment.time - qtdemux.segment_base so
    that it doesn't include the first 5s that weren't played and
    shouldn't be accounted on the running time
    
    This purposedly will remove the fix made for
    https://bugzilla.gnome.org/show_bug.cgi?id=700264, at this
    point it was decided to respect the gaps, even if they cause
    a delay on playback, because that's the way the file was crafted.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=345830