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 675132 - tsdemux: implement proper seeking with binary search and keyframe detection
tsdemux: implement proper seeking with binary search and keyframe detection
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: High enhancement
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 721812 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-04-30 10:53 UTC by Matej Knopp
Modified: 2014-06-12 12:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (14.89 KB, patch)
2012-04-30 10:53 UTC, Matej Knopp
none Details | Review
Patch (14.89 KB, patch)
2012-04-30 11:24 UTC, Matej Knopp
none Details | Review
Patch (14.82 KB, patch)
2012-05-03 15:05 UTC, Matej Knopp
none Details | Review
This patch corrects the observed bug. (16.01 KB, patch)
2014-03-19 23:26 UTC, Mathieu Duponchelle
needs-work Details | Review
Fixes Edward's observations, will be fixed up. (5.85 KB, patch)
2014-03-20 23:32 UTC, Mathieu Duponchelle
none Details | Review
Requested fixup (15.90 KB, patch)
2014-03-21 17:14 UTC, Mathieu Duponchelle
none Details | Review
Fixes Edward's observations, will be fixed up. (15.49 KB, patch)
2014-04-02 02:35 UTC, Mathieu Duponchelle
none Details | Review
Define a custom Gst Flow (15.76 KB, patch)
2014-04-02 13:11 UTC, Mathieu Duponchelle
none Details | Review
Plug some remaining leaks (16.78 KB, patch)
2014-04-07 14:56 UTC, Thibault Saunier
none Details | Review
fixes the reported issue (17.99 KB, patch)
2014-05-15 12:02 UTC, Mathieu Duponchelle
needs-work Details | Review
Fixes Edward's observations (15.49 KB, patch)
2014-05-27 09:34 UTC, Mathieu Duponchelle
none Details | Review
Really fixes Edward's observations (17.98 KB, patch)
2014-05-27 09:42 UTC, Mathieu Duponchelle
none Details | Review
Patch rebased on today's changes. (17.99 KB, patch)
2014-05-27 15:21 UTC, Mathieu Duponchelle
none Details | Review
Rebase latest version of the patch (17.97 KB, patch)
2014-05-30 14:56 UTC, Mathieu Duponchelle
none Details | Review
Fixes a bug on accurate seeking with stop == GST_CLOCK_TIME_NONE (18.03 KB, patch)
2014-06-10 18:34 UTC, Mathieu Duponchelle
committed Details | Review

Description Matej Knopp 2012-04-30 10:53:10 UTC
Created attachment 213093 [details] [review]
Patch

Attached patch implements hinted binary search for seek and keyframe detection of H264 and Mpeg2 streams.
Comment 1 Matej Knopp 2012-04-30 11:24:24 UTC
Created attachment 213094 [details] [review]
Patch

fixed typo
Comment 2 Matej Knopp 2012-05-03 15:05:45 UTC
Created attachment 213388 [details] [review]
Patch

Fixes the codecparsers path in makefile.am
Comment 3 Tim-Philipp Müller 2013-04-21 10:33:22 UTC
I didn't know this patch existed, nice!

Do you happen to have an updated version at hand? It doesn't seem to apply any more.
Comment 4 Matej Knopp 2013-04-21 11:40:23 UTC
There didn't seem to be any interest so I stopped updating the patch and I forked the demuxer instead. I can make another patch when I have some time, there are two things though. 

mpegts_packetizer_flush flushes observations. This means on every flush it forgets first / last PCRs, etc. This doesn't work well for seeking. In my fork I just commented that out, but perhaps it needs to be addressed differently.

The old code for finding MPEG 2 keyframe doesn't compile anymore (the parser interface has changed), I'll need to update it.
Comment 5 Tim-Philipp Müller 2013-04-21 11:53:28 UTC
Yeah, I noticed that too (that it flushes the observations), see my comments on bug #698050. I'm going to ask Josep about that tomorrow, I think it was him who added that.
Comment 6 Edward Hervey 2013-10-28 06:29:32 UTC
Update from discussions I had with Matej at the GStreamer conference:

Having a completely different codepath for "scanning for seeking" introduces too much complexity.

