audiotestsrc: Fix the way we check EOS in reverse playback

In reverse playback we were not taking into account the current buffer samples to check if we had reached EOS which was leading to a buffer with PTS = CLOCK_TIME_NONE containing too many frames followed by a useless buffer with pts=0 duration=0, and a g_critical issue in gst_object_sync_values.

Also add a validate based test case. Without that patch this is how the expectation fails:

--- log-asink-sink-expected       2020-05-22 23:22:42.654384579 -0400
+++ log-asink-sink-actual  2020-05-22 23:29:35.671586380 -0400
@@ -27,5 +27,6 @@
 buffer: pts=0:00:00.058820861, due=0:00:00.023219955, flags=discont
 buffer: pts=0:00:00.035600907, due=0:00:00.023219954, flags=discont
 buffer: pts=0:00:00.012380952, due=0:00:00.023219955, flags=discont
-buffer: pts=0:00:00.000000000, due=0:00:00.012380952, flags=discont
+buffer: due=0:00:00.012380953, flags=discont
+buffer: pts=0:00:00.000000000, flags=discont
 event eos: (no structure)

added 9 commits

• 8eac8ab9...eed54928 - 8 commits from branch gstreamer:master
• aee8f8e0 - audiotestsrc: Fix the way we compute EOS in reverse playback

Compare with previous version

changed title from audiotestsrc: Fix the way we compute EOS in reverse playback to audiotestsrc: Fix the way we check EOS in reverse playback

changed the description

Also the fact that all buffers have flags=discont is probably another bug.

I think that's correct for raw audio unless the samples inside the buffers are actually reversed (they're not currently). The last sample of the first buffer is not continuous with the first sample of the second buffer, but instead the first sample of the first buffer is continuous with the last sample of the second buffer.

Same thing we do elsewhere for reverse playback. Each "group" (of samples, pictures, ...) that has to be reversed still is separated by the discont flag.

added 1 commit

• 18be5d0c - audiotestsrc: Fix the way we compute EOS in reverse playback

Compare with previous version

resolved all threads

assigned to @gstreamer-merge-bot

mentioned in commit thiblahute/gst-plugins-base@b46718b1

added 9 commits

• 18be5d0c...eed54928 - 8 commits from branch gstreamer:master
• b46718b1 - audiotestsrc: Fix the way we compute EOS in reverse playback

Compare with previous version

assigned to @thiblahute and unassigned @gstreamer-merge-bot

I couldn't merge this branch: CI failed!

assigned to @gstreamer-merge-bot and unassigned @thiblahute

merged

this seems introduce different behaviour than gstreamer, where the tests option control all the tests including the validate. Why not make all the sub-repos agree on a same behaviour? When tests is disabled, the gst_tester is missing thus the configuration enable validate will always fail. Smarter way might check if gst_tester actually exists, if not, we should automatically disable all the targets require it.

Ah, I thought I had added a validate option in core too, should I @tpm?

We could indeed check if the tester was found in core in any case.

we probably should @tpm. We probably should add a tests option in gst-build too, to disable all the tests once for all. check the tester before subdir() will make the build more robust.

Thinking again about it I am not sure we should have 2 options, test could cover them both, we wait from more insight before doing though.

Don't have strong opinions..

1. The existing test option would be enough for now, wouldn't it?

2. Is it useful to have an explicit option for validate itself too? It will just be skipped at runtime anyway if the tool is not available, right? If someone wants finer-grained control we can always add it later.

3. For the gst-build setup scenario it might be useful to set gst_tester to a disabler() so that the variable is always defined?

1. I think so 2. Indeed, could be added later 3. We can pass a default value to subproject.get_variable() these days so we could but it is not mandatory to me.

We can pass a default value to subproject.get_variable() these days so we could but it is not mandatory to me.

Either way seems fine, but currently it's possible to configure things in such a way that the variable will not be defined if I'm not mistaken, so might be nice to avoid that.

Either way seems fine, but currently it's possible to configure things in such a way that the variable will not be defined if I'm not mistaken, so might be nice to avoid that.

Yep, @slomo hit this today. !697 (merged) fixes it.

mentioned in commit nirbheek/gst-plugins-base@d64aad41

mentioned in merge request !697 (merged)

mentioned in commit nirbheek/gst-plugins-base@ea420da7

mentioned in commit nirbheek/gst-plugins-base@3ce296e8

mentioned in commit nirbheek/gst-plugins-base@30509252

changed milestone to %1.17.1