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 738570 - flvdemux: Fix support for seeking flags
flvdemux: Fix support for seeking flags
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-15 09:58 UTC by Jan Alexander Steffens (heftig)
Modified: 2015-08-16 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Archive containing test files (1014.27 KB, application/x-compressed-tar)
2014-10-15 10:04 UTC, Jan Alexander Steffens (heftig)
  Details
patch 01/02 (4.79 KB, patch)
2014-11-12 13:11 UTC, Jan Alexander Steffens (heftig)
none Details | Review
patch 02/02 (1.68 KB, patch)
2014-11-12 13:12 UTC, Jan Alexander Steffens (heftig)
none Details | Review
patch (6.02 KB, patch)
2015-03-16 14:05 UTC, Jan Alexander Steffens (heftig)
committed Details | Review

Description Jan Alexander Steffens (heftig) 2014-10-15 09:58:00 UTC
flvdemux seems unable to seek accurately. When seeking a playbin playing a FLV file with a h.264 stream, seeks always go to key frames even though FLAG_ACCURATE is set.

Test files follow.
Comment 1 Jan Alexander Steffens (heftig) 2014-10-15 10:04:01 UTC
Created attachment 288572 [details]
Archive containing test files

test.flv and test.mp4 contain identical h.264 streams with a GOP size of 5 seconds.

The FLV file can be sought in in 5-second increments only.
The MP4 file can be sought in accurately.
Comment 2 Sebastian Dröge (slomo) 2014-10-20 10:22:55 UTC
Are you setting FLAG_KEYUNIT? If not it shouldn't go to keyframes even without FLAG_ACCURATE.
Comment 3 Jan Alexander Steffens (heftig) 2014-10-20 10:26:35 UTC
No, I don't.

gst-play-1.0 --interactive also shows the issue.
Comment 4 Sebastian Dröge (slomo) 2014-10-20 11:31:45 UTC
just to prevent confusion: without FLAG_KEYUNIT the demuxer is supposed to start with a keyframe too obviously... but the segment will be adjusted so that the first frame within the segment is the seek position, and not the keyframe position. With the FLAG_KEYUNIT the first frame within the segment will be the keyframe, not the seek position
Comment 5 Jan Alexander Steffens (heftig) 2014-11-12 13:11:26 UTC
Created attachment 290510 [details] [review]
patch 01/02
Comment 6 Jan Alexander Steffens (heftig) 2014-11-12 13:12:48 UTC
Created attachment 290511 [details] [review]
patch 02/02

Seems to work when using gst-launch-1.0 with navseek. gst-play-1.0 --interactive does KEY_UNIT seeks, which also still work.
Comment 7 Tim-Philipp Müller 2015-01-01 16:40:51 UTC
Thanks for working on this.

The first patch looks mostly fine to me.

The second patch looks a bit weird to me. Not wrong necessarily, it's just that we should really never generate a segment event start position just by 'locking on' the first timestamp, we should always generate a seek event based on the seek request, possibly adjusted as per the keyframe position, but not anything else. I suspect what this code (ca. line 1146 in parse_audio_tag and later for video) is supposed to do is to care for a non-0 based timeline. Perhaps this needs to be done differently, or the seek handlers should just generate the new segment event unconditionally in all cases.
Comment 8 Jan Alexander Steffens (heftig) 2015-01-20 09:20:38 UTC
I don't have time to delve deeper into this matter right now. Can the fixes we already have be applied?
Comment 9 Tim-Philipp Müller 2015-01-20 09:35:05 UTC
I'd rather not increase the WTF-factor of the code.
Comment 10 Jan Alexander Steffens (heftig) 2015-03-16 14:05:12 UTC
Created attachment 299509 [details] [review]
patch

Well then, one attempt at reducing the WTF factor.

Used test commands:

gst-launch-1.0 playbin uri=file://$PWD/test.flv video-sink="navseek ! autovideosink"
(does normal seeks)


gst-play-1.0 --interactive test.flv
(does KEY_UNIT seeks)
Comment 11 Jan Alexander Steffens (heftig) 2015-06-22 08:38:03 UTC
Tim hasn't found any time to review the patch. Is there anyone else who can?
Comment 12 Nicolas Dufresne (ndufresne) 2015-06-22 13:38:27 UTC
Review of attachment 299509 [details] [review]:

Looks good in general. I'm concern about the segment because start == time. But at the same time, this won't make anything worst then it was before. I would suggest merging. Setting as reviewed, so see if there is objection (or I missed something obvious).

::: gst/flv/gstflvdemux.c
@@ +2563,3 @@
       }
 
+      if (demux->segment.flags & GST_SEGMENT_FLAG_SEGMENT) {

Hmm, indeed.

@@ +2647,2 @@
         /* Adjust the segment so that the keyframe fits in */
+        segment->start = segment->time = time;

I can't find if we properly offset the start/stop later to compensate the DTS shift. In that case, time will be a little behind start (stream time will start from zero, while TS won't match streamtime).
Comment 13 Jan Alexander Steffens (heftig) 2015-06-29 09:01:53 UTC
IIRC this is the last piece of the puzzle needed to accurately timeshift RTMP streams.
Comment 14 Nicolas Dufresne (ndufresne) 2015-07-06 14:36:59 UTC
Thanks, this patch also falls in the "can't be worst" considering the code was not event checking the flags from the right values. My reading of it seems to indicate it's right, or at least clearly the right direction.

commit 439f98ed9a31c836da97176b58663a4a8fe64f0b
Author: Jan Alexander Steffens (heftig) <jsteffens@make.tv>
Date:   Wed Nov 12 12:08:58 2014 +0100

    flvdemux: Handle seek flags properly
    
    Allows for non-keyframe seeks.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738570