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_pipewire: add support for device selection #9734

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Jan 20, 2022

Open points:

  • Sometimes the process() callback is not fired by Pipewire, so the playback stays frozen. Needs to be investigated.
  • The proper hotplug driver-callbacks are not implemented because mpv only calls the hotplug handlers for the first AO that defines them, in this case it will be pulseaudio
  • No hotplug events are fired.

@t-8ch t-8ch mentioned this pull request Jan 20, 2022
2 tasks
@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 28, 2022

The random freezes are fixed now. This PR should be ready for review and testing now.

The hotplug points require changes to the larger AO logic.

Copy link
Member

@philipl philipl left a comment

Choose a reason for hiding this comment

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

I have to say it seems like a pretty convoluted mechanism just enumerate fundamental metadata. There's no imperative way to get it?

Anyway, It worked in my basic testing and does what it's supposed to do.

@Dudemanguy
Copy link
Member

The hotplug points require changes to the larger AO logic.

Just out of curiosity, what kind of changes would that be? I'm not doubting you, but pipewire code looks basically exactly like how wayland code looks so it's a bit surprising to hear that a higher level of abstraction is needed to handle hotplugging instead of it just living in ao_pipewire. I guess this has something to do with the client not being able to tell if pipewire is actually driving audio?

@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 29, 2022

I have to say it seems like a pretty convoluted mechanism just enumerate fundamental metadata. There's no imperative way to get it?
@philipl All the APIs which are using the remote protocol are based on callbacks defined in event structures.

@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 29, 2022

Just out of curiosity, what kind of changes would that be?

@Dudemanguy The problem is in ao_hotplug_get_device_list() in ao.c:

    for (int n = 0; audio_out_drivers[n]; n++) {
          const struct ao_driver *d = audio_out_drivers[n];
          if (d == &audio_out_null)
              break; // don't add unsafe/special entries
  
          struct ao *ao = ao_alloc(true, hp->global, hp->wakeup_cb, hp->wakeup_ctx,
                                   (char *)d->name);
          if (!ao)
              continue;
  
          if (ao->driver->hotplug_init) {
              if (!hp->ao && ao->driver->hotplug_init(ao) >= 0)
                  hp->ao = ao; // keep this one
              if (hp->ao && hp->ao->driver == d)
                  get_devices(hp->ao, list);
          } else {
              get_devices(ao, list);
          }
          if (ao != hp->ao)
              talloc_free(ao);
      }

This will make sure that only the first driver with a hotplug_init() method will have it called:

              if (!hp->ao && ao->driver->hotplug_init(ao) >= 0)
                  hp->ao = ao; // keep this one

Edit: seems to be intentional:

struct ao_hotplug {
     // A single AO instance is used to listen to hotplug events. It wouldn't
      // make much sense to allow multiple AO drivers; all sane platforms have
      // a single such audio API.
      // This is _not_ the same AO instance as used for playing audio.
      struct ao *ao;
}

@Dudemanguy
Copy link
Member

@t-8ch: Would it be better to just wait for the pipewire developer to come up with some way for clients to detect if pipewire is driving the audio?

@t-8ch
Copy link
Contributor Author

t-8ch commented Jan 29, 2022 via email

@philipl
Copy link
Member

philipl commented Jan 29, 2022

I'm fine with with merging as it is. Adding hot plug support on top is incremental, and there is value in what exists here right now,

Our allocated buffers should be big enough, but add some errorhandling
just in case.
@philipl
Copy link
Member

philipl commented Feb 4, 2022

@t-8ch Do you want to squash and I'll merge it, or do you want to wait for the detection work in pipewire?

@t-8ch
Copy link
Contributor Author

t-8ch commented Feb 4, 2022

@philipl I think it should be two commits. The cleanup commit is not actually related to the device list feature.

@philipl philipl merged commit 09343bc into mpv-player:master Feb 7, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants