[Bug 642671] fieldanalysis: New element for analysing video input to produce progressive output

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #11 from Sebastian Dröge <slomo at circular-chaos.org> 2011-02-24 13:48:31 UTC ---
Review of attachment 181248:
 --> (https://bugzilla.gnome.org/review?bug=642671&attachment=181248)

In general looks good I guess. Some minor comments below but I didn't review
every single line. If you're sure that all this works as expected feel free to
push it after fixing the GAP stuff mentioned below

::: gst/deinterlace/gstdeinterlace.c
@@ +3,3 @@
  * Copyright (C) 2005 Martin Eikermann <meiker at upb.de>
  * Copyright (C) 2008-2010 Sebastian Dröge <slomo at collabora.co.uk>
+ * Copyright (C) 2010 Robert Swain <robert.swain at collabora.co.uk>

We have 2011 :) The same for the fieldanalysis patch btw ;)

@@ +548,3 @@
+   * processing latency and accuracy of timestamp adjustment for telecine
+   * streams.
+   *

Since markers please

@@ +935,3 @@
+            0), "interlacing-method");
+    /* 8 is strlen "telecine" */
+    if (temp && strlen (temp) >= 8 && !strncmp (temp, "telecine", 8)) {

You could use sizeof("telecine") here and why don't you just use g_str_equal()
for example? :)

@@ +1021,3 @@
+  if (GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_GAP)) {
+    /* gap buffers don't get added to the field history as they are
essentially
+     * dropped */

Are they recreated later? They might be added by a videorate element for
example and if you don't recreate them you'll create timestamp gaps. At least
for interlaced content this should be handled like every other frame

@@ +2134,3 @@
+    /* 8 is strlen "telecine" */
+    if (caps_field_temp && strlen (caps_field_temp) >= 8
+        && !strncmp (caps_field_temp, "telecine", 8))

same as above, why not just g_str_equal()?

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