[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