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

WIP: Optional media player auto-resume, central management of device call-state responses #1318

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ferdnyc
Copy link
Member

@ferdnyc ferdnyc commented Mar 30, 2022

Add a new Telephony preference, 'after-unpause', default true.
(New UI in the preferences allows it to be switched on/off.)
When it's set to false, exiting the ringing and talking states
will still restore volume levels (if configured), but will leave
the media player(s) paused so the user can resume manually.

Fixes: #1316

The added preference UI:

image

@@ -182,6 +182,8 @@
<key name="talking-pause" type="b">
<default>true</default>
</key>
<key name="after-unpause" type="b">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the name here, but I tried to follow the "state-action" format of the other names, like "ringing-pause" and "talking-volume". (Maybe "end-unpause" would be a better choice?) Using "unpause" instead of "resume" was definitely a deliberate choice, to more directly indicate what exactly this controls.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "call ended" is probably the most recognizable term. "Call Ended" doesn't really flow from the other titles, but hey.

For the GSettings key I'd probably for ended-pause. Even though that's sort of inverted, the key itself isn't user-visible and flows from talking-pause, etc. I expect someone will ask for something to the effect of ended-volume eventually.

That's my two cents :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've already had a request (#1316 (comment)) for ended-delay, which I quickly erected a Somebody Else's Problem Field around.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "call ended" is probably the most recognizable term. "Call Ended" doesn't really flow from the other titles, but hey.

So, "Call Ended" for the UI string (the title of the new one-item list that is sure to grow longer eventually), ened-pause for the key? Works for me!

Hmmm. Tho... so, ended-pause should be false by default, then? And the preference would flip it to true, if it should not auto-unpause? (Ow, negatives! 🤯 ) Man, that really is kind of confusing. But, that's what comments are for, I guess!

Copy link
Member Author

@ferdnyc ferdnyc Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the preference would flip it to true, if it should not auto-unpause?

That would also make the key value be the inverse of the UI control. Ouch.

Maybe ended-resume really is a better idea.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ended-resume is fine, I was just thinking foobar-pause would refer to the paused state rather than "Pause" the action. I'm not into bikeshedding that particular one though :)

Copy link
Member Author

@ferdnyc ferdnyc Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyholmes *nod* ended-pause would still have the same problem, though, whether it's verb or noun. Normally we don't want to leave the players in a paused state after the call, so ended-pause would default to false. But then the UI switch still ends up being inverted, unless you word it something convoluted like, "Pause state continues after call end", or "Keep players paused at end of call".

But whenever I try to come up with a normal-person-speak explanation for what the switch in preferences does, it always contains some variation on "resume" and would be enabled by default. Which means its true/false states are the inverse of the most-obvious meaning of ended-pause.

It's fine. ended-resume seems perfectly workable, no reason not to just go with that.

Comment on lines +1007 to +1014
debug(`Unpaused ${name}`);
} else {
debug(`Cannot unpause ${name}, skipping`);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Hadn't intended to leave these in, but maybe that's not the worst thing.

src/service/plugins/telephony.js Outdated Show resolved Hide resolved
Comment on lines +113 to +120
* TODO: This potentially worsens the multi-device
* problem, since we may be erasing the player
* list when another device still needs it.
Copy link
Member Author

@ferdnyc ferdnyc Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment notes, this potentially exacerbates an existing problem that can come up when multiple devices are paired to the local machine. Solving that is beyond the scope of this change.

(The problem is that local player state is global to the extension (or should be), but the actions to manage that state are implemented in the per-device plugins. We may need to take a page from GNOME's Freedesktop's (I'm pretty sure) power-management, which allows applications to request and clear "inhibit" flags that will prevent various transitions, without providing them any direct control over the powersave state or functionality.

So, instead of the current logic here, something like:

  1. A device has an active incoming/outgoing call, for which it is configured to pause media
  2. It informs the MPRIS backend of this fact
  3. The backend pauses the media players and stores the device away as the reason for pausing
  4. A second device starts a call, and informs the backend
  5. The backend adds the second device's request to the list of reasons why playback is paused
  6. When either device ends its call, it informs the backend which removes that device from the list
  7. When all devices have cleared their requests, regardless of the order in which that happens, the list of reasons why playback can't continue will have become empty.
  8. The backend therefore resumes playback. (Assuming the user hasn't disabled that, via the new preference from this PR — which should probably be a global setting rather than a per-device one, if we move to an inhibit-based model).

Managing audio volumes is probably an even trickier juggling act, since the states there aren't as simple as "paused" and "not paused" and multiple devices' requests will have more complex interactions.

Also, open question: If a device disconnects from GSConnect, is its request deleted immediately? Deleted after a grace period (to see if it reconnects)? Deleted only if there are no other requests? Deleted only when there are other requests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds pretty reasonable.

As for the last question I don't really know if there's a good answer, but the best answer would probably be to err on the side of leaving the media in its current state (ie. paused). Likewise, if the playback is changed externally that should probably clear any pending "inhibitors" and assume the user has taken the steering wheel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyholmes That makes sense. So, if a device drops off unexpectedly, it still gets removed from the inhibit list immediately. (Because we have no idea what its call state is anymore, even if it comes back.) Buuuut, playback doesn't resume even if it's the last device on the list. At which point, since the list is empty, the MPRIS backend still throws away its stored player state and washes its hands of the whole thing, leaving it to the user to do as they please. (Basically, returning to its default state of not managing any player controls, where it will stay until a new call event comes in and prompts it to start the pause-and-monitor cycle over again.)

Copy link
Member Author

@ferdnyc ferdnyc Apr 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could post a notification to the Shell, too, when a device drops off unexpectedly. (Only in the specific case that it had an active call in progress and its inhibits were dropped. Otherwise, we're just pestering the user with irrelevancies.)

That way, they'll know why the auto resume "didn't work". (And hopefully not generate any new bug reports.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'd lean towards no notification myself, but I see the argument for it. I'll leave that to you :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyholmes So, to monitor devices with active pause tokens for unexpected disconnects from the MPRIS backend code, is a Gio.bus_watch_name() on each device's interface my best option? Or are there convenience functions somewhere I can use to ask, "This ID still connected?"

...Hrm, actually, that wouldn't work, I'd need to know if it'd disconnected and reconnected at any point as well. bus_watch_name() it is!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, now I see it. The telephony plugin's .disconnected() will be called (well all of the plugins' .disconnected()s will be called) if the device drops off, I can have it actively inform the backend that it's unexpectedly going bye-bye.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use the pattern from the component code, with a acquire()/release() pair that returns some no-op object with a "refCount" property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyholmes I ended up concluding that explicit ref/token counting didn't really buy me anything. I wanted to keep a central list (a Set, actually) of all devices with an active request, so its .size at any moment represents the count. That way:

  1. Any duplicate requests that may come through (for whatever reason) can just be processed normally and require zero special handling, since they won't affect the .size of the Set the way a new, valid request would.
  2. The device plugin instances don't have to hold onto any state at all, for their pause requests — they can statelessly fire off updates to the backend whenever a call event occurs that has some pause-affecting action enabled. The backend tracks the state of device requests right alongside the pause state it's already tracking for the media players.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I await your commit so I can have a good look :)

@ferdnyc
Copy link
Member Author

ferdnyc commented Apr 9, 2022

Central pause-management added

So, my latest commit seems to work well enough for managing player pause/unpause state. It DTRT for a single device's call transitions — I've discovered that I can't actually test it with multiple devices, because even though I have two phones paired to my desktop only one of them has a SIM card.

I haven't found any way to convince the other one to trigger any telephony events, since it's...
image

But for one device, the only time it's misbehaved so far has been when the media player state was wonky to begin with. (I had a VLC instance that was showing canPlay: false after being paused, which of course meant the backend failed to unpause it. No idea why, but seems like it was VLC's fault, not GSConnect's.)

There's still a bit of excess debugging output in the code, some or all of which I'd drop before merging, but for the moment it's useful for testing.

Having this logic in place for MPRIS but not the audio mixer seems a bit off, so implementing a similar system for volume/mute-change requests feels like a necessary next step. In fact, that's gotten me thinking about whether it'd be better to handle them both together, rather than separately.

Even without my changes, the preferences allow some weird configurations — for instance, you could configure GSConnect to pause media and mute volume while an incoming call rings, then leave the audio muted but unpause the media when the call is answered. (Nobody would actually want that, but it's possible.) That's not so bad, though, because it's contained to a single device, and GSConnect is only doing exactly what you told it to do (for some reason).

But with centrally-managed state, it's theoretically possible to reach more confusing outcomes without any particularly weird configuration. For example:

  1. Device A is configured to pause media while on a call
  2. Device B is set to mute audio while on a call.
  3. A call comes in on device A; media pauses
  4. A call comes in on device B, volume is muted

If the device A call ends before the device B call, the media players will immediately unpause (because it was the only pause request registered), but the volume will remain muted until the call on device B ends. But unlike the previous example, this no longer requires an actively-weird configuration on a single device. Now it occurs as a consequence of the interaction between two devices with totally reasonable configurations.

So, it kind of seems like multiple devices' requests should still affect (read: block/extend) each other, even if they're configured to trigger different actions.

@ferdnyc ferdnyc changed the title Make auto-resume of paused media optional WIP: Optional media player auto-resume, central management of device call-state responses Apr 9, 2022
@andyholmes
Copy link
Collaborator

...which of course meant the backend failed to unpause it

I think that's okay, in fact it's reassuring that this is the case when something goes wrong. I think the worst case scenario is someone's music suddenly blasting if something goes sideways.

That's not so bad, though, because it's contained to a single device, and GSConnect is only doing exactly what you told it to do (for some reason).

Agreed, you have to really try to shoot yourself in the foot with this.

So, it kind of seems like multiple devices' requests should still affect (read: block/extend) each other, even if they're configured to trigger different actions.

I think that's okay, too. It's good to have some logic here for these cases, but the likelihood someone will actually have to phones receiving calls at the same time is pretty unlikely. With a bit of a framework in place this can always be tweaked later, as has been the case with MPRIS handling in general.

@andyholmes andyholmes added the needs review A contribution that needs code review label Aug 5, 2022
Add a new Telephony preference, 'after-unpause', default true.
(New UI in the preferences allows it to be switched on/off.)
When it's set to false, exiting the ringing and talking states
will still restore volume levels (if configured), but will leave
the media player(s) paused so the user can resume manually.
@andyholmes
Copy link
Collaborator

Was this one good to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review A contribution that needs code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not auto resume after the call is over
2 participants