Created attachment 177044 [details] [review]
oggstream: implement tag extraction for Kate streams

Comment on attachment 177044 [details] [review]
oggstream: implement tag extraction for Kate streams

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?)

Created attachment 177088 [details] [review]
oggstream: implement tag extraction for Kate streams

This will mainly allow Totem to know the language of those streams,
so the subtitle selection menu gets properly filled out.


Yes, this wasn't obvious and called for a comment.
As for the gst_tag_get_language_* calls, I did not intend any conversion,
though I suppose it can do matching against known existing tags, which may
or may not be good (we reject invalid tags, but we might reject obscure
but correct tags). I've added it nevertheless.

Ah, I forgot to comment on the moving the second part into the switch: I'd rather not, as one could create an (invalid) stream which would segfault the code if it assumes list will be non NULL in 0x81 (and it can be NULL if the language tag is invalid).

Ok, pushed, thanks:

 commit 85cafac6af27a3f46cd7ffcce1ff503810cafc11
 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
 Date:   Sat Dec 25 15:22:42 2010 +0000

    oggstream: implement tag extraction for Kate streams
    
    This will mainly allow Totem to know the language of those streams,
    so the subtitle selection menu gets properly filled out.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=638005


I just dropped the ASCII verification and non-0-length check though since that's all implicitly done by the gst_tag_get_language_code_iso_639_1() anyway if I'm not mistaken, and moved some variables into the case() block.