GNOME Bugzilla – Bug 650406
vorbisdec does not handle headers in caps
Last modified: 2011-06-15 17:42:41 UTC
Created attachment 187973 [details] [review] vorbisdec handling of header in caps vorbisdec seems to always expect header packets before data packets. If a data packet is received (a no headers are received yet) the following error message shows up: gstvorbisdec.c(976): vorbis_handle_data_packet (): /GstPipeline:pipeline1/GstDecodeBin2:decodebin21/GstVorbisDec:vorbisdec0: no header sent yet However vorbisenc does put vorbis headers (identification, comment and bitstream codebook) inside buffer caps. This means vorbisdec could make use of them and initialize the decoder before handling the data packet. The following patch is a first try to solve this. I'm really new to gstreamer so it might be better ways to do this, or may be this does not solves the issue completely.
Review of attachment 187973 [details] [review]: Looks good in general and doing something like this for vorbis, theora, etc was on my TODO list too :) Just some minor things that need to be improved: ::: ext/vorbis/gstvorbisdec.c @@ +1014,3 @@ +{ + GstFlowReturn result = GST_FLOW_OK; + GstCaps *caps = GST_BUFFER_CAPS (buffer); You should get the caps from the sinkpad instead, the buffer might have NULL caps @@ +1023,3 @@ + + /* initial header */ + value = gst_value_array_get_value (array, 0); First check here if the array has at least 3 elements @@ +1024,3 @@ + /* initial header */ + value = gst_value_array_get_value (array, 0); + buf = gst_value_get_buffer (value); And check if it really contains buffers and error out otherwise @@ +1085,3 @@ + /* try to find header in caps so we can initialize the decoder */ + if (!vd->initialized) + (void) vorbis_dec_handle_header_caps (vd, buffer); Return GST_FLOW_NOT_NEGOTIATED in that function if something fails and return that value from here instead of trying to continue
Created attachment 188042 [details] [review] vorbisdec handling of header in caps Followed recommendations. You were right, I just found a case where buffer caps were NULL.
Review of attachment 188042 [details] [review]: ::: ext/vorbis/gstvorbisdec.c @@ +1021,3 @@ + fprintf (stderr, "%s streamheader array size %ds\n", + gst_structure_get_name (s), + gst_value_array_get_size(array)); Please remove this :) @@ +1146,3 @@ + { + GST_ELEMENT_ERROR (vd, STREAM, DECODE, (NULL), ("invalid streamheader in caps")); + result = GST_FLOW_ERROR; Don't set result here, you already set it to a correct flow return
Created attachment 188045 [details] [review] vorbisdec handling of header in caps Sorry, I just saw it a little before you sent the review. New one attached. I have also did a local commit and went through the GNU indent fixes.
commit 0f7522e44994048cf60636b2d1725467d450bff6 Author: Aleix Conchillo Flaque <aleix@oblong.com> Date: Wed May 18 22:07:58 2011 +0200 vorbisdec: Handle headers in caps
Pushed with some minor changes, thanks :)