There already is code which waits until a certain item (we have a PCR to calculate the proper stream time and PTS/DTS) before pushing downstream reconstructed packets. Search for ->pending_ts and ->pending to see what's going on.

We should re-use this logic to do "scanning" instead of having a dedicated codepath for doing header/packet/keyframe scanning. The reason is that currently most of the overhead in tsdemux is due to i/o latency, which you won't avoid by scanning 4-10 bytes out of 188 bytes. Furthermore by letting all data go through the current "regular" codepath it allows all pieces of code to collect more information (and therefore get a better byte offset the following time round).

Changes:
* If seeking and reconstructing buffers, activate h264/mpeg2 header scanning (and only in those cases).
* If doing accurate seek (and only then), once we have enough time information determine whether we are at/before the requested seek location and we have a keyframe. If we are, let the buffers go through. If we are not, re-request a new byte offset from the packetizer (it will have a better estimation now), drop pending buffers and carry on grabbing data from that location.

Note that we could also use that last item for non-accurate seeks if we end up really too far from target seek time (say > 10s).
Comment 7 Nicolas Dufresne (ndufresne) 2014-01-08 19:53:15 UTC
*** Bug 721812 has been marked as a duplicate of this bug. ***
Comment 8 Mathieu Duponchelle 2014-03-10 17:31:28 UTC
Hey Matej, I'm interested in your demuxer fork, I'd like to produce an upstreamable fix for tsdemux, and looking at your fork would be very valuable.
Comment 9 Matej Knopp 2014-03-10 17:49:54 UTC
Mathieu, the source code can be found at

https://s3.amazonaws.com/MatejK/tsdemux.tar.bz2

but I don't think it can be used for an upstreamable fix. It's forked from rather old gstreamer version, way before recent Edward's commits. It also doesn't support push mode and the overall approach probably wouldn't work for push mode anyway. It works for us because we have different requirements.
Comment 10 Mathieu Duponchelle 2014-03-10 17:55:27 UTC
Matej, thanks but I get an access denied with that link :)
Comment 11 Matej Knopp 2014-03-10 18:01:36 UTC
Sorry, should work now.
Comment 12 Mathieu Duponchelle 2014-03-10 18:42:24 UTC
Thanks !
Comment 13 Mathieu Duponchelle 2014-03-19 23:26:25 UTC
Created attachment 272448 [details] [review]
This patch corrects the observed bug.
Comment 14 Mathieu Duponchelle 2014-03-19 23:26:53 UTC
Hi, here is an implementation compliant with Edward's remarks, as it's integrated in the codepath. It's been tested with gst-validate and a significant set of sample files, and all the bugs we encountered are fixed with it.

It obviously makes seeking a little slower, as there now needs to be some decoding done between the keyframe and the actual frame, but that delay is only marginally introduced by the scanning, and mostly comes from the decoding, which is inevitable if one wants to do things correctly and not get artefacts.

Additionally, the case where a seek is requested and there's no keyframe before it (can happen with broken samples that I can provide) is correctly accounted for.

The exact process for seeking is described in the commit message.

In the case of interlaced samples, an additional fix is needed in codecparsers, which didn't set a framerate, the bug report is here : https://bugzilla.gnome.org/show_bug.cgi?id=726752 .

What do you think folks ?
Comment 15 Tim-Philipp Müller 2014-03-20 12:10:15 UTC
It should only make seeking slower if the KEYUNIT flag is not set. If it's set (and most movie players will set it), then the segment start should be updated to the position of the keyframe instead.
Comment 16 Tim-Philipp Müller 2014-03-20 14:29:30 UTC
For background information on demuxer behaviour when seeking:
http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/design/part-seeking.txt#n115

So the question is basically: what to do in the demuxer when various seek flags (ACCURATE, KEY_UNIT, SNAP_*) are set or not set.

