[Bug 628429] Add support for DivX XSUB subtitles

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #21 from Sebastian Dröge <slomo at circular-chaos.org> 2013-07-15 08:14:36 UTC ---
Review of attachment 249058:
 --> (https://bugzilla.gnome.org/review?bug=628429&attachment=249058)

::: gst/xsub/gstxsub.c
@@ +287,3 @@
+
+  frame_start = GST_BUFFER_TIMESTAMP (buf);
+  frame_stop = frame_start + GST_BUFFER_DURATION (buf);

You assume here that all video buffers have timestamps and a duration. You need
to check and fail if they are not there

@@ +304,3 @@
+
+      if (frame_start <= spu_stop)
+        break;

You can make the check a bit more simple I guess. The spu is to be displayed if
(spu_start < frame_stop && spu_stop > frame_start). It is to be dropped if
(spu_stop <= frame_start) and you have to wait if (spu_start >= frame_stop).

Also please implement synchronization between spu and video stream here, look
at assrender for an example.

@@ +406,3 @@
+    GST_ERROR_OBJECT (pad, "Parsing of header/spu data failed!");
+    gst_buffer_unref (parsed->buf);
+    g_slice_free (GstXSubData, parsed);

Shouldn't this return GST_FLOW_ERROR after a few errors in a row then?

@@ +411,3 @@
+    parsed->bgless_row = g_slice_alloc (parsed->width * XSUB_RGB_BPP);
+    g_mutex_lock (&filter->spu_queue_lock);
+    g_queue_push_tail (&filter->spu_queue, parsed);

Synchronization with video chain missing here

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