[Bug 675132] tsdemux: implement proper seeking with binary search and keyframe detection

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=675132
  GStreamer | gst-plugins-bad | git

--- Comment #22 from Mathieu Duponchelle <mathieu.duponchelle at epitech.eu> 2014-03-20 17:38:12 UTC ---
(In reply to comment #21)
> Review of attachment 272448 [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 ?

> 
> 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.