[Bug 621897] [oggdemux] reports wrong duration
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Fri Sep 16 04:43:50 PDT 2011
https://bugzilla.gnome.org/show_bug.cgi?id=621897
GStreamer | gst-plugins-base | git
--- Comment #16 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2011-09-16 11:43:45 UTC ---
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..
--
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.
More information about the gstreamer-bugs
mailing list