Resampled buffer #131

smimram · 2013-11-15T08:27:57Z

Added buffer.adaptative, which is like buffer but reads the buffer at varying speed in order to keep the buffer at decent filling. For instance,

sleep(delay=1.5,s)

simulates a source which is 1.5x slower than realtime (thus catchuping quickly). The following script however accomodates quite well of it

s = playlist("~/Music")
s = sleeper(delay=1.2,s)
s = mksafe(s)
s = buffer.adaptative(averaging=2.,s)
s = mksafe(s)
out(s)

@dbaelde @toots I have some minor stuff to fix but could you please review already it would help much. In particular, I am no so sure I am filling and reading from frames exactly correctly... Thanks!

and blocking or slow operators. sleep(delay=1.5,s) waits 1.5 times the duration of a frame at each get_frame. Expect catchup ahead! This is meant for testing operators (buffers in particular) and not at all for production.

This is just like buffer, excepting that the output reads the buffer at slower or faster speed in order to keep the buffer at around the same level always. For clocks a bit too slow (typically soundcard) it should help without being noticeable. For now the operator forgets about tracks boundaries and metadata (but they could come back).

smimram · 2013-11-15T08:36:58Z

And if you really want to have fun and remember old bad quality K7 (magnetic tapes) players, when battery was getting low, change ligne 2 to:

s = sleeper(delay=0.5,random=2.,s)

toots · 2013-11-16T01:59:22Z

src/operators/time_warp.ml

+        "", Lang.source_t k, None, None])
+    ~kind:(Lang.Unconstrained k)
+    ~category:Lang.Liquidsoap
+    ~descr:"Create a buffer between two different clocks. The speed of the output is adapted so that no buffer underrun or overrun occur. This wonderful behavior has a cost: the pitch of the sound might be changed a little. Also, this operator drops track boundaries and metadata for now."


Please, can you wrap you code to like 80 chars.. 😭

toots · 2013-11-16T02:23:15Z

Done reviewing what I could spot. Looks quite interesting to me!

toots · 2013-11-16T02:25:50Z

src/operators/time_warp.ml

+    let prebuf = Frame.audio_of_seconds pre_buffer in
+    (* let maxbuf = Frame.audio_of_seconds max_buffer in *)
+  object (self)
+    inherit Output.output


You could probably inherit from output.dummy here and get rid of the output_* methods..

smimram · 2013-11-16T08:44:41Z

Thanks @toots for comments! On a general note, you made a lot of comments about the general structure of producer/consumer, which is copied verbatim from the buffer operator in the same file. So, the modifications should be applied to both. However, I don't really feel confident in modifying buffer which is quite a core operator. A little help on this side would be welcome... (and in return I will limit to 80 chars ;) )

smimram · 2013-11-16T09:00:03Z

Side note: there's already one happy user:)
http://www.mail-archive.com/savonet-users@lists.sourceforge.net/msg08932.html

smimram · 2013-11-16T10:06:39Z

Ok, so I have implemented support for metadata. I think that it is ready for merge. I would like to delay for a second PR:

the cleaning of parameters of operators for buffer and buffer.adaptative (as @toots requested)
the use of the new Metadata generator in other generators
Both can take time, are unrelated to buffer.adaptative per se, and can potentially introduce bugs in sensitive parts.

@toots do you agree with that?
@toots @dbaelde do you greenlight the merge?

toots · 2013-11-16T16:16:12Z

src/operators/time_warp.ml

+    method stype = Source.Fallible
+
+    (* TODO *)
+    method remaining = -1


dbaelde · 2013-11-16T22:57:27Z

src/operators/sleeper.ml

+      Some "Amount of time to sleep at each frame, the unit being the frame length.";
+      "random", Lang.float_t, Some (Lang.float 0.),
+      Some "Maximal random amount of time added (unit is frame length).";
+      "die", Lang.float_t, Some (Lang.float (-1.)),


