GNOME Bugzilla – Bug 647856
[oggmux] Assumes that the first buffer can be used to detect the stream type
Last modified: 2011-04-16 11:00:57 UTC
This is not true for VP8 (at least), instead the first buffer of the stream-header caps field should be used here. The VP8 ogg headers are not pushed downstream at all, the first packet is a data packet. gst-launch-0.10 videotestsrc num-buffers=100 ! vp8enc ! oggmux ! filesink location=test.ogg Setting pipeline to PAUSED ... Pipeline is PREROLLING ... Redistribute latency... 0:00:02.006371347 21990 0x1ee57c0 ERROR oggmux gstoggmux.c:854:gst_ogg_mux_queue_pads:0x1edec00 mapper didn't recognise input stream (pad caps: (NULL))
Muxing still works but I wouldn't be surprised if something else is still wrong because the code seems to assume that header packets are always part of the stream.
Created attachment 186018 [details] [review] Fallback to headers in caps to determine stream type
Review of attachment 186018 [details] [review]: ::: ext/ogg/gstoggmux.c @@ +852,3 @@ + /* Use headers in caps, if any; this will allow us to be resilient to + starting streams on the fly, and some streams (like VP8 at least) do + not send headers packets, as other muxers don't expect/need them. */ I would always use the streamheader buffers in any case and only fallback to the in-stream header buffers if no streamheaders are available (does this happen at all?). IIRC oggmux is dropping in-stream header buffers anyway and uses them from the caps. @@ +863,3 @@ + && G_VALUE_TYPE (streamheader) == GST_TYPE_ARRAY) { + const GArray *bufarr = g_value_peek_pointer (streamheader); + const GValue *bufval = &g_array_index (bufarr, GValue, 0); Use the gst_value_array* API here @@ +866,3 @@ + if (G_VALUE_TYPE (bufval) == GST_TYPE_BUFFER) { + ogg_packet packet; + GstBuffer *buf = g_value_peek_pointer (bufval); gst_value_get_buffer()
Created attachment 186020 [details] [review] Comments addressed
Review of attachment 186020 [details] [review]: Looks good ::: ext/ogg/gstoggmux.c @@ +864,3 @@ + const GstBuffer *buf = gst_value_get_buffer (first_element); + if (buf) { + ogg_packet packet; you're shadowing the packet variable from the outer scope here. Might confuse people but it's not a problem of course ;)
I've moved most of this code into a new gst_ogg_stream_setup_map_from_caps_headers() function, which makes the logic here clearer IMHO, and also sprinkled some more debug logging for miscellaneous error/failure cases. Hope I didn't mess anything up in the process: commit 55e767b6328e6cc0db7bf6ac23cdf4e0eab537e6 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Apr 15 13:36:39 2011 +0100 oggmux: prefer headers from caps to determine stream type Ogg mandates the first header packet must determine a stream's type. However, some streams (such as VP8) do not include such a header when muxed in other containers, and thus do not include this header as a buffer, but only in caps. We thus use headers from caps when available to determine a new stream's type. https://bugzilla.gnome.org/show_bug.cgi?id=647856 Thanks for the patch!
Comment on attachment 186020 [details] [review] Comments addressed Committed, but not in this form, so marking as obsolete.