Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ao/alsa] Unable to set periods #6318

Closed
orbea opened this issue Nov 14, 2018 · 1 comment
Closed

[ao/alsa] Unable to set periods #6318

orbea opened this issue Nov 14, 2018 · 1 comment

Comments

@orbea
Copy link
Contributor

orbea commented Nov 14, 2018

mpv version and platform

OS: Slackware64-current
mpv - b222980
ffmpeg - FFmpeg/FFmpeg@36348d7
alsa-lib-1.1.7

Reproduction steps

  1. Stream any bandcamp link with mpv and --ao=alsa and --alsa-resample=yes.
  2. It should print [ao/alsa] Unable to set periods: Invalid argument right before the first track starts.
  3. It is only printed for a single track.

Expected behavior

It should not print [ao/alsa] Unable to set periods: Invalid argument.

Actual behavior

It prints [ao/alsa] Unable to set periods: Invalid argument once before the first track starts with all bancamp content when using --alsa-resample=yes.

Older mpv commits where I do not recall experiencing this are also affected, maybe there was a change in alsa or ffmpeg?

Log file

Log file - http://0x0.st/sl7z.txt

Sample files

https://massmediarecords.bandcamp.com/album/echoes-in-the-corridor
https://fangsonfur.bandcamp.com/album/fof-2

@orbea
Copy link
Contributor Author

orbea commented Nov 14, 2018

This doesn't occur in alsa-lib-1.1.6 and it also doesn't occur in the alsa-lib git master. It seems only 1.1.7 is affected.

I bisected alsa-lib for the first commit that fixed the issue.

$ git bisect bad
b420056604f06117c967b65d43d01536c5ffcbc9 is the first bad commit
commit b420056604f06117c967b65d43d01536c5ffcbc9
Author: Timo Wischer <twischer@de.adit-jv.com>
Date:   Thu Oct 18 13:33:24 2018 +0200

    pcm: interval: Interpret (x x+1] correctly and return x+1
    
    Without this change an interval of (x x+1] will be interpreted as an
    empty interval but the right value would be x+1.
    This leads to a failing snd_pcm_hw_params() call which returns -EINVAL.
    
    An example issue log is given in the following:
    snd_pcm_hw_params failed with err -22 (Invalid argument)
    ACCESS: MMAP_NONINTERLEAVED
    FORMAT: S16_LE
    SUBFORMAT: STD
    SAMPLE_BITS: 16
    FRAME_BITS: 16
    CHANNELS: 1
    RATE: 16000
    PERIOD_TIME: (15999 16000]
    PERIOD_SIZE: (255 256]
    PERIOD_BYTES: (510 512]
    PERIODS: [2 3)
    BUFFER_TIME: 32000
    BUFFER_SIZE: 512
    BUFFER_BYTES: 1024
    
    In case of (x x+1) we have to interpret it anyway as a single value of x to
    compensate rounding issues.
    For example the period size will result in an interval of (352 353) when
    the period time is 16ms and the sample rate 22050 Hz
    (16ms * 22,05 kHz = 352,8 frames). But 352 has to be chosen to allow a
    buffer size of 705 (32ms * 22,05 kHz = 705,6 frames) which has to be >= 2x
    period size to avoid Xruns. The buffer size will not end up with an
    interval of (705 706) simular to the period size because
    snd_pcm_rate_hw_refine_cchange() calls snd_interval_floor() for the buffer
    size. Therefore this value will be interpreted as an integer interval
    instead of a real interval further on.
    
    This issue seems to exist since the change of 9bb985c38 ("pcm:
    snd_interval_refine_first/last: exclude value only if also excluded
    before")
    
    Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
    Signed-off-by: Jaroslav Kysela <perex@perex.cz>

:040000 040000 9754f323091ac266a3cbe062ecf4d2a25b079e17 4a53192a2433cc8d15b6c64260678a8122f3b944 M	src

http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=b420056604f06117c967b65d43d01536c5ffcbc9

And applying the above commit as a patch to alsa-lib-1.1.7 solves this.

That said this made me realize Slackware current already has this patch, just the package wasn't correctly rebuilt in the main tree... I guess I will close this issue now.

@orbea orbea closed this as completed Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant