[Merge] lp:~phablet-team/media-hub/pulse-output-change-pause into lp:media-hub
Ricardo Salveti
rsalveti at rsalveti.net
Thu Nov 13 06:23:24 UTC 2014
Review: Needs Fixing
Comments inline.
Diff comments:
> === modified file 'debian/control'
> --- debian/control 2014-11-11 20:18:36 +0000
> +++ debian/control 2014-11-12 19:10:35 +0000
> @@ -28,6 +28,7 @@
> gstreamer1.0-libav,
> libgstreamer1.0-dev,
> pkg-config,
> + libpulse-dev,
> qtbase5-dev,
> libtelepathy-qt5-dev,
> Standards-Version: 3.9.5
>
> === modified file 'src/core/media/CMakeLists.txt'
> --- src/core/media/CMakeLists.txt 2014-11-03 00:56:55 +0000
> +++ src/core/media/CMakeLists.txt 2014-11-12 19:10:35 +0000
> @@ -1,5 +1,6 @@
> pkg_check_modules(PC_GSTREAMER_1_0 REQUIRED gstreamer-1.0)
> -include_directories(${PC_GSTREAMER_1_0_INCLUDE_DIRS} ${HYBRIS_MEDIA_CFLAGS})
> +pkg_check_modules(PC_PULSE_AUDIO REQUIRED libpulse)
> +include_directories(${PC_GSTREAMER_1_0_INCLUDE_DIRS} ${HYBRIS_MEDIA_CFLAGS} ${PC_PULSE_AUDIO_INCLUDE_DIRS})
>
> # We compile with all symbols visible by default. For the shipping library, we strip
> # out all symbols that are not in core::ubuntu::media*
> @@ -100,6 +101,7 @@
> ${PROCESS_CPP_LDFLAGS}
> ${GIO_LIBRARIES}
> ${HYBRIS_MEDIA_LIBRARIES}
> + ${PC_PULSE_AUDIO_LIBRARIES}
> )
>
> include_directories(${PROJECT_SOURCE_DIR}/src/ ${HYBRIS_MEDIA_CFLAGS})
>
> === modified file 'src/core/media/service_implementation.cpp'
> --- src/core/media/service_implementation.cpp 2014-11-03 00:56:55 +0000
> +++ src/core/media/service_implementation.cpp 2014-11-12 19:10:35 +0000
> @@ -15,6 +15,8 @@
> *
> * Authored by: Thomas Voß <thomas.voss at canonical.com>
> * Jim Hodapp <jim.hodapp at canonical.com>
> + *
> + * Note: Some of the PulseAudio code was adapted from telepathy-ofono
> */
>
> #include "service_implementation.h"
> @@ -27,10 +29,13 @@
> #include <boost/asio.hpp>
>
> #include <cstdint>
> +#include <cstring>
> #include <map>
> #include <memory>
> #include <thread>
>
> +#include <pulse/pulseaudio.h>
> +
> #include "util/timeout.h"
> #include "unity_screen_service.h"
> #include <hybris/media/media_recorder_layer.h>
> @@ -45,6 +50,11 @@
> : resume_key(std::numeric_limits<std::uint32_t>::max()),
> keep_alive(io_service),
> disp_cookie(0),
> + pulse_mainloop_api(nullptr),
> + pulse_context(nullptr),
> + active_idx(-1),
> + headphones_connected(false),
> + a2dp_connected(false),
> call_monitor(new CallMonitor)
> {
> bus = std::shared_ptr<dbus::Bus>(new dbus::Bus(core::dbus::WellKnownBus::session));
> @@ -54,6 +64,21 @@
> bus->run();
> }));
>
> + // We use Pulse to detect when the user has unplugged speakers from the headphone jack
> + // or disconnected an A2DP bluetooth device
> + pulse_mainloop = pa_threaded_mainloop_new();
> + if (pulse_mainloop == nullptr)
> + std::cerr << "Unable to create pulseaudio mainloop, audio output detection will not function" << std::endl;
> +
> + if (pa_threaded_mainloop_start(pulse_mainloop) != 0)
> + {
> + std::cerr << "Unable to start pulseaudio mainloop, audio output detection will not function" << std::endl;
> + pa_threaded_mainloop_free(pulse_mainloop);
> + pulse_mainloop = nullptr;
> + }
> + else
> + create_pulse_context();
> +
> // Connect the property change signal that will allow media-hub to take appropriate action
> // when the battery level reaches critical
> auto stub_service = dbus::Service::use_service(bus, "com.canonical.indicator.power");
> @@ -74,6 +99,15 @@
>
> ~Private()
> {
> + release_pulse_context();
> +
> + if (pulse_mainloop != nullptr)
> + {
> + pa_threaded_mainloop_stop(pulse_mainloop);
> + pa_threaded_mainloop_free(pulse_mainloop);
> + pulse_mainloop = nullptr;
> + }
> +
> bus->stop();
>
> if (worker.joinable())
> @@ -116,6 +150,233 @@
> p->media_recording_started(started);
> }
>
> + pa_threaded_mainloop *mainloop()
> + {
> + return pulse_mainloop;
> + }
> +
> + bool is_port_available(pa_card_port_info **ports, uint32_t n_ports, const char *name)
> + {
> + bool ret = false;
> +
> + if (ports != nullptr && n_ports > 0 && name != nullptr)
> + {
> + for (uint32_t i=0; i<n_ports; i++)
> + {
> + //std::cout << "port name: " << ports[i]->name << std::endl;
> + //std::cout << "port available? " << ((ports[i]->available != PA_PORT_AVAILABLE_NO) ? "yes" : "no") << std::endl;
Please remove the commented lines here.
> + if (strstr(ports[i]->name, name) != nullptr && ports[i]->available != PA_PORT_AVAILABLE_NO)
> + {
> + ret = true;
> + break;
> + }
> + }
> + }
> +
> + return ret;
> + }
> +
> + void pause_playback_if_necessary(uint32_t idx)
> + {
> + if (active_idx != (int) idx)
> + return;
> +
> + if (a2dp_connected and headphones_connected)
> + return;
> +
> + pause_playback();
> + }
> +
> + void update_device_states(uint32_t idx)
> + {
> + const pa_operation *o = pa_context_get_card_info_by_index(pulse_context, idx,
> + [](pa_context *context, const pa_card_info *info, int eol, void *userdata)
> + {
> + (void) context;
> + (void) eol;
> +
> + if (info == nullptr || userdata == nullptr)
> + return;
> +
> + Private *p = reinterpret_cast<Private*>(userdata);
> + std::cout << "Getting card info from the context (cb)" << std::endl;
> + std::cout << "name: " << info->name << std::endl;
> + // TODO: Currently hardcoding card name, need to expand for desktop.
> + if (strcmp(info->name, "droid_card.primary") == 0)
> + {
> + if (p->is_port_available(info->ports, info->n_ports, "output-wired"))
> + {
> + std::cout << "Wired headphones detected" << std::endl;
> +
> + // Headphones only become active when there is no A2DP device present
> + if (p->a2dp_connected == false)
> + p->active_idx = info->index;
> +
> + p->headphones_connected = true;
> + }
> + else if (p->headphones_connected == true)
> + {
> + std::cout << "Wired headphones disconnected" << std::endl;
> +
> + p->headphones_connected = false;
> + if (p->a2dp_connected == false)
> + p->active_idx = info->index;
> +
> + p->pause_playback_if_necessary(info->index);
> + }
> + }
> +
> + // TODO: Whenever an A2DP device is present, it is the default media output
> + // due to the way things are currently setup in Pulse. This might change.
> + if (strstr(info->name, "bluez_card") != nullptr)
> + {
> + if (p->is_port_available(info->ports, info->n_ports, "headset-output"))
> + {
> + std::cout << "A2DP headset detected" << std::endl;
> + p->a2dp_connected = true;
> + p->active_idx = info->index;
> + }
> + }
One thing that is not necessarily right in this block is the tracking of the active_idx, as we're basically duplicating that info from pulse, without it necessarily being in sync. We can easily get to a situation where someone could change the default sink and affect our internal logic (on desktop we're not necessarily switching to A2DP by default).
I think the right way to track the active state here is by getting that info from the pulse server struct (where we can find the default_sink_name), and listen for SERVER events, updating our internal active sink var every time it changes. That way we don't necessarily need to make the a2dp a special case, but instead just pausing when the active sink is the one that was removed.
So in the end we'd need to track 2 things:
- Sink changes, matching sink name with sink.primary, and checking if the headset is now connected/disconnected (only handling the event if sink.primary is the active one)
- If the active sink gets removed (which could be a2dp, usb, etc)
> + }, this);
> + (void) o;
> + }
> +
> + void create_pulse_context()
> + {
> + if (pulse_context != nullptr)
> + return;
> +
> + bool keep_going = true, ok = true;
> +
> + pulse_mainloop_api = pa_threaded_mainloop_get_api(pulse_mainloop);
> + pa_threaded_mainloop_lock(pulse_mainloop);
> +
> + pulse_context = pa_context_new(pulse_mainloop_api, "MediaHubPulseContext");
> + pa_context_set_state_callback(pulse_context,
> + [](pa_context *context, void *userdata)
> + {
> + (void) context;
> + Private *p = reinterpret_cast<Private*>(userdata);
> + // Signals the pa_threaded_mainloop_wait below to proceed
> + pa_threaded_mainloop_signal(p->mainloop(), 0);
> + }, this);
> +
> + if (pulse_context == nullptr)
> + {
> + std::cerr << "Unable to create new pulseaudio context" << std::endl;
> + pa_threaded_mainloop_unlock(pulse_mainloop);
> + return;
> + }
> +
> + if (pa_context_connect(pulse_context, nullptr, PA_CONTEXT_NOAUTOSPAWN, nullptr) < 0)
> + {
> + std::cerr << "Unable to create a connection to the pulseaudio context" << std::endl;
> + pa_threaded_mainloop_unlock(pulse_mainloop);
> + release_pulse_context();
> + return;
> + }
> +
> + pa_threaded_mainloop_wait(pulse_mainloop);
> +
> + while (keep_going)
> + {
> + switch (pa_context_get_state(pulse_context))
> + {
> + case PA_CONTEXT_CONNECTING:
> + case PA_CONTEXT_AUTHORIZING:
> + case PA_CONTEXT_SETTING_NAME:
> + break;
> +
> + case PA_CONTEXT_READY:
> + std::cout << "Pulseaudio connection established." << std::endl;
> + keep_going = false;
> + break;
> +
> + case PA_CONTEXT_TERMINATED:
> + std::cerr << "Pulseaudio context terminated." << std::endl;
> + keep_going = false;
> + ok = false;
> + break;
> +
> + case PA_CONTEXT_FAILED:
> + default:
> + std::cerr << "Pulseaudio connection failure: " << pa_strerror(pa_context_errno(pulse_context));
> + keep_going = false;
> + ok = false;
> + }
> +
> + if (keep_going)
> + pa_threaded_mainloop_wait(pulse_mainloop);
> + }
> +
> + if (ok)
> + {
> + pa_context_set_state_callback(pulse_context,
> + [](pa_context *context, void *userdata)
> + {
> + (void) context;
> + (void) userdata;
> + }, this);
> + pa_context_set_subscribe_callback(pulse_context,
> + [](pa_context *context, pa_subscription_event_type_t t, uint32_t idx, void *userdata)
> + {
> + (void) context;
> +
> + if (userdata == nullptr)
> + return;
> +
> + Private *p = reinterpret_cast<Private*>(userdata);
> + std::cout << "subscribe_callback lambda" << std::endl;
> + if ((t & PA_SUBSCRIPTION_EVENT_FACILITY_MASK) == PA_SUBSCRIPTION_EVENT_CARD)
> + {
> + std::cout << "Card event" << std::endl;
> + if ((t & PA_SUBSCRIPTION_EVENT_TYPE_MASK) == PA_SUBSCRIPTION_EVENT_CHANGE)
> + {
> + std::cout << "Output event changed" << std::endl;
> + p->update_device_states(idx);
> + }
> + else if ((t & PA_SUBSCRIPTION_EVENT_TYPE_MASK) == PA_SUBSCRIPTION_EVENT_NEW)
> + {
> + std::cout << "Output event new" << std::endl;
> + p->update_device_states(idx);
> + }
> + else if ((t & PA_SUBSCRIPTION_EVENT_TYPE_MASK) == PA_SUBSCRIPTION_EVENT_REMOVE)
> + {
> + std::cout << "Output device removed" << std::endl;
> +
> + p->pause_playback_if_necessary(idx);
> + // TODO: Currently only A2DP devices (BT) get their cards removed on disconnection
> + if (p->a2dp_connected)
> + p->a2dp_connected = false;
> + }
> + }
> + }, this);
> + pa_context_subscribe(pulse_context, PA_SUBSCRIPTION_MASK_CARD, nullptr, this);
> + }
> + else
> + {
> + if (pulse_context != nullptr)
> + {
> + pa_context_unref(pulse_context);
> + pulse_context = nullptr;
> + }
> + }
> +
> + pa_threaded_mainloop_unlock(pulse_mainloop);
> + }
> +
> + void release_pulse_context()
> + {
> + if (pulse_context != nullptr)
> + {
> + pa_threaded_mainloop_lock(pulse_mainloop);
> + pa_context_disconnect(pulse_context);
> + pa_context_unref(pulse_context);
> + pa_threaded_mainloop_unlock(pulse_mainloop);
> + pulse_context = nullptr;
> + }
> + }
> +
> // This holds the key of the multimedia role Player instance that was paused
> // when the battery level reached 10% or 5%
> media::Player::PlayerKey resume_key;
> @@ -129,14 +390,21 @@
> int disp_cookie;
> std::shared_ptr<dbus::Object> uscreen_session;
> MediaRecorderObserver *observer;
> + pa_mainloop_api *pulse_mainloop_api;
> + pa_threaded_mainloop *pulse_mainloop;
> + pa_context *pulse_context;
> + // Gets signaled when both the headphone jack is removed or an A2DP device is
> + // disconnected and playback needs pausing
> + core::Signal<void> pause_playback;
> + int active_idx;
> + bool headphones_connected;
> + bool a2dp_connected;
> std::unique_ptr<CallMonitor> call_monitor;
> std::list<media::Player::PlayerKey> paused_sessions;
> };
>
> media::ServiceImplementation::ServiceImplementation() : d(new Private())
> {
> - cout << __PRETTY_FUNCTION__ << endl;
> -
> d->power_level->changed().connect([this](const core::IndicatorPower::PowerLevel::ValueType &level)
> {
> // When the battery level hits 10% or 5%, pause all multimedia sessions.
> @@ -153,6 +421,12 @@
> resume_multimedia_session();
> });
>
> + d->pause_playback.connect([this]()
> + {
> + std::cout << "Got pause_playback signal, pausing all multimedia sessions" << std::endl;
> + pause_all_multimedia_sessions();
> + });
> +
> d->call_monitor->on_change([this](CallMonitor::State state) {
> switch (state) {
> case CallMonitor::OffHook:
>
> === modified file 'src/core/media/service_skeleton.cpp'
> --- src/core/media/service_skeleton.cpp 2014-10-14 16:21:47 +0000
> +++ src/core/media/service_skeleton.cpp 2014-11-12 19:10:35 +0000
> @@ -14,6 +14,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> *
> * Authored by: Thomas Voß <thomas.voss at canonical.com>
> + * Jim Hodapp <jim.hodapp at canonical.com>
> */
>
> #include "service_skeleton.h"
>
> === modified file 'tests/unit-tests/CMakeLists.txt'
> --- tests/unit-tests/CMakeLists.txt 2014-11-03 00:56:55 +0000
> +++ tests/unit-tests/CMakeLists.txt 2014-11-12 19:10:35 +0000
> @@ -36,6 +36,7 @@
> ${PC_GSTREAMER_1_0_LIBRARIES}
> ${PROCESS_CPP_LDFLAGS}
> ${GIO_LIBRARIES}
> + ${PC_PULSE_AUDIO_LIBRARIES}
>
> gmock
> gmock_main
>
--
https://code.launchpad.net/~phablet-team/media-hub/pulse-output-change-pause/+merge/241452
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.
More information about the Ubuntu-reviews
mailing list