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 781249 - directsoundsrc: Correctly calculate segsize and segtotal
directsoundsrc: Correctly calculate segsize and segtotal
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.12.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-04-13 07:33 UTC by Sebastian Dröge (slomo)
Modified: 2017-05-12 09:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
directsoundsrc: Correctly calculate segsize and segtotal (3.15 KB, patch)
2017-04-13 07:33 UTC, Sebastian Dröge (slomo)
none Details | Review
Use latency-time and buffer-time settings (11.40 KB, patch)
2017-05-03 17:21 UTC, Nirbheek Chauhan
committed Details | Review
Use GstClockID to wait instead of Sleep() (5.19 KB, patch)
2017-05-10 12:42 UTC, Nirbheek Chauhan
committed Details | Review

Description Sebastian Dröge (slomo) 2017-04-13 07:33:28 UTC
See commit message. This was reverted again recently because it causes
drop-outs and continuous underruns sometimes.

Also it's not complete: the DirectSound buffer-size is always configured to
2s, regardless of the properties.
Comment 1 Sebastian Dröge (slomo) 2017-04-13 07:33:32 UTC
Created attachment 349766 [details] [review]
directsoundsrc: Correctly calculate segsize and segtotal

segsize should be based on latency-time, and must be a multiple of the
frame size. segtotal should be based on buffer-time and segsize.

This prevents errors caused by outputting buffers that are not a
multiple of the frame size, and actually makes the buffer-time and
latency-time properties do what they're supposed to do.
Comment 2 Nirbheek Chauhan 2017-05-03 17:21:50 UTC
Created attachment 350998 [details] [review]
Use latency-time and buffer-time settings

I've squashed your commit into this one. It should fix this issue. Next on my list is https://bugzilla.gnome.org/show_bug.cgi?id=773681 which is due to similar problems in sleeping while polling.
Comment 3 Sebastian Dröge (slomo) 2017-05-08 14:14:05 UTC
Comment on attachment 350998 [details] [review]
Use latency-time and buffer-time settings

commit 1a8610f8f8305f20a235aa4fe78ab3fb0c785df4
Author: Nirbheek Chauhan <nirbheek@centricular.com>
Date:   Wed May 3 22:50:27 2017 +0530

    directsoundsrc: Use latency-time and buffer-time settings
    
    Earlier, the plugin was ignoring those settings and blindly setting
    buffer-time to 2 seconds and latency-time to 200ms, which forced all
    pipelines to have a minimum latency of 200ms + sink latency.
    
    The values of segsize and segtotal were also not derived correctly.
    Now we obey these values, and you can get close to the previous
    behaviour by setting buffer-time and latency-time manually. Note that
    they are set in microseconds.
    
    As a consequence, when we haven't received enough data from the
    device, we now sleep for a time proportional to the data remaining.
    However, Directsound is a deprecated API so it maintains its own
    software ringbuffer which updates at arbitrary intervals. Hence we
    might have to wait a full segsize to get the last 10% of data. To
    avoid tight loops, we clamp our sleep floor at 10ms.
    
    In my testing, this keeps the wakeups not-too-high (proportional to
    the latency-time set on the source). Further improvements should be
    made by fixing the WASAPI audio source plugin instead of this.
    Directsound is deprecated and as the comments explain, it is
    impossible to get low latency, decent quality, or good performance
    from it.
    
    Based on a patch by Sebastian Dröge <sebastian@centricular.com>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=781249
Comment 4 Sebastian Dröge (slomo) 2017-05-08 14:14:40 UTC
Please also apply changes like in https://bugzilla.gnome.org/show_bug.cgi?id=773681#c31 (also see my review comments for some cleanup needed on top of that). After that I think we can close this bug.
Comment 5 Nirbheek Chauhan 2017-05-10 12:42:53 UTC
Created attachment 351542 [details] [review]
Use GstClockID to wait instead of Sleep()

I've tested this on Windows 8.1 and it seems to work fine. Will push once I get a +1 from someone.
Comment 6 Sebastian Dröge (slomo) 2017-05-10 13:09:51 UTC
Comment on attachment 351542 [details] [review]
Use GstClockID to wait instead of Sleep()

Also push to 1.12 branch, and close this with target milestone 1.12.1 :)