[gstreamer-bugs] [Bug 638005] oggstream: implement tag extraction for Kate streams

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=638005
  GStreamer | gst-plugins-base | unspecified

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #177044|none                        |reviewed
             status|                            |

--- Comment #2 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-12-26 18:07:07 UTC ---
(From update of attachment 177044)
Thanks for the patch! Just a few code-style niggles:

>+static void
>+extract_tags_kate (GstOggStream * pad, ogg_packet * packet)
>+{
>+  GstTagList *list = NULL;
>+  char language[16], *ptr;
>+
>+  if (packet->bytes > 0) {
>+    switch (packet->packet[0]) {
>+      case 0x80:
>+        if (packet->bytes >= 64) {
>+          memcpy (language, packet->packet + 32, 16);

We generally prefer to keep things as flat as possible (and even use gotos if
needed). So maybe one could do

  if (packet->bytes == 0)
    return;

  switch (packet->packet[0]) {
    case 0x80:
      if (packet->bytes < 64)
        break-or-return-or-goto-label-that-does-a-GST_WARNING;
      memcpy (language, packet->packet + 32, 16);
      ...
    ...
  }


>+          language[15] = 0;
>+          /* language is a ISO 639-1 code, or RFC 3066 language code */
>+          g_strdelimit (language, NULL, '\0');
>+          for (ptr = language; *ptr; ptr++)
>+            if ((*ptr) & 0x80)
>+              break;
>+          if (language[0] && !(*ptr & 0x80)) {
>+            list =
>+                gst_tag_list_new_full (GST_TAG_LANGUAGE_CODE, language, NULL);
>+          }

It's not entirely clear to me what this code is trying to achieve (esp. the
parts that check for the 0x80 bit) - could you add a comment to the code?

Maybe one of these functions could be used (or be made to be a bit more
accepting input-wise)?
http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gsttaglanguagecodes.html

(Or a new function could be added)



>+  if (list) {
>+    if (pad->taglist) {
>+      /* ensure the comment packet cannot override the category/language
>+         from the identification header */
>+      gst_tag_list_insert (pad->taglist, list, GST_TAG_MERGE_KEEP_ALL);
>+    } else
>+      pad->taglist = list;
>+  }
>+}

Wouldn't it be easier to move this into the 0x80/0x81 case statements? (Either
packet is pretty much mandatory in the given order, no?)

-- 
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.