On Fri, 14 Feb 2025 11:03:15 -0500 Richard Sent <[email protected]> wrote:
> --- > Hi all, > > > Personally I have WM/DE hotkeys to tweak volume in audio output for > > that particular mpv instance (*), but yeah, it sounds like a good thing > > to have bundled with the player backend (to me at least), as it's very > > much tied to its API and provides kinda basic player functionality > > (that probably should've been there from the start). > > Understood. For now I'll keep it separate since it's easier for me to > track mentally but we can move it later. Yeah, thanks, it looks great. Guess I can merge and push it, unless anyone spots any issues in a couple days? I should probably add it to docs and to emms-volume.el as well, so that it'd be easier to know about and find/use in that case. It didn't occur to me at the time, but separate file seem to also make more sense in context of being a part of emms-volume.el subsystem too. > > "sound clipping starts to occur" point can only be measured by mpv and > > its audio processing pipeline, and don't think it exposes that via any > > properties in some very generic way. > > > > So best way is probably to just let user decide whether they want to > > change "volume" or "ao-volume", kinda like mpv itself does/supports. > > I've added emms-volume-mpv-method. This allows for pure "volume" > control, pure "ao-volume" control, and a "smart" mode. The smart mode > tries to avoid clipping by ensuring both "volume" and "ao-volume" are at > 100% before raising "volume" above 100%. When lowering volume, it first > drops "volume" to 100%, then lowers "ao-volume" to 0. This should allow > for mostly-system-integrated volume management while still supporting > mpv's software volume boost. Sorry, didn't mean it as in "should be changed to work like that" at all, was just thinking to comment on these volumes, but in retrospect guess it's easy to read like that. (as I'm supposed to be a maintainer or something, not just random person in a thread) But with a method-switch it definitely seems more flexible, and won't need e.g. someone wanting ao-volume directly to patch it later. > While ao-volume properties above 100% should be possible, I don't think > mpv supports them. Attempting to raise ao-volume above 100% causes this > error: > > (error "Failed to run (set_property ao-volume 105.0) with error unsupported > format for accessing property") > > Because ao-volume-max doesn't exist [1], this cap can't be changed. Ergo > I capped ao-volume at 100%. Don't think it really matters how it's done, but other way around that error that comes to mind is how mpv inputs do it - sending something like "add ao-volume 5" command, which should have mpv increase and cap the value as-needed. Iirc these commands don't return resulting value, so volume will still need to be fetched for minibuffer message, which seems important to echo instead of only changing stuff blindly. Guess alternative way to implement this with "add" commands in general can be to use emms-player-mpv-observe-property on 'volume and 'ao-volume from emms-player-mpv-event-connect-hook and process those to echo changing volume anytime via emms-player-mpv-event-functions. So that after e.g. "add volume -5" is processed and mpv changes volume, it'd notify emacs about "volume" property change and it'd be printed in minibuffer in a similar way. One advantage with observe_property would be that changing volume in mpv externally somehow - e.g. via pavucontrol or pressing hotkeys and GUI elements in mpv window - will also print info about changes in emacs, but dunno if it's needed there on those external changes. Also this allows to track and change these volumes via emacs-side variable (as observing property will keep it in sync), but don't think that's how emms-volume.el does it anyway, and maybe a bit too magical. Don't mean to say that anything needs changes at all, as I don't know how keeping volumes more in-sync like that would be useful. It just didn't occur to me earlier, so thought to mention that it can also be implemented in this other alternative way as well. (...guess maybe if there was "display emms volume emoji in mode line" widget or something like that, to avoid it needing to poll for volume(s) every N seconds, but then such widget can probably register its own observer hooks for its own purposes, doesn't really need to be rely on volume-changing implementation details) > While developing this patch I found some odd behavior when volume was > changed rapidly. Threads would indefinitely block on > emms-volume-mpv--ipc-sync-check and the volume would suddenly jump to > unrelated values. I think this was because of multiple threads calling > emms-volume-mpv-synchronous-ipc, which interacted with each other in odd > ways. It's also been my experience with threads that "put one sync mutex around as much as possible" tends to be the best way to side-step most quirks, and usually worth doing just in case, if possible. And most obvious emacs docs in particular seem to not guarantee strict cooperative multitasking behavior (where switching threads only happens at exact thread-yield calls or mutex/condition waits), saying that it's "mostly cooperative", so IMO seems best to just make these calls as sequential/non-concurrent as they can be, to be as future-proof against hard-to-debug threading quirks as possible. > One limitation of the current patch: ao-volume is only exposed while > mpv's audio output is active. With the 'system or 'smart methods, if the > user tries adjusting volume before mpv starts playing, or if the user > plays media without audio, the volume cannot be adjusted. I don't know > if this can be resolved. The initial value for ao-volume may change > depending on what it was when mpv was last run. Afaik that's also how mpv hotkeys (and commands, other UI elements) work - prints some error until ao-stream is actually created somehow after playback starts, where latter part can be delayed if it's from network source/fs, spun-down hdd or whatever. So seems like a current feature or mpv limitation in general. (and using e.g. "add ao-volume 1" command won't work around this either) I think if it's useful enough for emms to control this in a stricter way than how mpv itself does it, then emms-player-mpv-observe-property + emms-player-mpv-event-functions seem to be one obvious way to do it. I.e. observe 'ao-volume, which I think mpv should send immediately when it's available, and if it doesn't match what's set in emms - adjust it asap. To avoid emms completely taking control over ao-volume from other mpv inputs, alternatively maybe it can only be set by emms once, when it first sees that properly from mpv, and then unobserve or ignore it (leave up to mpv). Can see this kind of extra control being useful as an alternative to PULSE_PROP_media.role=music env var which I've mentioned in an earlier email (makes pulseaudio store emms-mpv stream volume separately). But same as above, it's just how I can think of implementing "this can be resolved" part, which I've no idea if useful enough or worth doing. -- Mike Kazantsev // fraggod.net
