[Bug 737183] audiomixer: Port to GstAggregator

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #4 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-10-05 19:15:29 UTC ---
Review of attachment 286885:
 --> (https://bugzilla.gnome.org/review?bug=737183&attachment=286885)

Generally looks good and straightforward :) Thanks!

::: gst/audiomixer/gstaudiomixer.c
@@ +413,3 @@

+  audiomixer->offset = gst_util_uint64_scale (agg->segment.position,
+      GST_AUDIO_INFO_RATE (&audiomixer->info), GST_SECOND);

Why do you set the offset when receiving a caps event? I would understand if
you would update the offset with the current rate, and then update the rate
from the caps... but that doesn't happen here. I think for rate changes we
would need to store an offset, and then use that to offset all following
timestamps

Also make sure that segment.position can never be -1 here... otherwise bad
stuff happens :)

@@ +820,3 @@
+    return FALSE;
+
+  gst_buffer_replace (&audiomixer->current_buffer, NULL);

offsets and all that should probably be also reset here

::: tests/check/elements/audiomixer.c
@@ +983,3 @@
   ret = gst_pad_chain (sinkpad, buffer);
+  main_loop = local_mainloop;
+  g_timeout_add (100, (GSourceFunc) _quit, main_loop);

Why? A comment would be good, timeouts/sleep in tests are always suspicious :)

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