Review of attachment 267276 [details] [review]:

::: gst/matroska/matroska-demux.c
@@ +2188,2 @@
 finish:
+  if (keyunit && (cur_type != GST_SEEK_TYPE_NONE)) {

I think this part is not correct, if the keyunit flag is set we should move to a keyunit.

@@ +2227,2 @@
   /* update some (segment) state */
+  if (cur_type != GST_SEEK_TYPE_NONE)

Also here

Shouldn't the key unit be ignored in case of an undefined position?

If cur_type is set to _TYPE_NONE we should not move current position regardless of the value of the keyunit flag or have we misunderstood something?

I'm not sure, the docs don't really define that. But I would expect it to go to the nearest keyframe. If you don't want that you would not set the flag :)

According to GStreamer 1.0 Core Reference Manual:

gst_event_new_seek()
"...
A type of GST_SEEK_TYPE_NONE means that the position should not be updated.
..."

Are we missing something here?

See docs/design/part-seeking.txt:

2. GST_SEEK_FLAG_KEY_UNIT:

If the KEY_UNIT flag is specified, the demuxer/parser should adjust the
segment start to the position of the key frame closest to the requested
seek position and then start pushing out data from there. The nearest
key frame may be before or after the requested seek position, but many
implementations will only look for the closest keyframe before the
requested position.




There's some ambiguity here. SEEK_TYPE_NONE says "do not update position", however KEY_UNIT says "position should be adjusted to closest key unit". It's not clear to me which of the two would take preference here, but I think it is more powerful if in this case the demuxer would go to the nearest keyframe according to the current position. If you don't want that you would not set the KEY_UNIT flag on the seek, and then it wouldn't go to the nearest keyframe for the current position but instead stay exactly at the current position.

Does that make sense to you?

I would not be surprise though if by accident, when the flush flag is present, the demuxer endup stepping to the next frame. Also, I would be surprise if all demuxer take the time to operate a seek to effectively find the closest (rather then the closest coming after). Something we can improve of course.

You're talking about implementation :) I'm sure many are not behaving correctly...


But first of all we should try to understand what the expected and intended behaviour according to the API is. I think we should consider the flags even if the seek type is NONE (especially FLUSH flag of course). And well, the KEY_UNIT flag only says to a close key unit, so could be before or after. You can define if it should be before or after by using the two SNAP flags (which of course also first need to be implemented properly in the demuxer).

I understand your point of view. However, the documentation should be more clear about what seek flags are prioritized , the combination of the flags etc.

I'll try to solve the problem in gst-rtsp-server and make sure that KEY_INIT flag is not set while seek with SEEK_TYPE_NONE is about to be performed.

Thank you!

commit 450b9d0a14fa8e727c339c02e0b09a2a5969b4c7
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Thu Feb 6 09:48:05 2014 +0100

    media: only set keyframe flag when modifying start
    
    Only set the keyframe flag when we modify the start position. The
    keyframe flag should probably be ignored when no change is requested but
    until we can claim this is all documented properly and all demuxer
    implement this, avoid setting the flag.
    
    See also https://bugzilla.gnome.org/show_bug.cgi?id=723075

Wim, can we close this then and everything's correct in matroskademux?

Seems to work, please reopen if there is any new issue around this.