[gstreamer-bugs] [Bug 600929] [kate] tiger element doesn't handle segments and text/video synchronization properly

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #25 from Sebastian Dröge <slomo at circular-chaos.org> 2010-12-12 14:14:58 UTC ---
Review of attachment 153537:
 --> (https://bugzilla.gnome.org/review?bug=600929&attachment=153537)

Sorry for taking that long to review

First of all, please try to split your changes into separate commits, all doing
a single change, instead of one large commit that does $stuff. That makes it
much easier to review

::: ext/kate/gstkatedec.c
@@ +393,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_PAUSED_TO_READY:
+      g_queue_free (kd->event_queue);

You're going to leak all events here that were not pushed downstream yet

@@ +425,3 @@
+
+  // Delay events till we've set caps
+  if (kd->delay_events) {

You should never delay flush-start, flush-stop and eos events, only all others

@@ +460,3 @@
+
+      gst_event_parse_seek (event, &rate, &format, &flags, &cur_type, &cur,
+          &stop_type, &stop);

No need to parse the seek event here if you're not going to use the values
anyway

::: ext/kate/gstkatetiger.c
@@ +652,3 @@
+        kate_float t =
+            gst_segment_to_stream_time (&tiger->video_segment,
GST_FORMAT_TIME,
+            tiger->video_segment.last_stop) / (gdouble) GST_SECOND;

If you're using this for synchronization you should really use running time
instead of stream time

@@ +892,3 @@
+      }
+
+      res = gst_pad_event_default (pad, event);

Only forward events here if the source pad caps are set already, same story as
in katedec

::: ext/kate/gstkateutil.c
@@ +168,3 @@
     GST_ELEMENT_ERROR (element, STREAM, DECODE, (NULL),
         ("Failed to decode Kate packet: %d", ret));
+    gst_util_dump_mem (kp.data, kp.nbytes);

Please remove this :)

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