After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737183 - audiomixer: Port to GstAggregator
audiomixer: Port to GstAggregator
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-23 14:27 UTC by Thibault Saunier
Modified: 2014-10-06 17:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audiomixer: Port to GstAggregator (66.55 KB, patch)
2014-09-23 14:27 UTC, Thibault Saunier
none Details | Review
audiomixer: Port to GstAggregator (66.85 KB, patch)
2014-09-23 15:00 UTC, Thibault Saunier
needs-work Details | Review
aggregator: Query seeking when a seek failed to see if it was expected (4.09 KB, patch)
2014-10-06 09:30 UTC, Thibault Saunier
committed Details | Review
audiomixer: Port to GstAggregator (66.64 KB, patch)
2014-10-06 09:30 UTC, Thibault Saunier
none Details | Review
Updated patch with: (66.88 KB, patch)
2014-10-06 09:38 UTC, Thibault Saunier
committed Details | Review

Description Thibault Saunier 2014-09-23 14:27:21 UTC
The audiomixer element (replaceement of adder) should be ported to GstAggregator.
Comment 1 Thibault Saunier 2014-09-23 14:27:24 UTC
Created attachment 286879 [details] [review]
audiomixer: Port to GstAggregator
Comment 2 Thibault Saunier 2014-09-23 15:00:59 UTC
Created attachment 286885 [details] [review]
audiomixer: Port to GstAggregator

https://bugzilla.gnome.org/show_bug.cgi?id=737183

Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Comment 3 Thibault Saunier 2014-10-03 17:08:14 UTC
Ping? :)
Comment 4 Sebastian Dröge (slomo) 2014-10-05 19:15:29 UTC
Review of attachment 286885 [details] [review]:

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 :)
Comment 5 Sebastian Dröge (slomo) 2014-10-06 07:55:12 UTC
Bug #734499 feels relevant for the segment.position comment ;)
Comment 6 Thibault Saunier 2014-10-06 09:30:11 UTC
Created attachment 287818 [details] [review]
aggregator: Query seeking when a seek failed to see if it was expected

And do not worry if seeking failed on a stream that is not seekable
Comment 7 Thibault Saunier 2014-10-06 09:30:22 UTC
Created attachment 287819 [details] [review]
audiomixer: Port to GstAggregator

https://bugzilla.gnome.org/show_bug.cgi?id=737183

Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Comment 8 Thibault Saunier 2014-10-06 09:38:39 UTC
Created attachment 287822 [details] [review]
Updated patch with:

* New offset setting happens right after actually setting srcpad caps Making
  * sure that a '-1' position will not be used when setting the offset Adding a
  * comment about why we need to do a timeout based test to check if aggregation
    actually happens (or not).
Comment 9 Thibault Saunier 2014-10-06 17:33:30 UTC
commit dd65d70f65c72a8d5cfc5560787ead90f4982b66
Author: Thibault Saunier <tsaunier@gnome.org>
Date:   Wed May 28 16:29:37 2014 +0200

    audiomixer: Port to GstAggregator
    
    https://bugzilla.gnome.org/show_bug.cgi?id=737183
    
    Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>