[gstreamer-bugs] [Bug 370836] Looping feature for gnlobject

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=370836
  GStreamer | gnonlin | git

Edward Hervey <bilboed> changed:

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

--- Comment #16 from Edward Hervey <bilboed at gmail.com> 2010-09-02 11:31:52 UTC ---
Review of attachment 169317:
 --> (https://bugzilla.gnome.org/review?bug=370836&attachment=169317)

In addition to the comments below, could you create some unit tests to validate
proper behaviour ?

::: gnl/gnlghostpad.c
@@ +257,3 @@
+  if (GNL_OBJECT_LOOP (object)) {
+    stop = start + GNL_OBJECT_MEDIA_DURATION (object);
+  }

You are not allowed to modify the start/stop values. This must be removed, the
time-shifting handling is done solely by modifying the stream position of the
newsegment event....

... which you forgot to do.

You need to modify the stream position to reflect the looping:
i.e. the first time the stream position will be object.start, the second time
it will be object.start + media_duration, the third time object.start +
2xmedia_duration, etc....

@@ +263,3 @@
       GST_TIME_FORMAT, GST_TIME_ARGS (start), GST_TIME_ARGS (stop),
       GST_TIME_ARGS (nstream));
+

Please remove the unneeded whitespaces

@@ +308,3 @@
+    stop = start + GNL_OBJECT_MEDIA_DURATION (object);
+  }
+

Same comment as above.

@@ +351,3 @@
+            keepit = GNL_OBJECT_DURATION (object) > 0
+                && GNL_OBJECT_LOOP_PROGRESS (object) >=
+                GNL_OBJECT_DURATION (object);

GNL_OBJECT_DURATION (object) will always be > 0 at this point (else the seek
would have failed).

@@ +357,3 @@
+                  GST_SEEK_TYPE_SET, GNL_OBJECT_MEDIA_START (object),
+                  GST_SEEK_TYPE_SET,
+                  GNL_OBJECT_MEDIA_STOP (object));

You need to make sure you don't seek beyond the last loop.

ex: with a media_duration of 1s and a duration of 4.5s you need to make sure
the last seek has a stop value of MEDIA_START + 0.5s

@@ +364,3 @@
+              GST_LOG_OBJECT (object, "broadcasting loop seek event");
+              g_cond_broadcast (GNL_OBJECT_LOOP_CONDITION (object));
+              return FALSE;

You need to return TRUE since you did process the event.

::: gnl/gnlobject.c
@@ +288,3 @@
+  object->loop_progress = 0;
+  object->loop_condition = g_cond_new ();
+  object->loop_mutex = g_mutex_new ();

Those 4 variables could be moved to gnlghostpad since they're not being used at
all by gnlobject.[ch]

@@ +495,3 @@
+      || (object->loop
+          && ((object->media_start + object->media_duration) !=
+              object->media_stop))) {

if ((A && B) || (C && B))  => if ((A || C) && B)

@@ +518,3 @@
+    } else {
+      object->rate = 1;
+    }

It would be cleaner (and faster) to check whether looping is activated before
this big if()

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.