GNOME Bugzilla – Bug 748680
directsoundsink: fix sleep for buffer-time lower than 200000
Last modified: 2015-12-21 11:47:07 UTC
Created attachment 302625 [details] [review] directsoundsink: fix sleep for buffer-time lower than 200000 There's a fix sleep of 100ms!!! when the ring buffer is already full, but if you have lower buffer-time eg. 60ms this fixed sleep makes it impossible to have a smooth playback. A generic solution or parameter is needed, but I think 100ms fixed sleep is far too much - changed to 10ms sleep. Patch attached
You should use the configured buffer_time in the ring buffer, instead of another hardcoded value. spec->buffer_time / 2 seems like a good value for this sleep.
But never smaller than latency_time
What about using latency_time? It should be the maximum rate the buffer can be filled with data?
Created attachment 317474 [details] [review] replace the fix 100ms sleep with a sleep calculated as the latency-time minus free buffer
Review of attachment 317474 [details] [review]: ::: sys/directsound/gstdirectsoundsink.c @@ +506,3 @@ + dsoundsink->latency_time = spec->latency_time; + + dsoundsink->sample_rate = spec->info.rate; You could also just access that at any point after prepare() from the spec field of GstAudioBaseSink's ringbuffer field. @@ +641,3 @@ + dsoundsink->sample_rate; + + sleepTime = (dsoundsink->latency_time / 1000) - dwFreeBufferTime; Why? Wouldn't it be better to wait for ((length - dwFreeBufferSize) * 1000) / (bytes_per_sample * sample_rate) milliseconds instead? That would be the amount of time that is missing from the buffer size before we can fill it with the complete data we currently have.
"You could also just access that at any point after prepare() from the spec field of GstAudioBaseSink's ringbuffer field." -> I don't understand how to access this in gst_directsound_sink_write. Could you please explain!? And I also tried your suggestion for the calculation. But decided finally it's best to do it like in directsoundsrc and use the latency-time in ms for the sleep. It's because windows Sleep() function isn't correct enough to gain something out of a correct calculation.
Created attachment 317505 [details] [review] replace the fix 100ms sleep with latency-time
(In reply to Thomas Roos from comment #6) > "You could also just access that at any point after prepare() from the spec > field of GstAudioBaseSink's ringbuffer field." -> I don't understand how to > access this in gst_directsound_sink_write. Could you please explain!? GST_AUDIO_BASE_SINK(asink)->ringbuffer->spec.latency_time or similar > And I also tried your suggestion for the calculation. But decided finally > it's best to do it like in directsoundsrc and use the latency-time in ms for > the sleep. It's because windows Sleep() function isn't correct enough to > gain something out of a correct calculation. Why calculate anything at all then? ;) It would probably be better to wait for the correct time instead of some random other value, even if Sleep() is not very accurate. Is there some way to get notified from directsound when the buffer is more free again instead of using Sleep() here?
> Is there some way to get notified from directsound when the buffer is more > free again instead of using Sleep() here? No, that's one reason more why nowadays you use WASAPI, there you can register a callback.
Alright, let's go with sleeping then. But I still think it would be better to sleep for a more accurately calculated time instead of taking some almost random number :)
Created attachment 317729 [details] [review] replace the fix 100ms sleep with (length - dwFreeBufferSize) * 1000) / (bytes_per_sample * rate) ok, lets do it the way you proposed
commit 51f94288ce66a0a6a28e45801f4e17eb74044298 Author: Thomas Roos <thomas.roos@industronic.de> Date: Mon Dec 21 11:37:26 2015 +0100 directsoundsink: Fix sleep for buffer-time lower than 200000 https://bugzilla.gnome.org/show_bug.cgi?id=748680