ACCURATE is about accuracy of the position itself from the start (that is: don't use bitrate estimate and just jump somewhere).

If a seek to position X is requested, the demuxer should start outputting data so that a frame at position X can be decoded correctly. This implies that if NO seek flags are set, it needs to output data from the keyframe before or at X, so that X can be decoded correctly.

If the KEY_UNIT flag is set, the demuxer is theoretically free to pick whether the start outputting from the keyframe before X or after X, but the segment start needs to be updated to the position of that keyframe then, so it needs to scan for a keyframe.

If KEY_UNIT is not set the segment start is X (and it needs to start outputting data from the previous keyframe so that X can be decoded).

If KEY_UNIT | SNAP_AFTER are set, the demuxer needs to scan forward from X and set segment start to keyframe after X.

SNAP flags are nice to have, but not essential. Movie players will use KEY_UNIT usually.

Outputting data from X and letting parser/decoder throw away data to the following keyframe is neither correct nor very fast. It is not correct because if X is requested then the frame at position X needs to be decodable. It is not super-fast either, because effectively you throw data at parser+decoder that's going to be thrown away until you get a keyframe (but of course processing in forward order is more efficient).
Comment 17 Mathieu Duponchelle 2014-03-20 14:44:51 UTC
OK, so considering the SNAP flags as an optional feature, I think that behaviour wise what the patch does is correct.

I'll wait for Edward's observations now.
Comment 18 Edward Hervey 2014-03-20 14:52:36 UTC
(In reply to comment #16)
> 
> ACCURATE is about accuracy of the position itself from the start (that is:
> don't use bitrate estimate and just jump somewhere).

  Or rather:
    * DO use bitrate estimation
    * DO jump somewhere
    * BUT wait until the first timestamp,
      * If it's the position you wanted, output stuff (segment, buffers,...)
      * If not, go back to the first step

  Sorry, we don't have any other magic to figure out offset from requested time. Unless you want to scan the whole file first (Don't do that...).
Comment 19 Mathieu Duponchelle 2014-03-20 15:05:21 UTC
Not sure I understand, the estimation we use is the same as was there before, if it's not accurate enough (ie we could find a timestamp > what we really want and it would so happen that we would find a keyframe while scanning back with timestamp > what we really want) that's not the problem the patch intends to fix, what it tackles is keyframe detection, maybe the bug can stay open as "Fix binary search" ? Again, not sure I understand.
Comment 20 Tim-Philipp Müller 2014-03-20 15:06:07 UTC
Of course you can do something like a bounded bisect and use min/max/average bitrates in the process, that's not what I meant.
Comment 21 Edward Hervey 2014-03-20 17:19:00 UTC
Review of attachment 272448 [details] [review]:

This is going in the right direction, but should be rebased in a slightly different way:

* Accumulate buffers that contain SPS and/or PPS and/or SEI (and not use bytewriters) starting from the first SPS you see
* Accumulate frames starting from the first keyframe
* If you see a new keyframe, flush all previous keyframe/frame
* When you see a frame for your target seek time, you can push out the requested seek segment on all streams and push out pending buffers

The advantage of this is that you don't need to rewind back to a previous position. You've already parsed/collected everything you want and can just push it.

Note that non-h264 streams will also need to have their buffers collected up until the requested seek position. You can use the stream->pending list for that.

::: gst/mpegtsdemux/mpegtsbase.c
@@ +1105,2 @@
     /* If we don't have enough data, return */
+    if (G_UNLIKELY (pret == PACKET_NEED_MORE)) {

Not needed

@@ +1288,3 @@
       if (G_UNLIKELY (ret != GST_FLOW_OK))
         goto error;
+      GST_DEBUG ("base seek offset set to %" G_GUINT64_FORMAT,

Not needed

::: gst/mpegtsdemux/tsdemux.c
@@ +576,3 @@
+  GstH264NalParser *parser = h264infos->parser;
+
+  if (G_UNLIKELY (parser == NULL)) {

Just initialize everything in one go instead of one by one ? (i.e. parser and other fields like sps,...)

@@ +738,3 @@
+  GstTsDemuxKeyFrameScanFunction scan_function = NULL;
+
+  if (stream && stream->stream_type == GST_MPEG_TS_STREAM_TYPE_VIDEO_H264) {

Just set all of this once for the stream instead of on every function call ?

@@ +808,2 @@
   if (G_UNLIKELY (start_offset == -1)) {
+    GST_WARNING ("Couldn't convert tart position to an offset");

Wasn't meant to be there ?

@@ +817,3 @@
   res = GST_FLOW_OK;
 
+  if (flags & GST_SEEK_FLAG_ACCURATE) {

Would make more sense to just store the segment in a separate field (seek_segment?) and use/adjust it later.

@@ +829,3 @@
   }
 
+  for (tmp = demux->program->stream_list; tmp; tmp = tmp->next) {

That's not entirely true. You won't need keyframe on audio streams, and you won't need keyframes on streams you can't handle.
Comment 22 Mathieu Duponchelle 2014-03-20 17:38:12 UTC
(In reply to comment #21)
> Review of attachment 272448 [details] [review]:
> 
> This is going in the right direction, but should be rebased in a slightly
> different way:
> 
> * Accumulate buffers that contain SPS and/or PPS and/or SEI (and not use
> bytewriters) starting from the first SPS you see
> * Accumulate frames starting from the first keyframe
> * If you see a new keyframe, flush all previous keyframe/frame
> * When you see a frame for your target seek time, you can push out the
> requested seek segment on all streams and push out pending buffers
> 
> The advantage of this is that you don't need to rewind back to a previous
> position. You've already parsed/collected everything you want and can just push
> it.

We need to rewind anyway to find the first SPS, why would we want to make a second forward pass when we can get everything in the first pass ?

> 
> Note that non-h264 streams will also need to have their buffers collected up
> until the requested seek position. You can use the stream->pending list for
> that.

up until the requested seek position but from which point ?

> 
> ::: gst/mpegtsdemux/mpegtsbase.c
> @@ +1105,2 @@
>      /* If we don't have enough data, return */
> +    if (G_UNLIKELY (pret == PACKET_NEED_MORE)) {
> 
> Not needed
> 
> @@ +1288,3 @@
>        if (G_UNLIKELY (ret != GST_FLOW_OK))
>          goto error;
> +      GST_DEBUG ("base seek offset set to %" G_GUINT64_FORMAT,
> 
> Not needed
> 

OK

> ::: gst/mpegtsdemux/tsdemux.c
> @@ +576,3 @@
> +  GstH264NalParser *parser = h264infos->parser;
> +
> +  if (G_UNLIKELY (parser == NULL)) {
> 
> Just initialize everything in one go instead of one by one ? (i.e. parser and
> other fields like sps,...)
> 

OK

> @@ +738,3 @@
> +  GstTsDemuxKeyFrameScanFunction scan_function = NULL;
> +
> +  if (stream && stream->stream_type == GST_MPEG_TS_STREAM_TYPE_VIDEO_H264) {
> 
> Just set all of this once for the stream instead of on every function call ?
> 

OK

> @@ +808,2 @@
>    if (G_UNLIKELY (start_offset == -1)) {
> +    GST_WARNING ("Couldn't convert tart position to an offset");
> 
> Wasn't meant to be there ?
> 

What do you mean ?

> @@ +817,3 @@
>    res = GST_FLOW_OK;
> 
> +  if (flags & GST_SEEK_FLAG_ACCURATE) {
> 
> Would make more sense to just store the segment in a separate field
> (seek_segment?) and use/adjust it later.
> 

What would be the reason for that ?

> @@ +829,3 @@
>    }
> 
> +  for (tmp = demux->program->stream_list; tmp; tmp = tmp->next) {
> 
> That's not entirely true. You won't need keyframe on audio streams, and you
> won't need keyframes on streams you can't handle.

OK, I'll discriminate based on the stream content.
Comment 23 Mathieu Duponchelle 2014-03-20 23:32:48 UTC
Created attachment 272529 [details] [review]
Fixes Edward's observations, will be fixed up.

I fixed the observations that I could fix, I'll need Edward's answers to proceed with the rest, in particular the one regarding the scanning process.

To find the SPS/PPS preceding a keyframe, I need to rewind the stream, maybe I don't understand what you meant with "the first SPS you see", certainly you don't mean scanning the whole file from the beginning ?
Comment 24 Edward Hervey 2014-03-21 08:15:07 UTC
(In reply to comment #22)
> (In reply to comment #21)
> > Review of attachment 272448 [details] [review] [details]:
> > 
> > This is going in the right direction, but should be rebased in a slightly
> > different way:
> > 
> > * Accumulate buffers that contain SPS and/or PPS and/or SEI (and not use
> > bytewriters) starting from the first SPS you see
> > * Accumulate frames starting from the first keyframe
> > * If you see a new keyframe, flush all previous keyframe/frame
> > * When you see a frame for your target seek time, you can push out the
> > requested seek segment on all streams and push out pending buffers
> > 
> > The advantage of this is that you don't need to rewind back to a previous
> > position. You've already parsed/collected everything you want and can just push
> > it.
> 
> We need to rewind anyway to find the first SPS, why would we want to make a
> second forward pass when we can get everything in the first pass ?

  You *need* to collect everything going forward.
  For a certain frame you need to push the SPS, PPS and SEI that are just before, and not some misc one you found later on. You're not guaranteed a SPS/PPS/SEI you saw after the target frame actually applies to that frame/keyframe.

  From a given position, until you've found a SPS/PPS, you don't collect/store anything. If you see a frame which is after the requested seek position and you haven't seen a PPS/SPS, you recalculate another (earlier) offset.

  If you're lucky and you do see/store SPs/PPS/SEI/Keyframe ... you can just push them.

  If there was too much (multiple keyframes between SPS/PPS and target frame), you discard the keyframe/frame that you didn't want.

> 
> > 
> > Note that non-h264 streams will also need to have their buffers collected up
> > until the requested seek position. You can use the stream->pending list for
> > that.
> 
> up until the requested seek position but from which point ?

  You'll just have to put some delta before the seek position (2 previous buffers for audio should be enough, 1 second for video might be required).

> 
> > @@ +817,3 @@
> >    res = GST_FLOW_OK;
> > 
> > +  if (flags & GST_SEEK_FLAG_ACCURATE) {
> > 
> > Would make more sense to just store the segment in a separate field
> > (seek_segment?) and use/adjust it later.
> > 
> 
> What would be the reason for that ?
> 

  To handle all the seek variants that tim explained.


Could you also make it just one updated patch instead of patches over patches ? Makes reviewing easier.
Comment 25 Mathieu Duponchelle 2014-03-21 17:14:29 UTC
Created attachment 272581 [details] [review]
Requested fixup
Comment 26 Thibault Saunier 2014-03-21 17:35:33 UTC
(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Review of attachment 272448 [details] [review] [details] [details]:
> > > 
> > > This is going in the right direction, but should be rebased in a slightly
> > > different way:
> > > 
> > > * Accumulate buffers that contain SPS and/or PPS and/or SEI (and not use
> > > bytewriters) starting from the first SPS you see
> > > * Accumulate frames starting from the first keyframe
> > > * If you see a new keyframe, flush all previous keyframe/frame
> > > * When you see a frame for your target seek time, you can push out the
> > > requested seek segment on all streams and push out pending buffers
> > > 
> > > The advantage of this is that you don't need to rewind back to a previous
> > > position. You've already parsed/collected everything you want and can just push
> > > it.
> > We need to rewind anyway to find the first SPS, why would we want to make a
> > second forward pass when we can get everything in the first pass ?
> 
>   You *need* to collect everything going forward.
>   For a certain frame you need to push the SPS, PPS and SEI that are just
> before, and not some misc one you found later on. You're not guaranteed a
> SPS/PPS/SEI you saw after the target frame actually applies to that
> frame/keyframe.
This is why we always get the SPS/PPS/SEI placed *before* the keyframe that will be first pushed.
>   From a given position, until you've found a SPS/PPS, you don't collect/store
> anything. If you see a frame which is after the requested seek position and you
> haven't seen a PPS/SPS, you recalculate another (earlier) offset.
> 
>   If you're lucky and you do see/store SPs/PPS/SEI/Keyframe ... you can just
> push them.
> 
>   If there was too much (multiple keyframes between SPS/PPS and target frame),
> you discard the keyframe/frame that you didn't want.

I am not sure what you are saying is wrong with the current behaviour. What is done right now is the following:

  1 - We get the data from the packetizer computed as previously
  2 - We try to find a PPS/SPS/keyframe in the first chunk of data passed
  3 - if we do not find SPS + PPS + SEI:
     - rewind further in the stream
     - got back to 1

So we end up accumulating the SPS/PPS that are right *before* the wanted keyframe and the various SEI present  between the SPS/PPS and the wanted keyframe. We know that the SPS/PPS might be far away from the seeked point and it  would make sense to actually keep the information about where they are... etc  somewhere to avoid needing to reparse so often.

> > > Note that non-h264 streams will also need to have their buffers collected up
> > > until the requested seek position. You can use the stream->pending list for
> > > that.
> > 
> > up until the requested seek position but from which point ?
> 
>   You'll just have to put some delta before the seek position (2 previous
> buffers for audio should be enough, 1 second for video might be required).

Yes, this is what is done already with the current seeking code anyway.
 
> > 
> > > @@ +817,3 @@
> > >    res = GST_FLOW_OK;
> > > 
> > > +  if (flags & GST_SEEK_FLAG_ACCURATE) {
> > > 
> > > Would make more sense to just store the segment in a separate field
> > > (seek_segment?) and use/adjust it later.
> > > 
> > 
> > What would be the reason for that ?
> > 
> 
>   To handle all the seek variants that tim explained.

Apart from SNAP which is not handled, I  think the current patch already handles the various variants.
Comment 27 Edward Hervey 2014-03-24 07:24:27 UTC
Review of attachment 272581 [details] [review]:

::: gst/mpegtsdemux/mpegtsbase.c
@@ +1105,2 @@
     /* If we don't have enough data, return */
+    if (G_UNLIKELY (pret == PACKET_NEED_MORE)) {

Remove the {} so that this block doesn't end up in the patch.

@@ +1287,3 @@
       if (G_UNLIKELY (ret != GST_FLOW_OK))
         goto error;
+

Remove this. extra line

::: gst/mpegtsdemux/mpegtsbase.h
@@ +114,3 @@
   guint64	seek_offset;
 
+  /* Used when seeking for a keyframe to go backward in the stream */

This is only used in tsdemux afaics. It should therefore be moved there (and we end up with no changes in mpegtsbase)

::: gst/mpegtsdemux/tsdemux.c
@@ +793,3 @@
       SEGMENT_ARGS (seeksegment));
 
+  start = MAX (0, start - SEEK_TIMESTAMP_OFFSET);

This is fine ... but in that case you need to remove the same thing from the _ts_to_offset() call just below (just use start)

@@ +1411,3 @@
+tsdemux_h264_parsing_info_clear (TSDemuxH264ParsingInfos * h264infos)
+{
+  if (h264infos->sps)

Won't h264infos *always* have all those fields set and present ? If so all the if() are not needed. And you should reset them to NULL if you reuse this structure.
Comment 28 Mathieu Duponchelle 2014-04-02 02:35:44 UTC
Created attachment 273444 [details] [review]
Fixes Edward's observations, will be fixed up.

I fixed Edward's observations. Regarding the h264infos they don't get reused.

I also fixed a remaining problem with my algorithm, which queued packets that it shouldn't have queued as the rewind was still happening, in certain cases it was leading to a buffer getting discarded.

It returns GST_FLOW_CUSTOM_ERROR for now in push_pending_data, Edward do you think I should actually define a custom error value for that? Not too sure if it is needed.

To test the patch, validate is a good option, I use such command lines :

gst-validate-1.0 filesrc location= /home/meh/Downloads/GH1_00094_1920x1280.MTS ! tsdemux ! h264parse ! avdec_h264 ! xvimagesink --set-scenario scrub_forward_seeking to assert that the behaviour is correct.
Comment 29 Edward Hervey 2014-04-02 06:04:20 UTC
  Thanks for the update !

> It returns GST_FLOW_CUSTOM_ERROR for now in push_pending_data, Edward do you
> think I should actually define a custom error value for that? Not too sure if
> it is needed.

  It would be cleaner for code comprehension to redefine it (but still use that value).

> 
> To test the patch, validate is a good option, I use such command lines :
> 
> gst-validate-1.0 filesrc location= /home/meh/Downloads/GH1_00094_1920x1280.MTS
> ! tsdemux ! h264parse ! avdec_h264 ! xvimagesink --set-scenario
> scrub_forward_seeking to assert that the behaviour is correct.

  Will have a look at that.
Comment 30 Mathieu Duponchelle 2014-04-02 13:11:10 UTC
Created attachment 273467 [details] [review]
Define a custom Gst Flow
Comment 31 Thibault Saunier 2014-04-07 14:56:08 UTC
Created attachment 273719 [details] [review]
Plug some remaining leaks
Comment 32 antistress 2014-04-14 19:22:59 UTC
Hi,

I've a question which may have or may not have the place here.

TS format allows to embed subtitles. That's incidentally how french digital TV (TNT) works.

Is GStreamer able to keep embedded subtitles while working with these files ?

Thanks
Comment 33 Nicolas Dufresne (ndufresne) 2014-04-14 19:53:12 UTC
(In reply to comment #32)
> Hi,
> 
> I've a question which may have or may not have the place here.
> 
> TS format allows to embed subtitles. That's incidentally how french digital TV
> (TNT) works.
> 
> Is GStreamer able to keep embedded subtitles while working with these files ?
> 
> Thanks

This is not a support forum and this comment has nothing to do with this bug. Please ask question on mailing list.
Comment 34 Mathieu Duponchelle 2014-04-19 00:34:28 UTC
Hey Edward, did you find some time to consider merging that ?
Comment 35 Alex Băluț 2014-05-11 18:25:10 UTC
Review of attachment 273719 [details] [review]:

I tried the patch and when I render in Pitivi a project with clips backed by Panasonic MTS files there are some random pops in the audio when a clip ends and another starts. They seem to occur randomly - I rendered twice and compared in audacity the audio streams.
Comment 36 Mathieu Duponchelle 2014-05-15 11:56:10 UTC
Hi aleb, can you share a project that will allow us to reproduce ?

The patch I'm about to submit might have fixed this issue, I would be glad to have feedback on it too :)
Comment 37 Mathieu Duponchelle 2014-05-15 12:02:03 UTC
Created attachment 276600 [details] [review]
fixes the reported issue

That updated patch fixes audio / video synchronization when seeking before the first timestamp, and plugs some minor leaks.
Comment 38 Edward Hervey 2014-05-27 08:02:27 UTC
Review of attachment 276600 [details] [review]:

::: gst/mpegtsdemux/tsdemux.c
@@ +75,3 @@
     GST_TIME_ARGS((a).position), GST_TIME_ARGS((a).duration)
 
+#define GST_FLOW_REWINDING GST_FLOW_CUSTOM_ERROR

That custom flow return is only stored in one place, and the only place it's being read in is ... to convert it back to a GST_FLOW_OK ?

Is it really needed ?

@@ +849,3 @@
+    stream->needs_keyframe = TRUE;
+
+    stream->seeked_pts = 0;

It would be safer to use GST_CLOCK_TIME_NONE as default/unset values instead of 0.

@@ +2137,3 @@
   }
 
+  if ((stream->seeked_pts && GST_CLOCK_TIME_IS_VALID (stream->seeked_pts)

All of this would be simpler if the default value was GST_CLOCK_TIME_NONE :)

@@ +2213,3 @@
+   * and didn't want the data to be queued
+   */
+  if (res == GST_FLOW_REWINDING)

Only place where the custom GstFlowReturn seems to be used...
Comment 39 Mathieu Duponchelle 2014-05-27 09:34:09 UTC
Created attachment 277267 [details] [review]
Fixes Edward's observations
Comment 40 Mathieu Duponchelle 2014-05-27 09:35:18 UTC
As discussed on IRC, we'll keep the custom flow, it is used to avoid queuing data and setting the continuity counter when rewinding.

initialization to CLOCK_TIME_NONE is done, thanks for spotting that.
Comment 41 Mathieu Duponchelle 2014-05-27 09:42:14 UTC
Created attachment 277268 [details] [review]
Really fixes Edward's observations
Comment 42 Edward Hervey 2014-05-27 09:51:37 UTC
Review of attachment 277268 [details] [review]:

::: gst/mpegtsdemux/tsdemux.c
@@ +1993,3 @@
       demux->segment.rate = demux->rate;
     }
+  } else if (demux->segment.start < firstts) {

Just noticed this

What has it got to do with the seeking feature ?

This code will only be called if demux.segment is not in GST_FORMAT_TIME.

And then you modify the (non-time) values ?
Comment 43 Edward Hervey 2014-05-27 09:55:23 UTC
Ignore the part about it not being GST_FORMAT_TIME. The rest of the questions still apply
Comment 44 Mathieu Duponchelle 2014-05-27 10:02:28 UTC
This has to do with the seeking feature in that we now keep the segment asked for in the seek:

+  if (flags & GST_SEEK_FLAG_ACCURATE) {
+    /* keep the seek infos for our segment */
+    demux->segment = seeksegment;
+  }

However, if the requested segment is [0 -> X] and the first timestamp is > 0, we want to update the segment we will push to start at the first ts (the logic is similar to the non-seeking case).
Comment 45 Mathieu Duponchelle 2014-05-27 15:21:36 UTC
Created attachment 277310 [details] [review]
Patch rebased on today's changes.
Comment 46 Edward Hervey 2014-05-27 15:55:47 UTC
(In reply to comment #45)
> Created an attachment (id=277310) [details] [review]
> Patch rebased on today's changes.

wrong patch ? That one has gone back to using 0 (instead of GST_CLOCK_TIME_NONE)
Comment 47 Mathieu Duponchelle 2014-05-30 14:56:12 UTC
Created attachment 277561 [details] [review]
Rebase latest version of the patch

Wrong patch indeed, sorry.
Comment 48 Mathieu Duponchelle 2014-06-10 18:34:59 UTC
Created attachment 278218 [details] [review]
Fixes a bug on accurate seeking with stop == GST_CLOCK_TIME_NONE

When an accurate seek is requested, we keep the requested segment to send it, and update it if the pts of the first buffer that we send is superior to the start of the segment. We also update the stop of the requested segment, adding to it the offset so that we can indeed push the requested duration. However, we were also adding it when stop == GST_CLOCK_TIME_NONE, which was not a very good idea. This patch fixes the bug.
Comment 49 Edward Hervey 2014-06-11 07:37:46 UTC
Yay, that does fix the regression. I guess the last thing is refusing ACCURATE behaviour if there's no parsing features (i.e. if there's no H264), otherwise it ends up with a worse behaviour (lag when resuming playback after seek).
Comment 50 Mathieu Duponchelle 2014-06-11 17:16:29 UTC
Hm I can observe a similar amount of lag here with and without the patch, and I think updating the segment to actually keep the stop value when an accurate seek is requested for any streams is desirable, not sure what to do / how to reproduce the lag that you observe Edward ?
Comment 51 Edward Hervey 2014-06-12 04:53:28 UTC
(In reply to comment #50)
> Hm I can observe a similar amount of lag here with and without the patch, and I
> think updating the segment to actually keep the stop value when an accurate
> seek is requested for any streams is desirable, not sure what to do / how to
> reproduce the lag that you observe Edward ?

I wasn't complaining about the stop value fix. That's totally needed and correct :)

OK, since ACCURATE was not handled before, I won't consider any weird behaviour when used as a regression. I'll do a final check/review today and if nothing earth-shattering pops up I'll commit it.
Comment 52 Edward Hervey 2014-06-12 12:46:19 UTC
Rebased and pushed. Thanks !

commit 79c13b713eedbe99ac8c1de496c6a137ebef8a3a
Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Date:   Fri May 30 16:52:09 2014 +0200

    tsdemux: implement proper seeking for h264 streams.
    
    Co-Authored by: Thibault Saunier <tsaunier@gnome.org>
    
    From a high level perspective, the new process for seeking h264
    streams is as follows:
    
    1) Rewind the stream until we find the first I-slice of a frame,
       and mark its offset in the stream.
    2) Rewind the stream until we find SPS and PPS informations,
       to make sure the subsequent parser is up to date.
    3) Accumulate optionnal SEI NAL units on the way.
    4) Push the SPS, PPS and SEI units before the new keyframe.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=675132