From 9cbe7c1403de3d774dc25bc41fadcd1391de952e Mon Sep 17 00:00:00 2001 From: Monty Montgomery Date: Thu, 21 Jul 2011 17:16:26 -0400 Subject: [PATCH 01/13] vorbisenc: Relax overly-tight jitter tolerances in gstvobisenc vorbisenc currently reacts in a rater draconian fashion if input timestamps are more than 1/2 sample off what it considers ideal. If data is 'too late' it truncates buffers, if it is 'too soon' it completely shuts down encode and restarts it. This is causingvorbisenc to produce corrupt output when encoding data produced by sources with bugs that produce a smple or two of jitter (eg, flacdec) --- ext/vorbis/gstvorbisenc.c | 44 +++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/ext/vorbis/gstvorbisenc.c b/ext/vorbis/gstvorbisenc.c index 9cdf484e15..85d47c3249 100644 --- a/ext/vorbis/gstvorbisenc.c +++ b/ext/vorbis/gstvorbisenc.c @@ -1012,12 +1012,12 @@ gst_vorbis_enc_buffer_check_discontinuous (GstVorbisEnc * vorbisenc, vorbisenc->expected_ts != GST_CLOCK_TIME_NONE && timestamp + duration != vorbisenc->expected_ts) { /* It turns out that a lot of elements don't generate perfect streams due - * to rounding errors. So, we permit small errors (< 1/2 a sample) without + * to rounding errors. So, we permit small errors (< 3 samples) without * causing a discont. */ - int halfsample = GST_SECOND / vorbisenc->frequency / 2; + int threesample = GST_SECOND / vorbisenc->frequency * 3; - if ((GstClockTimeDiff) (timestamp - vorbisenc->expected_ts) > halfsample) { + if ((GstClockTimeDiff) (timestamp - vorbisenc->expected_ts) > threesample) { GST_DEBUG_OBJECT (vorbisenc, "Expected TS %" GST_TIME_FORMAT ", buffer TS %" GST_TIME_FORMAT, GST_TIME_ARGS (vorbisenc->expected_ts), GST_TIME_ARGS (timestamp)); @@ -1135,28 +1135,36 @@ gst_vorbis_enc_chain (GstPad * pad, GstBuffer * buffer) if (vorbisenc->expected_ts != GST_CLOCK_TIME_NONE && timestamp < vorbisenc->expected_ts) { + int threesample = GST_SECOND / vorbisenc->frequency * 3; guint64 diff = vorbisenc->expected_ts - timestamp; guint64 diff_bytes; - GST_WARNING_OBJECT (vorbisenc, "Buffer is older than previous " - "timestamp + duration (%" GST_TIME_FORMAT "< %" GST_TIME_FORMAT - "), cannot handle. Clipping buffer.", - GST_TIME_ARGS (timestamp), GST_TIME_ARGS (vorbisenc->expected_ts)); + /* Don't freak out on tiny jitters; use the same < 3 sample + tolerance as in the discontinuous detection */ + if ((GstClockTimeDiff) (vorbisenc->expected_ts - timestamp) > threesample) { - diff_bytes = - GST_CLOCK_TIME_TO_FRAMES (diff, - vorbisenc->frequency) * vorbisenc->channels * sizeof (gfloat); - if (diff_bytes >= GST_BUFFER_SIZE (buffer)) { - gst_buffer_unref (buffer); - return GST_FLOW_OK; + GST_WARNING_OBJECT (vorbisenc, "Buffer is older than previous " + "timestamp + duration (%" GST_TIME_FORMAT "< %" GST_TIME_FORMAT + "), cannot handle. Clipping buffer.", + GST_TIME_ARGS (timestamp), GST_TIME_ARGS (vorbisenc->expected_ts)); + + diff_bytes = + GST_CLOCK_TIME_TO_FRAMES (diff, + vorbisenc->frequency) * vorbisenc->channels * sizeof (gfloat); + if (diff_bytes >= GST_BUFFER_SIZE (buffer)) { + gst_buffer_unref (buffer); + return GST_FLOW_OK; + } + buffer = gst_buffer_make_metadata_writable (buffer); + GST_BUFFER_DATA (buffer) += diff_bytes; + GST_BUFFER_SIZE (buffer) -= diff_bytes; + + if (GST_BUFFER_DURATION_IS_VALID (buffer)) + GST_BUFFER_DURATION (buffer) -= diff; } - buffer = gst_buffer_make_metadata_writable (buffer); - GST_BUFFER_DATA (buffer) += diff_bytes; - GST_BUFFER_SIZE (buffer) -= diff_bytes; + /* adjust the input timestamp in either case */ GST_BUFFER_TIMESTAMP (buffer) += diff; - if (GST_BUFFER_DURATION_IS_VALID (buffer)) - GST_BUFFER_DURATION (buffer) -= diff; } if (gst_vorbis_enc_buffer_check_discontinuous (vorbisenc, timestamp, From 4e9508e2ec9db9a7eb8cccf1b8fef8bd80d1d779 Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Mon, 22 Aug 2011 14:55:59 +0100 Subject: [PATCH 02/13] oggdemux: do not ignore sparse streams' start time But do not wait for them either, if we don't have a packet for them. https://bugzilla.gnome.org/show_bug.cgi?id=657062 --- ext/ogg/gstoggdemux.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index ff05c82120..2e349a22e1 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -679,13 +679,18 @@ gst_ogg_demux_collect_start_time (GstOggDemux * ogg, GstOggChain * chain) for (i = 0; i < chain->streams->len; i++) { GstOggPad *pad = g_array_index (chain->streams, GstOggPad *, i); - if (pad->map.is_sparse) + if (pad->map.is_skeleton) continue; /* can do this if the pad start time is not defined */ + GST_DEBUG_OBJECT (ogg, "Pad %08x (%s) start time is %" GST_TIME_FORMAT, + pad->map.serialno, gst_ogg_stream_get_media_type (&pad->map), + GST_TIME_ARGS (pad->start_time)); if (pad->start_time == GST_CLOCK_TIME_NONE) { - start_time = G_MAXUINT64; - break; + if (!pad->map.is_sparse) { + start_time = G_MAXUINT64; + break; + } } else { start_time = MIN (start_time, pad->start_time); } From 8a752e44e20fa865db29316af0f946c50666ed4a Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Mon, 22 Aug 2011 14:56:38 +0100 Subject: [PATCH 03/13] oggdemux: do not skip sparse streams when determining start times This fixes demuxing of streams containing only sparse streams, which would cause an infinite loop in _read_end_chain. https://bugzilla.gnome.org/show_bug.cgi?id=657062 --- ext/ogg/gstoggdemux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 2e349a22e1..e5c047396d 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -2830,7 +2830,7 @@ gst_ogg_demux_read_end_chain (GstOggDemux * ogg, GstOggChain * chain) for (i = 0; i < chain->streams->len; i++) { GstOggPad *pad = g_array_index (chain->streams, GstOggPad *, i); - if (pad->map.is_sparse) + if (pad->map.is_skeleton) continue; if (pad->map.serialno == ogg_page_serialno (&og)) { From 2301f1806fcef40c502560304d2f17641a2dea53 Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Wed, 17 Aug 2011 17:09:44 +0100 Subject: [PATCH 04/13] oggmux: add skeleton write support Version written is 3.0 Base times are left empty for now. Content-Type should be the MIME type of the stream. It is set to the GStreamer media type for now, which is probably the same for the streams oggmux supports. https://bugzilla.gnome.org/show_bug.cgi?id=563251 --- ext/ogg/gstoggmux.c | 148 +++++++++++++++++++++++++++++++++++++++++--- ext/ogg/gstoggmux.h | 3 + 2 files changed, 142 insertions(+), 9 deletions(-) diff --git a/ext/ogg/gstoggmux.c b/ext/ogg/gstoggmux.c index f039efca6e..5a5a23e7a4 100644 --- a/ext/ogg/gstoggmux.c +++ b/ext/ogg/gstoggmux.c @@ -41,6 +41,7 @@ #include #include +#include #include #include "gstoggmux.h" @@ -83,12 +84,15 @@ enum #define DEFAULT_MAX_DELAY G_GINT64_CONSTANT(500000000) #define DEFAULT_MAX_PAGE_DELAY G_GINT64_CONSTANT(500000000) #define DEFAULT_MAX_TOLERANCE G_GINT64_CONSTANT(40000000) +#define DEFAULT_SKELETON FALSE + enum { ARG_0, ARG_MAX_DELAY, ARG_MAX_PAGE_DELAY, - ARG_MAX_TOLERANCE + ARG_MAX_TOLERANCE, + ARG_SKELETON }; static GstStaticPadTemplate src_factory = GST_STATIC_PAD_TEMPLATE ("src", @@ -211,6 +215,11 @@ gst_ogg_mux_class_init (GstOggMuxClass * klass) "Maximum timestamp difference for maintaining perfect granules", 0, G_MAXUINT64, DEFAULT_MAX_TOLERANCE, (GParamFlags) G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + g_object_class_install_property (gobject_class, ARG_SKELETON, + g_param_spec_boolean ("skeleton", "Skeleton", + "Whether to include a Skeleton track", + DEFAULT_SKELETON, + (GParamFlags) G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); gstelement_class->change_state = gst_ogg_mux_change_state; @@ -1121,16 +1130,103 @@ gst_ogg_mux_set_header_on_caps (GstCaps * caps, GList * buffers) } static void -create_header_packet (ogg_packet * packet, GstBuffer * buf, GstOggPadData * pad) +gst_ogg_mux_create_header_packet_with_flags (ogg_packet * packet, + GstBuffer * buf, gboolean bos, gboolean eos) { packet->packet = GST_BUFFER_DATA (buf); packet->bytes = GST_BUFFER_SIZE (buf); packet->granulepos = 0; /* mark BOS and packet number */ - packet->b_o_s = (pad->packetno == 0); - packet->packetno = pad->packetno++; + packet->b_o_s = bos; /* mark EOS */ - packet->e_o_s = 0; + packet->e_o_s = eos; +} + +static void +gst_ogg_mux_create_header_packet (ogg_packet * packet, GstBuffer * buf, + GstOggPadData * pad) +{ + gst_ogg_mux_create_header_packet_with_flags (packet, buf, pad->packetno == 0, + 0); + packet->packetno = pad->packetno++; +} + +static void +gst_ogg_mux_submit_skeleton_header_packet (GstOggMux * mux, + ogg_stream_state * os, GstBuffer * buf, gboolean bos, gboolean eos) +{ + ogg_packet packet; + gst_ogg_mux_create_header_packet_with_flags (&packet, buf, bos, eos); + ogg_stream_packetin (os, &packet); + gst_buffer_unref (buf); +} + +static void +gst_ogg_mux_make_fishead (GstOggMux * mux, ogg_stream_state * os) +{ + GstByteWriter bw; + GstBuffer *fishead; + + GST_DEBUG_OBJECT (mux, "Creating fishead"); + + fishead = gst_buffer_new_and_alloc (64); + gst_byte_writer_init_with_buffer (&bw, fishead, TRUE); + gst_byte_writer_put_string_utf8 (&bw, "fishead"); + gst_byte_writer_put_int16_le (&bw, 3); /* version major */ + gst_byte_writer_put_int16_le (&bw, 0); /* version minor */ + gst_byte_writer_put_int64_le (&bw, 0); /* presentation time numerator */ + gst_byte_writer_put_int64_le (&bw, 1000); /* ...and denominator */ + gst_byte_writer_put_int64_le (&bw, 0); /* base time numerator */ + gst_byte_writer_put_int64_le (&bw, 1000); /* ...and denominator */ + gst_byte_writer_fill (&bw, ' ', 20); /* UTC time */ + g_assert (gst_byte_writer_get_pos (&bw) == GST_BUFFER_SIZE (fishead)); + gst_ogg_mux_submit_skeleton_header_packet (mux, os, fishead, 1, 0); +} + +static void +gst_ogg_mux_byte_writer_put_string_utf8 (GstByteWriter * bw, const char *s) +{ + gst_byte_writer_put_data (bw, (const guint8 *) s, strlen (s)); +} + +static void +gst_ogg_mux_make_fisbone (GstOggMux * mux, ogg_stream_state * os, + GstOggPadData * pad) +{ + GstByteWriter bw; + + GST_DEBUG_OBJECT (mux, + "Creating %s fisbone for serial %08x", + gst_ogg_stream_get_media_type (&pad->map), pad->map.serialno); + + gst_byte_writer_init (&bw); + gst_byte_writer_put_string_utf8 (&bw, "fisbone"); + gst_byte_writer_put_int32_le (&bw, 44); /* offset to message headers */ + gst_byte_writer_put_uint32_le (&bw, pad->map.serialno); + gst_byte_writer_put_uint32_le (&bw, pad->map.n_header_packets); + gst_byte_writer_put_uint64_le (&bw, pad->map.granulerate_n); + gst_byte_writer_put_uint64_le (&bw, pad->map.granulerate_d); + gst_byte_writer_put_uint64_le (&bw, 0); /* base granule */ + gst_byte_writer_put_uint32_le (&bw, pad->map.preroll); + gst_byte_writer_put_uint8 (&bw, pad->map.granuleshift); + gst_byte_writer_fill (&bw, 0, 3); /* padding */ + /* message header fields - MIME type for now */ + gst_ogg_mux_byte_writer_put_string_utf8 (&bw, "Content-Type: "); + gst_ogg_mux_byte_writer_put_string_utf8 (&bw, + gst_ogg_stream_get_media_type (&pad->map)); + gst_ogg_mux_byte_writer_put_string_utf8 (&bw, "\r\n"); + + gst_ogg_mux_submit_skeleton_header_packet (mux, os, + gst_byte_writer_reset_and_get_buffer (&bw), 0, 0); +} + +static void +gst_ogg_mux_make_fistail (GstOggMux * mux, ogg_stream_state * os) +{ + GST_DEBUG_OBJECT (mux, "Creating fistail"); + + gst_ogg_mux_submit_skeleton_header_packet (mux, os, + gst_buffer_new_and_alloc (0), 0, 1); } /* @@ -1148,6 +1244,8 @@ gst_ogg_mux_send_headers (GstOggMux * mux) GList *hbufs, *hwalk; GstCaps *caps; GstFlowReturn ret; + ogg_page page; + ogg_stream_state skeleton_stream; hbufs = NULL; ret = GST_FLOW_OK; @@ -1180,7 +1278,6 @@ gst_ogg_mux_send_headers (GstOggMux * mux) GstOggPadData *pad; GstBuffer *buf; ogg_packet packet; - ogg_page page; GstPad *thepad; GstCaps *caps; GstStructure *structure; @@ -1213,7 +1310,7 @@ gst_ogg_mux_send_headers (GstOggMux * mux) } /* create a packet from the buffer */ - create_header_packet (&packet, buf, pad); + gst_ogg_mux_create_header_packet (&packet, buf, pad); /* swap the packet in */ ogg_stream_packetin (&pad->map.stream, &packet); @@ -1245,6 +1342,16 @@ gst_ogg_mux_send_headers (GstOggMux * mux) gst_caps_unref (caps); } + /* The Skeleton BOS goes first - even before the video that went first before */ + if (mux->use_skeleton) { + ogg_stream_init (&skeleton_stream, gst_ogg_mux_generate_serialno (mux)); + gst_ogg_mux_make_fishead (mux, &skeleton_stream); + while (ogg_stream_flush (&skeleton_stream, &page) > 0) { + GstBuffer *hbuf = gst_ogg_mux_buffer_from_page (mux, &page, FALSE); + hbufs = g_list_append (hbufs, hbuf); + } + } + GST_LOG_OBJECT (mux, "creating next headers"); walk = mux->collect->data; while (walk) { @@ -1256,6 +1363,9 @@ gst_ogg_mux_send_headers (GstOggMux * mux) walk = walk->next; + if (mux->use_skeleton) + gst_ogg_mux_make_fisbone (mux, &skeleton_stream, pad); + GST_LOG_OBJECT (mux, "looping over headers for pad %s:%s", GST_DEBUG_PAD_NAME (thepad)); @@ -1263,12 +1373,11 @@ gst_ogg_mux_send_headers (GstOggMux * mux) while (hwalk) { GstBuffer *buf = GST_BUFFER (hwalk->data); ogg_packet packet; - ogg_page page; hwalk = hwalk->next; /* create a packet from the buffer */ - create_header_packet (&packet, buf, pad); + gst_ogg_mux_create_header_packet (&packet, buf, pad); /* swap the packet in */ ogg_stream_packetin (&pad->map.stream, &packet); @@ -1299,6 +1408,21 @@ gst_ogg_mux_send_headers (GstOggMux * mux) g_list_free (pad->map.headers); pad->map.headers = NULL; } + + if (mux->use_skeleton) { + /* flush accumulated fisbones, the fistail must be on a separate page */ + while (ogg_stream_flush (&skeleton_stream, &page) > 0) { + GstBuffer *hbuf = gst_ogg_mux_buffer_from_page (mux, &page, FALSE); + hbufs = g_list_append (hbufs, hbuf); + } + gst_ogg_mux_make_fistail (mux, &skeleton_stream); + while (ogg_stream_flush (&skeleton_stream, &page) > 0) { + GstBuffer *hbuf = gst_ogg_mux_buffer_from_page (mux, &page, FALSE); + hbufs = g_list_append (hbufs, hbuf); + } + ogg_stream_clear (&skeleton_stream); + } + /* hbufs holds all buffers for the headers now */ /* create caps with the buffers */ @@ -1741,6 +1865,9 @@ gst_ogg_mux_get_property (GObject * object, case ARG_MAX_TOLERANCE: g_value_set_uint64 (value, ogg_mux->max_tolerance); break; + case ARG_SKELETON: + g_value_set_boolean (value, ogg_mux->use_skeleton); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); break; @@ -1765,6 +1892,9 @@ gst_ogg_mux_set_property (GObject * object, case ARG_MAX_TOLERANCE: ogg_mux->max_tolerance = g_value_get_uint64 (value); break; + case ARG_SKELETON: + ogg_mux->use_skeleton = g_value_get_boolean (value); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); break; diff --git a/ext/ogg/gstoggmux.h b/ext/ogg/gstoggmux.h index 3edc1f5b51..3db42e7a5b 100644 --- a/ext/ogg/gstoggmux.h +++ b/ext/ogg/gstoggmux.h @@ -127,6 +127,9 @@ struct _GstOggMux pages as delta frames up to the page that has the keyframe */ + + /* whether to create a skeleton track */ + gboolean use_skeleton; }; struct _GstOggMuxClass From 43cb76b1d8afcec7041c90a7899d07b134f2343b Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:28:33 +0100 Subject: [PATCH 05/13] oggstream: set skeleton stream media type to application/x-ogg-skeleton This is to match the typefinder, and to make logs clearer. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/ogg/gstoggstream.c b/ext/ogg/gstoggstream.c index a0b4e687c6..253e963ec4 100644 --- a/ext/ogg/gstoggstream.c +++ b/ext/ogg/gstoggstream.c @@ -1094,7 +1094,7 @@ setup_fishead_mapper (GstOggStream * pad, ogg_packet * packet) pad->is_skeleton = TRUE; pad->is_sparse = TRUE; - pad->caps = gst_caps_new_simple ("none/none", NULL); + pad->caps = gst_caps_new_simple ("application/x-ogg-skeleton", NULL); return TRUE; } From 67a882afe7dbda862ec716230b0d690b3756fa5c Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:29:49 +0100 Subject: [PATCH 06/13] oggstream: include stream type in warnings It makes it easier to work out what's going on. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggstream.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/ext/ogg/gstoggstream.c b/ext/ogg/gstoggstream.c index 253e963ec4..abc5910fda 100644 --- a/ext/ogg/gstoggstream.c +++ b/ext/ogg/gstoggstream.c @@ -139,7 +139,8 @@ gst_ogg_stream_granulepos_to_granule (GstOggStream * pad, gint64 granulepos) } if (mappers[pad->map].granulepos_to_granule_func == NULL) { - GST_WARNING ("Failed to convert granulepos to granule"); + GST_WARNING ("Failed to convert %s granulepos to granule", + gst_ogg_stream_get_media_type (pad)); return -1; } @@ -168,7 +169,8 @@ gst_ogg_stream_granule_to_granulepos (GstOggStream * pad, gint64 granule, } if (mappers[pad->map].granule_to_granulepos_func == NULL) { - GST_WARNING ("Failed to convert granule to granulepos"); + GST_WARNING ("Failed to convert %s granule to granulepos", + gst_ogg_stream_get_media_type (pad)); return -1; } @@ -184,7 +186,8 @@ gst_ogg_stream_granulepos_is_key_frame (GstOggStream * pad, gint64 granulepos) } if (mappers[pad->map].is_key_frame_func == NULL) { - GST_WARNING ("Failed to determine key frame"); + GST_WARNING ("Failed to determine keyframeness for %s granulepos", + gst_ogg_stream_get_media_type (pad)); return FALSE; } @@ -195,7 +198,8 @@ gboolean gst_ogg_stream_packet_is_header (GstOggStream * pad, ogg_packet * packet) { if (mappers[pad->map].is_header_func == NULL) { - GST_WARNING ("Failed to determine header"); + GST_WARNING ("Failed to determine headerness of %s packet", + gst_ogg_stream_get_media_type (pad)); return FALSE; } @@ -206,7 +210,8 @@ gint64 gst_ogg_stream_get_packet_duration (GstOggStream * pad, ogg_packet * packet) { if (mappers[pad->map].packet_duration_func == NULL) { - GST_WARNING ("Failed to determine packet duration"); + GST_WARNING ("Failed to determine %s packet duration", + gst_ogg_stream_get_media_type (pad)); return -1; } From 5d18496a5b0eaede10fb2d687e4bab7e879dddcc Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:32:36 +0100 Subject: [PATCH 07/13] oggdemux: do not try to determine duration of header packets Headers are inherently durationless. Instead, set duration to 0 to avoid increasing tracked granpos, and do not warn about it, since it is totally expected. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index e5c047396d..1af79a298d 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -499,8 +499,13 @@ gst_ogg_demux_chain_peer (GstOggPad * pad, ogg_packet * packet, } /* get timing info for the packet */ - duration = gst_ogg_stream_get_packet_duration (&pad->map, packet); - GST_DEBUG_OBJECT (ogg, "packet duration %" G_GUINT64_FORMAT, duration); + if (gst_ogg_stream_packet_is_header (&pad->map, packet)) { + duration = 0; + GST_DEBUG_OBJECT (ogg, "packet is header"); + } else { + duration = gst_ogg_stream_get_packet_duration (&pad->map, packet); + GST_DEBUG_OBJECT (ogg, "packet duration %" G_GUINT64_FORMAT, duration); + } if (packet->b_o_s) { out_timestamp = GST_CLOCK_TIME_NONE; From 564eedd2143e5f7739fc17ac6f03b76fcf9ca90e Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:40:12 +0100 Subject: [PATCH 08/13] oggdemux: rename local variable for clarity While the casual reader might end up bewildered by just why this change might increase clarity, it just happens than, in the libogg and associated sources, op is the canonical name for an ogg_packet whlie og is the canonical name for an ogg_page, and reading this code confuses me. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 1af79a298d..5280270a34 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -2656,7 +2656,7 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) GstFlowReturn ret; GstOggChain *chain = NULL; gint64 offset = ogg->offset; - ogg_page op; + ogg_page og; gboolean done; gint i; @@ -2668,12 +2668,12 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) GstOggPad *pad; guint32 serial; - ret = gst_ogg_demux_get_next_page (ogg, &op, -1, NULL); + ret = gst_ogg_demux_get_next_page (ogg, &og, -1, NULL); if (ret != GST_FLOW_OK) { GST_WARNING_OBJECT (ogg, "problem reading BOS page: ret=%d", ret); break; } - if (!ogg_page_bos (&op)) { + if (!ogg_page_bos (&og)) { GST_WARNING_OBJECT (ogg, "page is not BOS page"); /* if we did not find a chain yet, assume this is a bogus stream and * ignore it */ @@ -2687,7 +2687,7 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) chain->offset = offset; } - serial = ogg_page_serialno (&op); + serial = ogg_page_serialno (&og); if (gst_ogg_chain_get_stream (chain, serial) != NULL) { GST_WARNING_OBJECT (ogg, "found serial %08x BOS page twice, ignoring", serial); @@ -2695,7 +2695,7 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) } pad = gst_ogg_chain_new_stream (chain, serial); - gst_ogg_pad_submit_page (pad, &op); + gst_ogg_pad_submit_page (pad, &og); } if (ret != GST_FLOW_OK || chain == NULL) { @@ -2734,7 +2734,7 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) gboolean known_serial = FALSE; GstFlowReturn ret; - serial = ogg_page_serialno (&op); + serial = ogg_page_serialno (&og); done = TRUE; for (i = 0; i < chain->streams->len; i++) { GstOggPad *pad = g_array_index (chain->streams, GstOggPad *, i); @@ -2748,10 +2748,10 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) /* submit the page now, this will fill in the start_time when the * internal decoder finds it */ - gst_ogg_pad_submit_page (pad, &op); + gst_ogg_pad_submit_page (pad, &og); if (!pad->map.is_skeleton && pad->start_time == -1 - && ogg_page_eos (&op)) { + && ogg_page_eos (&og)) { /* got EOS on a pad before we could find its start_time. * We have no chance of finding a start_time for every pad so * stop searching for the other start_time(s). @@ -2777,7 +2777,7 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) } if (!done) { - ret = gst_ogg_demux_get_next_page (ogg, &op, -1, NULL); + ret = gst_ogg_demux_get_next_page (ogg, &og, -1, NULL); if (ret != GST_FLOW_OK) break; } From 4fdb52871c87d419429ce697ed80387855daf64b Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:47:53 +0100 Subject: [PATCH 09/13] oggdemux: do not warn when finding a non BOS page After all, we do hope to find actual data for these streams. However, warn if we could not set up a chain when we find a non BOS page, as that means we don't have a valid Ogg stream. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 5280270a34..f04b5cc279 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -2674,11 +2674,13 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) break; } if (!ogg_page_bos (&og)) { - GST_WARNING_OBJECT (ogg, "page is not BOS page"); + GST_INFO_OBJECT (ogg, "page is not BOS page, all streams identified"); /* if we did not find a chain yet, assume this is a bogus stream and * ignore it */ - if (!chain) + if (!chain) { + GST_WARNING_OBJECT (ogg, "No chain found, no Ogg data in stream ?"); ret = GST_FLOW_UNEXPECTED; + } break; } From 68ed992e7e23a2a1d9494977f845cf1a7897b1f4 Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 10:58:20 +0100 Subject: [PATCH 10/13] oggdemux: do not warn about expected occurences In this case, finding a skeleton packet. Once upon a time, it used to be rare indeed, but no more. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index f04b5cc279..1131fb9afb 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -752,7 +752,7 @@ gst_ogg_pad_submit_packet (GstOggPad * pad, ogg_packet * packet) if (gst_ogg_map_parse_fisbone (&pad->map, packet->packet, packet->bytes, &serialno, &type)) { - GST_WARNING_OBJECT (pad->ogg, + GST_DEBUG_OBJECT (pad->ogg, "got skeleton packet for stream 0x%08x", serialno); skel_pad = gst_ogg_chain_get_stream (pad->chain, serialno); From 1e606c0456b8adcb2618f55c36a984a749afeb85 Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 11:05:11 +0100 Subject: [PATCH 11/13] oggstream: correctly identify skeleton EOS packet It is 0 byte, and was triggering the "bad packet" logic. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggstream.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ext/ogg/gstoggstream.c b/ext/ogg/gstoggstream.c index abc5910fda..c58daeab2b 100644 --- a/ext/ogg/gstoggstream.c +++ b/ext/ogg/gstoggstream.c @@ -1111,12 +1111,15 @@ gst_ogg_map_parse_fisbone (GstOggStream * pad, const guint8 * data, guint size, GstOggSkeleton stype; guint serial_offset; - if (size < SKELETON_FISBONE_MIN_SIZE) { + if (size != 0 && size < SKELETON_FISBONE_MIN_SIZE) { GST_WARNING ("small fisbone packet of size %d, ignoring", size); return FALSE; } - if (memcmp (data, "fisbone\0", 8) == 0) { + if (size == 0) { + /* Skeleton EOS packet is zero bytes */ + return FALSE; + } else if (memcmp (data, "fisbone\0", 8) == 0) { GST_INFO ("got fisbone packet"); stype = GST_OGG_SKELETON_FISBONE; serial_offset = 12; From df40ddf0aab02d9269974ddb86879abeb0429aee Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 11:08:25 +0100 Subject: [PATCH 12/13] oggdemux: add media type to chain information reports One more little step in making logs a little less abstruse. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 1131fb9afb..165bb238f7 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -3688,7 +3688,8 @@ gst_ogg_print (GstOggDemux * ogg) for (j = 0; j < chain->streams->len; j++) { GstOggPad *stream = g_array_index (chain->streams, GstOggPad *, j); - GST_INFO_OBJECT (ogg, " stream %08x:", stream->map.serialno); + GST_INFO_OBJECT (ogg, " stream %08x: %s", stream->map.serialno, + gst_ogg_stream_get_media_type (&stream->map)); GST_INFO_OBJECT (ogg, " start time: %" GST_TIME_FORMAT, GST_TIME_ARGS (stream->start_time)); } From 7b8b0fa1bbc22bb08e291c001e4413b47d0b120e Mon Sep 17 00:00:00 2001 From: Vincent Penquerc'h Date: Tue, 23 Aug 2011 11:12:10 +0100 Subject: [PATCH 13/13] oggdemux: do not warn when reaching EOS while scanning for the end chain After all, we were asking for it. This gets rid of the last warning-about-expected-condition. w00t. https://bugzilla.gnome.org/show_bug.cgi?id=657151 --- ext/ogg/gstoggdemux.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ext/ogg/gstoggdemux.c b/ext/ogg/gstoggdemux.c index 165bb238f7..56d23c49f1 100644 --- a/ext/ogg/gstoggdemux.c +++ b/ext/ogg/gstoggdemux.c @@ -2670,7 +2670,11 @@ gst_ogg_demux_read_chain (GstOggDemux * ogg, GstOggChain ** res_chain) ret = gst_ogg_demux_get_next_page (ogg, &og, -1, NULL); if (ret != GST_FLOW_OK) { - GST_WARNING_OBJECT (ogg, "problem reading BOS page: ret=%d", ret); + if (ret == GST_FLOW_UNEXPECTED) { + GST_DEBUG_OBJECT (ogg, "Reached EOS, done reading end chain"); + } else { + GST_WARNING_OBJECT (ogg, "problem reading BOS page: ret=%d", ret); + } break; } if (!ogg_page_bos (&og)) {