[Bug 733819] Port teletextdec to 1.0

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=733819
  GStreamer | gst-plugins-bad | git

Edward Hervey <bilboed> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #281814|none                        |needs-work
             status|                            |

--- Comment #2 from Edward Hervey <bilboed at bilboed.com> 2014-07-28 09:02:06 UTC ---
Review of attachment 281814:
 --> (https://bugzilla.gnome.org/review?bug=733819&attachment=281814)

::: ext/teletextdec/gstteletextdec.c
@@ +126,3 @@
     GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("video/mpeg,mpegversion=2,systemstream=TRUE;"
+        "private/teletext;")

fwiw, supporting mpeg systemstream isn't that useful and not in sync with how
gstreamer elements work (do one thing, do it well).

That would remove quite a bit of code also.

@@ +134,3 @@
     GST_STATIC_CAPS
+    (GST_VIDEO_CAPS_MAKE ("RGBA") ";"
+        "text/x-raw, format={utf-8,pango-markup} ; text/html ;")

While RGBA and text/pango output make sense ... text/html makes no sense
whatsoever.

How would one use it ?

That would remove yet some more code.

@@ +311,3 @@
       gst_teletextdec_event_handler, teletext);

+  teletext->queue = gst_atomic_queue_new (16);

Is there any real functional reason to switch from cond/gqueue to atomic queue
? Would be nice to keep the port patch as concise as possible.

@@ +418,3 @@
+      if (NULL == teletext->export_func) {
+        /* save the segment event and send it after sending caps. */
+        teletext->segment = event;

You should check if a segment event is already present (and not pushed), and if
so remove the previous one.

@@ +487,3 @@

+  ret =
+      GST_ELEMENT_CLASS (gst_teletextdec_parent_class)->change_state (element,

fwiw, you can avoid doing those changes by just using a #define
gst_teletext_parent_class parent_class at the top of the file

@@ +635,2 @@
   GstTeletextDec *teletext = GST_TELETEXTDEC (user_data);
+  (void) dx;

??

@@ +685,3 @@

+static void
+gst_teletextdec_negotiate_caps (GstTeletextDec * teletext,

There is something missing in this, which is to do an ALLOCATION query to
figure out if downstream can provide a GstBufferPool, and if so, use buffers
from that pool instead of creating our own (assuming the underlying library can
be provided buffers in which to write)

@@ +757,2 @@
   GstFlowReturn ret = GST_FLOW_OK;
+  (void) pad;

??

@@ +966,3 @@
+  GstBuffer *lbuf;
+  GstMapInfo buf_map;
+  (void) teletext;

??

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