Duration is correctly reported if I manually download and play the file locally

This is simply caused by wrong bitrate information in the included a/v streams.

(In reply to comment #2)
> This is simply caused by wrong bitrate information in the included a/v streams.

Still, I'm curious how mplayer manages to correctly report the duration ;)

Via HTTP or local file? For a local file you could get the end time of the last Ogg page.

via HTTP

I guess we should calculate our own bitrate estimates instead of always relying on the values from the stream.

This should fix this problem:

http://git.collabora.co.uk/?p=user/vincent/gst-plugins-base;a=shortlog;h=refs/heads/seeking

(In reply to comment #7)
> This should fix this problem:
> 
> http://git.collabora.co.uk/?p=user/vincent/gst-plugins-base;a=shortlog;h=refs/heads/seeking

Yes indeed, thanks for your work on this! Can we get this in master after the freeze?

Branch mentioned above handles quite some of these cases/bugs; some review comments (mainly on the first commit, unaffected by second commit):

* a few changes seem included that may be unrelated to the (duration/push seek) enhancements, e.g. the discont marking in gst_ogg_pad_stream_out, or dropping a _free in gst_ogg_demux_deactivate_current_chain.  If so, they best go in a separate commit, rather than drowned in this one.

* the QUERY_SEEKING case in gst_ogg_pad_src_query now has 2 identical if (...) tests in a row

* time info in debug statements is provided here with %f, customary is to use GST_TIME_FORMAT and GST_TIME_ARGS

* some debug only segments might do with #ifndef GST_DISABLE_GST_DEBUG

* don't know what gst-indent is (not) doing here, since it does not "complain" but there are quite some code style issues, e.g. many lines exceeding 80 chars; comments, debug strings, and also code in gst_ogg_demux_get_duration_push, or no blank line following declaration in e.g. gst_ogg_demux_check_duration_push 

* also // comments left here or there (along with TODO)

* code chunk added to gst_ogg_demux_chain_peer to determine duration might go nicer in gst_ogg_demux_submit_packet where new segment event is setup, as it is also an opportunity to determine the chain->segment_stop there

* comment in gst_ogg_pad_submit_page about "queue blocking" and eos handler is unclear; sending a flushing seek upstream should take care of queue in between (see also later)

* in gst_ogg_demux_perform_seek_push:
- various state variables are manipulated (e.g. push_state) without any locking, though they might be accessed by streaming thread in the meantime without being setup fully/properly.  Seems best to do as little possible to these at that time, and only send seek upstream and perform remaining stuff when that comes in (newsegment).
- also, validity of all data may not be checked (already push_start_time by then ? etc)
- it may be easier to keep the seek event around rather than all individual bits and pieces of it (i.e. flags, start etc)
- worse, using the original seek event's start_type, stop_type, etc in an upstream BYTE seek later on is wrong; the upstream BYTE seeks always simply target a specific offset
- moreover, the only kind of seek that is really handled properly in push mode is a flushing seek with seek_type_set start time (and usually open-ended stop time), cfr other demuxers or baseparse.  So, better check accordingly and only seek if in such a case.  In particular, only handling flushing cases may help (a.o.) with the queue situation above.
- [minor nitpick] on the other hand, checking for a PUSH_DURATION in progress should not be needed, i.e. it should handle being interrupted (particularly by flushing seek), though that may need some other variable caution

Created attachment 193873 [details] [review]
oggdemux: do not propagate discontinuities in sparse streams

The first packet of a sparse stream may arrive after an initial
delay in the stream. If ogg_stream_packetout reports a discontinuity
in a sparse stream, do not propagate it to other streams in the
chain unnecessarily.

Created attachment 193874 [details] [review]
oggdemux: implement push mode seeking

This patch implements seeking in push mode (eg, over the net)
in Ogg, using the double bisection method.
As a side effect, it also fixes duration determination of network
streams, by seeking to the end to check the actual duration.

Known issues:
- Getting an EOS while seeking stops the streaming task, I can't
  find a way to prevent this (eg, by issuing a seek in the event
  handler).
- Seeking with playbin2 fails for streams with subtitles, something
  causes the multiqueue to fill up (also with the 'sparse' patch).
- Seeking is slow on slow links - byte ranges guesses could be
  made better, decreasing the number of required requests
- If no granule position is found in the last 64 KB of a stream,
  duration will be left unknown (should be pretty rare)

Thanks for the comments. This had completely fallen off the radar, sorry.

I split the discont patch, but the free change is part of the seeking patch - an ended chain was freed, but we could (at some point, the current patch disables seeking on chained streams) seek back to it.

Created attachment 196391 [details] [review]
oggdemux: implement push mode seeking

This patch implements seeking in push mode (eg, over the net)
in Ogg, using the double bisection method.
As a side effect, it also fixes duration determination of network
streams, by seeking to the end to check the actual duration.

Known issues:
- Getting an EOS while seeking stops the streaming task, I can't
  find a way to prevent this (eg, by issuing a seek in the event
  handler).
- Seeking twice in a VERY short succession with playbin2 fails
  for streams with subtitles, we end up pushing in a dataqueue
  which is flushing. Rare in normal use AFAICT.
- Seeking is slow on slow links - byte ranges guesses could be
  made better, decreasing the number of required requests
- If no granule position is found in the last 64 KB of a stream,
  duration will be left unknown (should be pretty rare)

Rebased. Seeking failure with subtitles seem to be rare in normal use (eg, with Totem), change message accordingly.

Created attachment 196483 [details] [review]
oggdemux: fix wedge when seeking twice quickly in push mode

This could happen when testing with navseek, and pressing
right and left at roughly the same time. The current chain
is temporarily moved away, and this caused the flush events
not to be sent to the source pads, which would cause the
data queues downstream to reject incoming data after the
seek, and shut down, wedging the pipeline.

Now, I can't really decide whether this is a nasty steaming
hack or a good fix, but it certainly does fix the issue, and
does not seem to break anything else so far.

This looks quite nifty, seems to work well for me, at least with the stream
above (even if a bit slow, but that's the server I think, would be nice to give
the user some feedback that something is happening though).


First, a couple of style nitpicks, if I may:

 - in assignments like a = b == c; please do:
        a = (b == c) ,
    also when using the ? operator
    (e.g. close_enough = ...)

 - try to aovid comments "inside" code, e.g.
    /*stop */ twice in submit_page()

 - please leave an empty line between a variable
   declaration and code (even if gst-indent doesn't
   enforce it)

 - avoid comments at the end of long lines, because
   this seems to prevent gst-indent from wrapping
   them (which arguably is a bug, but still), e.g. in
   gst_ogg_demux_get_duration_push(). Put comments
   in a separate line before the code in such cases.


Then:

 - GST_PUSH_{LOCK,UNLOCK}: why not just use the
   sinkpad's stream lock? (which is taken by default
   anyway) (just needs extra care in functions that
   might be called from non-streaming thread such
   as src event/query funcs)

 - GST_PUSH_{LOCK,UNLOCK}: use TRACE debug
   level if you keep the lock

 - maybe use gst_ogg_stream_get_media_type()
   for gst_structure_get_name (gst_caps_get_structure (pad->map.caps, 0))
   (in some GST_DEBUG_OBJECT statement)

 - GST_PUSH_LOCK/UNLOCK should probably be
   inside of
any if (!ogg->pullmode) instead of
   outside of it? (see e.g. submit_packet)

 - in gst_ogg_demux_seek_back_after_push_duration_check_unlock():
    why does the seek back seek to byte 1 instead of byte 0?

 - I'd skip remove the #ifndef GST_DISABLE_GST_DEBUG
    in _submit_page(), there's no real work done in there to
    warrant that besides that if() - it affects readability imho

 - would it be possible to move that huge push-related block
   in submit_page() into a separate function?

 - in submit_page(), should probably post an error message
    using GST_ELEMENT_ERROR when we (if we) return
    GST_FLOW_ERROR (return res ? GST_FLOW_OK : GST_FLOW_ERROR)
    (didn't check if it's propagated upstream though)

 - I wonder if pushing a seek event upstream from the streaming
   thread is really kosher, or if it just happens to work in this case..

Created attachment 196751 [details] [review]
oggdemux: implement push mode seeking

This patch implements seeking in push mode (eg, over the net)
in Ogg, using the double bisection method.
As a side effect, it also fixes duration determination of network
streams, by seeking to the end to check the actual duration.

Known issues:
- Getting an EOS while seeking stops the streaming task, I can't
  find a way to prevent this (eg, by issuing a seek in the event
  handler).
- Seeking twice in a VERY short succession with playbin2 fails
  for streams with subtitles, we end up pushing in a dataqueue
  which is flushing. Rare in normal use AFAICT.
- Seeking is slow on slow links - byte ranges guesses could be
  made better, decreasing the number of required requests
- If no granule position is found in the last 64 KB of a stream,
  duration will be left unknown (should be pretty rare)

Thanks much for the review:

(In reply to comment #16)
> This looks quite nifty, seems to work well for me, at least with the stream
> above (even if a bit slow, but that's the server I think, would be nice to give
> the user some feedback that something is happening though).

That's out of the purview of a demuxer, no ? What would you suggest ?

[nits picked and snipped]

>  - GST_PUSH_{LOCK,UNLOCK}: why not just use the
>    sinkpad's stream lock? (which is taken by default
>    anyway) (just needs extra care in functions that
>    might be called from non-streaming thread such
>    as src event/query funcs)

I actually tried that a couple days back. It did not go well, there
was a deadlock, which I can't recall the details of, but it did make
some kind of sense from the backtrace IIRC.
It would also add more contention, as the stream lock is held for
longer.

>  - maybe use gst_ogg_stream_get_media_type()
>    for gst_structure_get_name (gst_caps_get_structure (pad->map.caps, 0))
>    (in some GST_DEBUG_OBJECT statement)

Very good point, I'd done that code way before I added _get_media_type :D

>  - GST_PUSH_LOCK/UNLOCK should probably be
>    inside of
any if (!ogg->pullmode) instead of
>    outside of it? (see e.g. submit_packet)

It usually is, except in the cases where the test is compound with
another push_ member, which needs locking. I could split the test,
and lock in the middle, but it would add nesting. I think it's OK
as it is, but I don't mind doing this if you think it's better.

>  - in gst_ogg_demux_seek_back_after_push_duration_check_unlock():
>     why does the seek back seek to byte 1 instead of byte 0?

Hysterical raisins. IIRC I'd have a full stream reinit. Now back
to 0 as expected.

>  - in submit_page(), should probably post an error message
>     using GST_ELEMENT_ERROR when we (if we) return
>     GST_FLOW_ERROR (return res ? GST_FLOW_OK : GST_FLOW_ERROR)
>     (didn't check if it's propagated upstream though)

Right, I've added an error, not too sure it's the best (resource seek
error).

>  - I wonder if pushing a seek event upstream from the streaming
>    thread is really kosher, or if it just happens to work in this case..

It certainly happens to work. avidemux (which we all know is such
a good example to follow) does it, and it's a used enough format
that we'd have had bugs if it didn't work (I did not check if we do,
so there may be :P).

Other points not quoted are fixed.

Thanks

> > [would be nice to give the user feedback that something is happening]
>
> That's out of the purview of a demuxer, no ? What would you suggest ?

Don't know yet, something to do some other time, not really essential to this bug.


> >  why not just use the sinkpad's stream lock?
> 
> I actually tried that a couple days back. It did not go well, there
> was a deadlock, which I can't recall the details of, but it did make
> some kind of sense from the backtrace IIRC.
> It would also add more contention, as the stream lock is held for
> longer.

In the normal case (streaming) there should be no contention at all really, because only the streaming thread takes/releases the lock. But it's no biggie anyway, can always be changed later. Was just wondering if there was a reason or not.


> >  - GST_PUSH_LOCK/UNLOCK should probably be
> >    inside of
any if (!ogg->pullmode) instead of
> >    outside of it? (see e.g. submit_packet)
> 
> It usually is, except in the cases where the test is compound with
> another push_ member, which needs locking. I could split the test,
> and lock in the middle, but it would add nesting. I think it's OK
> as it is, but I don't mind doing this if you think it's better.

Ok, I guess that makes sense. I think there's seomthing to be said for really keeping all the push code stuff 100% in push mode code paths, but I see your point about indentation. Let's keep it as it is for now then.

Ok, let's do this!

commit 89fc5b4bd8714d4d666d5cee8139495712389e20
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Wed Sep 14 12:23:19 2011 +0100

    oggdemux: fix wedge when seeking twice quickly in push mode
    
    This could happen when testing with navseek, and pressing
    right and left at roughly the same time. The current chain
    is temporarily moved away, and this caused the flush events
    not to be sent to the source pads, which would cause the
    data queues downstream to reject incoming data after the
    seek, and shut down, wedging the pipeline.
    
    Now, I can't really decide whether this is a nasty steaming
    hack or a good fix, but it certainly does fix the issue, and
    does not seem to break anything else so far.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=621897

commit 0173afa38cc6f8a2cb810f99a24ddb1d5d5f4868
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Sat Aug 13 14:18:56 2011 +0100

    oggdemux: implement push mode seeking
    
    This patch implements seeking in push mode (eg, over the net)
    in Ogg, using the double bisection method.
    As a side effect, it also fixes duration determination of network
    streams, by seeking to the end to check the actual duration.
    
    Known issues:
    - Getting an EOS while seeking stops the streaming task, I can't
      find a way to prevent this (eg, by issuing a seek in the event
      handler).
    - Seeking twice in a VERY short succession with playbin2 fails
      for streams with subtitles, we end up pushing in a dataqueue
      which is flushing. Rare in normal use AFAICT.
    - Seeking is slow on slow links - byte ranges guesses could be
      made better, decreasing the number of required requests
    - If no granule position is found in the last 64 KB of a stream,
      duration will be left unknown (should be pretty rare)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=621897


Thanks for your awesome work on this!