[Bug 621897] [oggdemux] reports wrong duration

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #18 from Vincent Penquerc'h <vincent.penquerch at collabora.co.uk> 2011-09-16 17:05:22 UTC ---
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

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