After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 650406 - vorbisdec does not handle headers in caps
vorbisdec does not handle headers in caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.33
Other Linux
: Normal normal
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-05-17 16:28 UTC by Aleix Conchillo Flaqué
Modified: 2011-06-15 17:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vorbisdec handling of header in caps (2.12 KB, patch)
2011-05-17 16:28 UTC, Aleix Conchillo Flaqué
needs-work Details | Review
vorbisdec handling of header in caps (3.13 KB, patch)
2011-05-18 15:17 UTC, Aleix Conchillo Flaqué
needs-work Details | Review
vorbisdec handling of header in caps (3.84 KB, patch)
2011-05-18 15:44 UTC, Aleix Conchillo Flaqué
committed Details | Review

Description Aleix Conchillo Flaqué 2011-05-17 16:28:57 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.
Comment 1 Sebastian Dröge (slomo) 2011-05-18 11:04:24 UTC
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
Comment 2 Aleix Conchillo Flaqué 2011-05-18 15:17:12 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2011-05-18 15:31:37 UTC
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
Comment 4 Aleix Conchillo Flaqué 2011-05-18 15:44:27 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2011-05-18 20:09:25 UTC
commit 0f7522e44994048cf60636b2d1725467d450bff6
Author: Aleix Conchillo Flaque <aleix@oblong.com>
Date:   Wed May 18 22:07:58 2011 +0200

    vorbisdec: Handle headers in caps
Comment 6 Sebastian Dröge (slomo) 2011-05-18 20:09:46 UTC
Pushed with some minor changes, thanks :)