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 638005 - oggstream: implement tag extraction for Kate streams
oggstream: implement tag extraction for Kate streams
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-12-25 16:41 UTC by Vincent Penquerc'h
Modified: 2010-12-27 10:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
oggstream: implement tag extraction for Kate streams (2.22 KB, patch)
2010-12-25 16:41 UTC, Vincent Penquerc'h
reviewed Details | Review
oggstream: implement tag extraction for Kate streams (2.71 KB, patch)
2010-12-27 10:21 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2010-12-25 16:41:31 UTC
This will mainly allow Totem to know the language of those streams,
so the subtitle selection menu gets properly filled out.
Comment 1 Vincent Penquerc'h 2010-12-25 16:41:34 UTC
Created attachment 177044 [details] [review]
oggstream: implement tag extraction for Kate streams
Comment 2 Tim-Philipp Müller 2010-12-26 18:07:07 UTC
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?)
Comment 3 Vincent Penquerc'h 2010-12-27 10:21:20 UTC
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.
Comment 4 Vincent Penquerc'h 2010-12-27 10:23:07 UTC
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).
Comment 5 Tim-Philipp Müller 2010-12-27 10:59:42 UTC
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.