Instead of "die", "stall" or "freeze" seem more appropriate.

dbaelde · 2013-11-16T23:22:16Z

Thanks for the feature Sam, adaptative resampling is a nice thing to try and I trust you did it well, at least far better than I could have. I'm fine with a quick merge as long as it does not have any impact on the rest of the code. In that respect I would be happier if the new adaptative buffer was in its own file and the experimental metadata generator was not in the main generator file.

dbaelde · 2013-11-16T23:33:30Z

src/operators/time_warp.ml

+          in
+
+          let ofs = Frame.position frame in
+          (* Yes, I've seen some cases... *)


What does this comment mean?

I used the following script to test new tracks:

s = playlist(id="playlist","~/Music") s = skip_blank(max_blank=5.,threshold=0.,s) s = sleeper(delay=1.2,s) s = mksafe(s) s = output.dummy(s) s = buffer.adaptative(averaging=1.,s) s = mksafe(s) out(s)

And after a few tracks, I had requests to fill frames with negative Frame.position, which I don't think is really related to our operator... So I just worked around...

If Frame.position returns a negative number, it means that someone has added a break at a negative position in that frame. I find it hard to bet that it isn't related to the new operator and generator. In any case, this really needs to be properly investigated.

dbaelde · 2013-11-16T23:35:13Z

Did you have to use a ring buffer rather than a generator? Using a generator, you could have re-used the old consumer and changed only the producer. (I don't care much for the slightly better perf that the ring buffer gets us, I prefer simple and clean code.)

What is the difference between a metadata generator and a general From_frames generator for frames without any channels? I see they are not interchangeable but it may be easy to use From_frames and some operations on Frames to do what you do with your MG, and this would avoid duplicating code in Generator.

dbaelde · 2013-11-16T23:38:56Z

src/stream/generator.ml

@@ -168,6 +168,96 @@ let get g size =

 end

+(* TODO: use this in the following modules instead of copying the code... *)


Please don't introduce a TODO in one of our core files... Also, I'm not in favor of a rework of Generator (using Metadata as part of From_frames) now, because I'm not sure to have the time to review this carefully enough, and we don't have the time to test it enough either, with an upcoming release.

Yeah, I'm kinda weirded out by the amount of code as well.. It seems to me that there should be better path of least resistance..

Well, I think that this TODO is really deserved:

the code for handling metadata is duplicated in all the generators, which is really bad

as you remark, we do not want to do this now, in order not to mess up with generators which are a very sensitive part of Liq, so the TODO will help us remember

toots · 2013-11-17T00:45:17Z

@smimram sorry if some of my comments were nitpicking.. I just like to mark all I think of but some of it isn't to be taken too seriously.. I still think that the to_source calls should be made in the operator's arguments parsing, unless required for some reasons..

I put a couple of comments, the only two things standing out for me are the metadata code, which seem like there could be a shorter solution and the missing remaining value. Other than that, great work, it should be very useful.

Well actually, now that I think about it, we should also document that properly.. ^^

smimram · 2013-11-17T09:08:03Z

@toots No problem, I like detailed comments! I have filled remaining, and concerning metadata I really think that this is the right solution: we want a generator for metadata (I mean it seems to be right structure), and actually this metadata generator could be used to simplify and factor much of the code in other generators (which we don't want to do now for obvious reasons explained above)

smimram · 2013-11-18T07:37:31Z

Concerning the negative offset and actually it seems that, as Brandy and Monica would say: the bug is mine.

I'll handle this soon.

smimram · 2013-11-18T21:45:03Z

Last bug corrected (tm).

dbaelde · 2013-11-19T21:20:59Z

Nice. I have no more serious problem with the code, feel free to merge.
Thanks!

On Mon, Nov 18, 2013 at 10:45 PM, Samuel Mimram notifications@gitpro.ttaallkk.topwrote:

Last bug corrected (tm).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/131#issuecomment-28741117
.

David

toots · 2013-11-19T21:22:31Z

Same here!

2013/11/19 David Baelde notifications@github.com

Nice. I have no more serious problem with the code, feel free to merge.
Thanks!

On Mon, Nov 18, 2013 at 10:45 PM, Samuel Mimram notifications@gitpro.ttaallkk.topwrote:

Last bug corrected (tm).

—
Reply to this email directly or view it on GitHub<
https://github.com/savonet/liquidsoap/pull/131#issuecomment-28741117>
.

David

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/131#issuecomment-28836752
.

Resampled buffer

smimram added 2 commits November 15, 2013 09:14

New operator: sleeper to simulate network delays

debd8ce

and blocking or slow operators. sleep(delay=1.5,s) waits 1.5 times the duration of a frame at each get_frame. Expect catchup ahead! This is meant for testing operators (buffers in particular) and not at all for production.

ghost assigned smimram Nov 15, 2013

smimram added 3 commits November 15, 2013 18:00

Sleepers can die.

41cf85e

Add comments and averaging parameter.

70641f7

Merge branch 'master' into resampled-buffer

e8cd8d7

toots reviewed Nov 16, 2013
View reviewed changes

smimram added 2 commits November 16, 2013 10:56

Cosmetic.

574cd83

Add support for metadata in buffer.adaptative.

3cfb3cb

toots reviewed Nov 16, 2013
View reviewed changes

src/operators/time_warp.ml

method stype = Source.Fallible

(* TODO *)

method remaining = -1

Copy link

Member

toots Nov 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO... ^^

Implement remaining.

bf0fb8c

dbaelde reviewed Nov 16, 2013
View reviewed changes

smimram referenced this pull request Nov 17, 2013

Add comments to generator.ml

87432a0

Freeze is better terminology than die.

968d615

Correct negative offset problem.

3903f1f

smimram added a commit that referenced this pull request Nov 19, 2013

Merge pull request #131 from savonet/resampled-buffer

ec971b9

Resampled buffer

smimram merged commit ec971b9 into master Nov 19, 2013

smimram deleted the resampled-buffer branch November 19, 2013 22:11

jdbuys mentioned this pull request Sep 9, 2022

Liquidsoap deadlock #2587

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resampled buffer #131

Resampled buffer #131

smimram commented Nov 15, 2013

smimram commented Nov 15, 2013

toots Nov 16, 2013

toots commented Nov 16, 2013

toots Nov 16, 2013

smimram commented Nov 16, 2013

smimram commented Nov 16, 2013

smimram commented Nov 16, 2013

toots Nov 16, 2013

dbaelde Nov 16, 2013

dbaelde commented Nov 16, 2013

dbaelde Nov 16, 2013

smimram Nov 17, 2013

dbaelde Nov 17, 2013

smimram Nov 17, 2013

dbaelde commented Nov 16, 2013

dbaelde Nov 16, 2013

toots Nov 17, 2013

smimram Nov 17, 2013

toots commented Nov 17, 2013

smimram commented Nov 17, 2013

smimram commented Nov 18, 2013

smimram commented Nov 18, 2013

dbaelde commented Nov 19, 2013

toots commented Nov 19, 2013

		@@ -168,6 +168,96 @@ let get g size =

		end

		(* TODO: use this in the following modules instead of copying the code... *)

Resampled buffer #131

Resampled buffer #131

Conversation

smimram commented Nov 15, 2013

smimram commented Nov 15, 2013

Choose a reason for hiding this comment

toots commented Nov 16, 2013

Choose a reason for hiding this comment

smimram commented Nov 16, 2013

smimram commented Nov 16, 2013

smimram commented Nov 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaelde commented Nov 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaelde commented Nov 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toots commented Nov 17, 2013

smimram commented Nov 17, 2013

smimram commented Nov 18, 2013

smimram commented Nov 18, 2013

dbaelde commented Nov 19, 2013

toots commented Nov 19, 2013