From 38ed9e59addfcd102202926a42b8eb164a21d28e Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Sun, 24 May 2020 20:26:18 +0200 Subject: [PATCH 01/14] [listener] Update so that comments etc match actual use --- src/listener.c | 6 +++--- src/listener.h | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/listener.c b/src/listener.c index 39a35a1e..121b836f 100644 --- a/src/listener.c +++ b/src/listener.c @@ -80,15 +80,15 @@ listener_remove(notify notify_cb) } void -listener_notify(enum listener_event_type type) +listener_notify(short event_mask) { struct listener *listener; listener = listener_list; while (listener) { - if (type & listener->events) - listener->notify_cb(type); + if (event_mask & listener->events) + listener->notify_cb(event_mask & listener->events); listener = listener->next; } } diff --git a/src/listener.h b/src/listener.h index 13988835..b5e717ab 100644 --- a/src/listener.h +++ b/src/listener.h @@ -36,7 +36,8 @@ typedef void (*notify)(short event_mask); * Registers the given callback function to the given event types. * This function is not thread safe. Listeners must be added once at startup. * - * @param notify_cb Callback function + * @param notify_cb Callback function (should be a non-blocking function, + * especially when the event is from the player) * @param event_mask Event mask, one or more of LISTENER_* * @return 0 on success, -1 on failure */ @@ -57,10 +58,10 @@ listener_remove(notify notify_cb); * Calls the callback function of the registered listeners listening for the * given type of event. * - * @param type The event type, on of the LISTENER_* values + * @param event_mask Event mask, one or more of LISTENER_* * */ void -listener_notify(enum listener_event_type type); +listener_notify(short event_mask); #endif /* !__LISTENER_H__ */ From 3885b921113c0584a4acd572c0914e7f0d3312f7 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Sun, 24 May 2020 20:26:56 +0200 Subject: [PATCH 02/14] [worker] Remove not-so-useful log message --- src/worker.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/worker.c b/src/worker.c index 20a7146b..354a1f92 100644 --- a/src/worker.c +++ b/src/worker.c @@ -140,8 +140,6 @@ worker_execute(void (*cb)(void *), void *cb_arg, size_t arg_size, int delay) struct worker_arg *cmdarg; void *argcpy; - DPRINTF(E_DBG, L_MAIN, "Got worker execute request\n"); - cmdarg = calloc(1, sizeof(struct worker_arg)); if (!cmdarg) { From 3b033e48eee829ad8a6390bd194785f461bda627 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Sun, 24 May 2020 20:27:46 +0200 Subject: [PATCH 03/14] [player] Consolidate listener handling in one player trigger - wip The goal is to make the listener invokation more unified and less ad hoc. Also reduce risk of blocking/deadlocking player thread. --- src/outputs.c | 8 --- src/player.c | 138 ++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 100 insertions(+), 46 deletions(-) diff --git a/src/outputs.c b/src/outputs.c index f643925d..a6d62df0 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -772,8 +772,6 @@ outputs_device_add(struct output_device *add, bool new_deselect) device->advertised = 1; - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); - return device; } @@ -817,8 +815,6 @@ outputs_device_remove(struct output_device *remove) outputs_device_free(remove); vol_adjust(); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); } void @@ -915,8 +911,6 @@ outputs_device_volume_register(struct output_device *device, int absvol, int rel device->volume = rel_to_vol(relvol, outputs_master_volume); vol_adjust(); - - listener_notify(LISTENER_VOLUME); } int @@ -1099,8 +1093,6 @@ outputs_volume_set(int volume, output_status_cb cb) pending += ret; } - listener_notify(LISTENER_VOLUME); - return pending; } diff --git a/src/player.c b/src/player.c index 6d9ce000..d623de01 100644 --- a/src/player.c +++ b/src/player.c @@ -23,7 +23,7 @@ * - handle playback commands, status checks and events from other threads * - receive audio from the input thread and to own the playback buffer * - feed the outputs at the appropriate rate (controlled by the playback timer) - * - output device handling (partly outsourced to outputs.c) + * - output device handling (outsourced to outputs.c) * - notify about playback status changes * - maintain the playback queue * @@ -33,6 +33,15 @@ * not always obeyed, for instance some outputs do their setup in ways that * could block. * + * Listener events + * --------------- + * Events will be signaled via listener_notify(). The following rules apply to + * how this must be done in the code: + * - always use trigger_listener_notify() to make sure the callbacks that the listener + * makes do not block the player thread, and to avoid any risk of deadlocks + * - if the event is a result of an external command then trigger it when the + * command is completed, so generally after commands_exec_sync() + * */ #ifdef HAVE_CONFIG_H @@ -367,6 +376,27 @@ scrobble_cb(void *arg) } #endif +// Callback from the worker thread (async operation as it may block) +static void +trigger_listener_notify_cb(void *arg) +{ + short *events = arg; + + DPRINTF(E_DBG, L_PLAYER, "Executing listener notification %d\n", *events); + + listener_notify(*events); +} + +#define trigger_listener_notify(x) trigger_listener((x), __func__) + +static void +trigger_listener(short events, const char *who) +{ + DPRINTF(E_DBG, L_PLAYER, "Scheduling listener notification %d from %s\n", events, who); + + worker_execute(trigger_listener_notify_cb, &events, sizeof(events), 0); +} + /* * Add the song with the given id to the list of previously played songs */ @@ -461,7 +491,7 @@ status_update(enum play_status status) { player_state = status; - listener_notify(LISTENER_PLAYER); + trigger_listener_notify(LISTENER_PLAYER); } /* ------ All this is for dealing with metadata received from the input ----- */ @@ -1348,8 +1378,15 @@ device_add(void *arg, int *retval) new_deselect = (player_state == PLAY_PLAYING); device = outputs_device_add(device, new_deselect); - *retval = device ? 0 : -1; + if (!device) + { + *retval = -1; + return COMMAND_END; + } + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + + *retval = 0; return COMMAND_END; } @@ -1401,6 +1438,8 @@ device_remove_family(void *arg, int *retval) outputs_device_free(remove); + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + *retval = 0; return COMMAND_END; } @@ -1425,12 +1464,8 @@ device_streaming_cb(struct output_device *device, enum output_device_state statu if (!device) { DPRINTF(E_LOG, L_PLAYER, "Output device disappeared during streaming!\n"); - return; } - - DPRINTF(E_DBG, L_PLAYER, "Callback from %s device %s to device_streaming_cb (status %d)\n", device->type_name, device->name, status); - - if (status == OUTPUT_STATE_FAILED) + else if (status == OUTPUT_STATE_FAILED) { DPRINTF(E_LOG, L_PLAYER, "The %s device '%s' failed - attempting reconnect in %d sec\n", device->type_name, device->name, PLAYER_SPEAKER_RESURRECT_TIME); @@ -1445,11 +1480,19 @@ device_streaming_cb(struct output_device *device, enum output_device_state statu DPRINTF(E_INFO, L_PLAYER, "The %s device '%s' stopped\n", device->type_name, device->name); } else - outputs_device_cb_set(device, device_streaming_cb); + { + DPRINTF(E_DBG, L_PLAYER, "Callback from %s device %s to device_streaming_cb (status %d)\n", device->type_name, device->name, status); + outputs_device_cb_set(device, device_streaming_cb); + } + + // We don't do this in the other cb's because they are triggered by a command + // and thus the caller is responsible for making the notification when the + // command is completed + trigger_listener_notify(LISTENER_SPEAKER); } static void -device_command_cb(struct output_device *device, enum output_device_state status) +device_volume_cb(struct output_device *device, enum output_device_state status) { if (!device) { @@ -1457,7 +1500,7 @@ device_command_cb(struct output_device *device, enum output_device_state status) goto out; } - DPRINTF(E_DBG, L_PLAYER, "Callback from %s device %s to device_command_cb (status %d)\n", device->type_name, device->name, status); + DPRINTF(E_DBG, L_PLAYER, "Callback from %s device %s to device_volume_cb (status %d)\n", device->type_name, device->name, status); outputs_device_cb_set(device, device_streaming_cb); @@ -1565,8 +1608,8 @@ player_pmap(void *p) return "device_activate_cb"; else if (p == device_streaming_cb) return "device_streaming_cb"; - else if (p == device_command_cb) - return "device_command_cb"; + else if (p == device_volume_cb) + return "device_volume_cb"; else if (p == device_flush_cb) return "device_flush_cb"; else if (p == device_shutdown_cb) @@ -2720,7 +2763,7 @@ volume_set(void *arg, int *retval) volume = cmdarg->intval; - *retval = outputs_volume_set(volume, device_command_cb); + *retval = outputs_volume_set(volume, device_volume_cb); if (*retval > 0) return COMMAND_PENDING; // async @@ -2744,7 +2787,7 @@ volume_setrel_speaker(void *arg, int *retval) outputs_device_volume_register(device, -1, vol_param->volume); - *retval = outputs_device_volume_set(device, device_command_cb); + *retval = outputs_device_volume_set(device, device_volume_cb); if (*retval > 0) return COMMAND_PENDING; // async @@ -2768,7 +2811,7 @@ volume_setabs_speaker(void *arg, int *retval) outputs_device_volume_register(device, vol_param->volume, -1); - *retval = outputs_device_volume_set(device, device_command_cb); + *retval = outputs_device_volume_set(device, device_volume_cb); if (*retval > 0) return COMMAND_PENDING; // async @@ -2831,8 +2874,6 @@ repeat_set(void *arg, int *retval) return COMMAND_END; } - listener_notify(LISTENER_OPTIONS); - *retval = 0; return COMMAND_END; } @@ -2862,9 +2903,8 @@ shuffle_set(void *arg, int *retval) db_queue_inc_version(); } - // Update shuffle mode and notify listeners + // Update shuffle mode shuffle = new_shuffle; - listener_notify(LISTENER_OPTIONS); out: *retval = 0; @@ -2878,8 +2918,6 @@ consume_set(void *arg, int *retval) consume = cmdarg->intval; - listener_notify(LISTENER_OPTIONS); - *retval = 0; return COMMAND_END; } @@ -2894,8 +2932,6 @@ playerqueue_clear_history(void *arg, int *retval) cur_plversion++; // TODO [db_queue] need to update db queue version - listener_notify(LISTENER_QUEUE); - *retval = 0; return COMMAND_END; } @@ -3083,8 +3119,8 @@ player_speaker_set(uint64_t *ids) speaker_set_param.device_ids = ids; ret = commands_exec_sync(cmdbase, speaker_set, NULL, &speaker_set_param); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3121,8 +3157,8 @@ player_speaker_enable(uint64_t id) int ret; ret = commands_exec_sync(cmdbase, speaker_enable, NULL, &id); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3133,8 +3169,8 @@ player_speaker_disable(uint64_t id) int ret; ret = commands_exec_sync(cmdbase, speaker_disable, NULL, &id); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3149,8 +3185,8 @@ player_speaker_prevent_playback_set(uint64_t id, bool prevent_playback) param.prevent_playback = prevent_playback; ret = commands_exec_sync(cmdbase, speaker_prevent_playback_set, speaker_prevent_playback_set_bh, ¶m); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3165,8 +3201,8 @@ player_speaker_busy_set(uint64_t id, bool busy) param.busy = busy; ret = commands_exec_sync(cmdbase, speaker_busy_set, speaker_prevent_playback_set_bh, ¶m); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3175,12 +3211,13 @@ void player_speaker_resurrect(void *arg) { struct speaker_set_param param; + int ret; param.device_ids = (uint64_t *)arg; - commands_exec_sync(cmdbase, speaker_resurrect, speaker_resurrect_bh, ¶m); - - listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, speaker_resurrect, speaker_resurrect_bh, ¶m); + if (ret >= 0) + trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); } int @@ -3198,6 +3235,9 @@ player_volume_set(int vol) cmdarg.intval = vol; ret = commands_exec_sync(cmdbase, volume_set, NULL, &cmdarg); + if (ret >= 0) + trigger_listener_notify(LISTENER_VOLUME); + return ret; } @@ -3217,6 +3257,9 @@ player_volume_setrel_speaker(uint64_t id, int relvol) vol_param.volume = relvol; ret = commands_exec_sync(cmdbase, volume_setrel_speaker, NULL, &vol_param); + if (ret >= 0) + trigger_listener_notify(LISTENER_VOLUME); + return ret; } @@ -3236,6 +3279,9 @@ player_volume_setabs_speaker(uint64_t id, int vol) vol_param.volume = vol; ret = commands_exec_sync(cmdbase, volume_setabs_speaker, NULL, &vol_param); + if (ret >= 0) + trigger_listener_notify(LISTENER_VOLUME); + return ret; } @@ -3249,6 +3295,9 @@ player_volume_update_speaker(uint64_t id, const char *volstr) vol_param.volstr = volstr; ret = commands_exec_sync(cmdbase, volume_update_speaker, NULL, &vol_param); + if (ret >= 0) + trigger_listener_notify(LISTENER_VOLUME); + return ret; } @@ -3258,6 +3307,9 @@ player_repeat_set(enum repeat_mode mode) int ret; ret = commands_exec_sync(cmdbase, repeat_set, NULL, &mode); + if (ret >= 0) + trigger_listener_notify(LISTENER_OPTIONS); + return ret; } @@ -3270,6 +3322,9 @@ player_shuffle_set(int enable) cmdarg.intval = enable; ret = commands_exec_sync(cmdbase, shuffle_set, NULL, &cmdarg); + if (ret >= 0) + trigger_listener_notify(LISTENER_OPTIONS); + return ret; } @@ -3282,13 +3337,20 @@ player_consume_set(int enable) cmdarg.intval = enable; ret = commands_exec_sync(cmdbase, consume_set, NULL, &cmdarg); + if (ret >= 0) + trigger_listener_notify(LISTENER_OPTIONS); + return ret; } void player_queue_clear_history() { - commands_exec_sync(cmdbase, playerqueue_clear_history, NULL, NULL); + int ret; + + ret = commands_exec_sync(cmdbase, playerqueue_clear_history, NULL, NULL); + if (ret >= 0) + trigger_listener_notify(LISTENER_QUEUE); } void From 010185eab5428ed6b5e6e44fa74baed9c8847f0c Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Sun, 24 May 2020 23:53:38 +0200 Subject: [PATCH 04/14] [player] Another way of implementing commit 3b033e48 --- src/player.c | 149 ++++++++++++++++++++++++--------------------------- 1 file changed, 69 insertions(+), 80 deletions(-) diff --git a/src/player.c b/src/player.c index d623de01..e80b8e5f 100644 --- a/src/player.c +++ b/src/player.c @@ -37,10 +37,10 @@ * --------------- * Events will be signaled via listener_notify(). The following rules apply to * how this must be done in the code: - * - always use trigger_listener_notify() to make sure the callbacks that the listener + * - always use status_update() to make sure the callbacks that the listener * makes do not block the player thread, and to avoid any risk of deadlocks * - if the event is a result of an external command then trigger it when the - * command is completed, so generally after commands_exec_sync() + * command is completed, so generally in a bottom half * */ @@ -376,25 +376,16 @@ scrobble_cb(void *arg) } #endif -// Callback from the worker thread (async operation as it may block) +// This is just to be able to log the caller in a simple way +#define status_update(x, y) status_update_impl((x), (y), __func__) static void -trigger_listener_notify_cb(void *arg) +status_update_impl(enum play_status status, short listener_events, const char *caller) { - short *events = arg; + DPRINTF(E_DBG, L_PLAYER, "Status update - status: %d, events: %d, caller: %s\n", status, listener_events, caller); - DPRINTF(E_DBG, L_PLAYER, "Executing listener notification %d\n", *events); + player_state = status; - listener_notify(*events); -} - -#define trigger_listener_notify(x) trigger_listener((x), __func__) - -static void -trigger_listener(short events, const char *who) -{ - DPRINTF(E_DBG, L_PLAYER, "Scheduling listener notification %d from %s\n", events, who); - - worker_execute(trigger_listener_notify_cb, &events, sizeof(events), 0); + listener_notify(listener_events); } /* @@ -486,13 +477,6 @@ queue_item_prev(uint32_t item_id) return db_queue_fetch_prev(item_id, shuffle); } -static void -status_update(enum play_status status) -{ - player_state = status; - - trigger_listener_notify(LISTENER_PLAYER); -} /* ------ All this is for dealing with metadata received from the input ----- */ @@ -569,7 +553,7 @@ metadata_finalize(void *arg, int *retval) outputs_metadata_send(pb_session.playing_now->item_id, false, metadata_finalize_cb); - status_update(player_state); + status_update(player_state, LISTENER_PLAYER); return COMMAND_END; } @@ -1103,7 +1087,7 @@ event_play_start() session_update_play_start(); - status_update(PLAY_PLAYING); + status_update(PLAY_PLAYING, LISTENER_PLAYER); } static void @@ -1384,7 +1368,7 @@ device_add(void *arg, int *retval) return COMMAND_END; } - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + status_update(player_state, LISTENER_SPEAKER | LISTENER_VOLUME); *retval = 0; return COMMAND_END; @@ -1438,7 +1422,7 @@ device_remove_family(void *arg, int *retval) outputs_device_free(remove); - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + status_update(player_state, LISTENER_SPEAKER | LISTENER_VOLUME); *retval = 0; return COMMAND_END; @@ -1486,9 +1470,9 @@ device_streaming_cb(struct output_device *device, enum output_device_state statu } // We don't do this in the other cb's because they are triggered by a command - // and thus the caller is responsible for making the notification when the - // command is completed - trigger_listener_notify(LISTENER_SPEAKER); + // and thus the update should be done as part of the command completion (which + // can better determine which type of listener event to use) + status_update(player_state, LISTENER_SPEAKER); } static void @@ -1736,7 +1720,7 @@ pb_session_stop(void) session_stop(); - status_update(PLAY_STOPPED); + status_update(PLAY_STOPPED, LISTENER_PLAYER); } static void @@ -1831,7 +1815,7 @@ pb_suspend(void) pb_timer_stop(); - status_update(PLAY_PAUSED); + status_update(PLAY_PAUSED, LISTENER_PLAYER); seek_save(); @@ -1940,7 +1924,7 @@ playback_stop(void *arg, int *retval) // Stops the input pb_session_stop(); - status_update(PLAY_STOPPED); + status_update(PLAY_STOPPED, LISTENER_PLAYER); // We're async if we need to flush devices if (*retval > 0) @@ -1967,7 +1951,9 @@ playback_start_bh(void *arg, int *retval) if (ret < 0) goto error; - status_update(PLAY_PLAYING); + // We also ask listeners to update speaker/volume state, since it is possible + // some of the speakers we tried to start responded with failure + status_update(PLAY_PLAYING, LISTENER_PLAYER | LISTENER_SPEAKER | LISTENER_VOLUME); *retval = 0; return COMMAND_END; @@ -1991,7 +1977,7 @@ playback_start_item(void *arg, int *retval) { DPRINTF(E_DBG, L_PLAYER, "Player is already playing, ignoring call to playback start\n"); - status_update(player_state); + status_update(player_state, LISTENER_PLAYER); *retval = 1; // Value greater 0 will prevent execution of the bottom half function return COMMAND_END; @@ -2382,7 +2368,7 @@ playback_pause_bh(void *arg, int *retval) goto error; } - status_update(PLAY_PAUSED); + status_update(PLAY_PAUSED, LISTENER_PLAYER); *retval = 0; return COMMAND_END; @@ -2613,6 +2599,13 @@ speaker_disable(void *arg, int *retval) return COMMAND_END; } +static enum command_state +speaker_generic_bh(void *arg, int *retval) +{ + status_update(player_state, LISTENER_SPEAKER | LISTENER_VOLUME); + return COMMAND_END; +} + /* * Airplay speakers can via DACP set the "busy" + "prevent-playback" properties, * which we handle below. We try to do this like iTunes, except we need to @@ -2667,6 +2660,8 @@ speaker_prevent_playback_set_bh(void *arg, int *retval) DPRINTF(E_INFO, L_PLAYER, "Ending playback, speaker (id=%" PRIu64 ") set 'busy' or 'prevent-playback' flag\n", param->spk_id); pb_abort(); // TODO Would be better for the user if we paused, but we don't have a handy function for that } + else + status_update(player_state, LISTENER_SPEAKER | LISTENER_VOLUME); *retval = 0; return COMMAND_END; @@ -2751,6 +2746,8 @@ speaker_resurrect_bh(void *arg, int *retval) if (player_state == PLAY_PAUSED) return playback_start_bh(arg, retval); + status_update(player_state, LISTENER_SPEAKER | LISTENER_VOLUME); + *retval = 0; return COMMAND_END; } @@ -2849,6 +2846,13 @@ volume_update_speaker(void *arg, int *retval) return COMMAND_END; } +static enum command_state +volume_generic_bh(void *arg, int *retval) +{ + status_update(player_state, LISTENER_VOLUME); + return COMMAND_END; +} + static enum command_state repeat_set(void *arg, int *retval) { @@ -2922,6 +2926,13 @@ consume_set(void *arg, int *retval) return COMMAND_END; } +static enum command_state +options_generic_bh(void *arg, int *retval) +{ + status_update(player_state, LISTENER_OPTIONS); + return COMMAND_END; +} + /* * Removes all items from the history */ @@ -2946,6 +2957,12 @@ playerqueue_plid(void *arg, int *retval) return COMMAND_END; } +static enum command_state +playerqueue_generic_bh(void *arg, int *retval) +{ + status_update(player_state, LISTENER_QUEUE); + return COMMAND_END; +} /* ------------------------------- Player API ------------------------------- */ @@ -2992,6 +3009,7 @@ player_playback_start(void) int ret; ret = commands_exec_sync(cmdbase, playback_start, playback_start_bh, NULL); + return ret; } @@ -3011,6 +3029,7 @@ player_playback_start_byitem(struct db_queue_item *queue_item) int ret; ret = commands_exec_sync(cmdbase, playback_start_item, playback_start_bh, queue_item); + return ret; } @@ -3023,6 +3042,7 @@ player_playback_start_byid(uint32_t id) cmdarg.id = id; ret = commands_exec_sync(cmdbase, playback_start_id, playback_start_bh, &cmdarg); + return ret; } @@ -3118,9 +3138,7 @@ player_speaker_set(uint64_t *ids) speaker_set_param.device_ids = ids; - ret = commands_exec_sync(cmdbase, speaker_set, NULL, &speaker_set_param); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, speaker_set, speaker_generic_bh, &speaker_set_param); return ret; } @@ -3156,9 +3174,7 @@ player_speaker_enable(uint64_t id) { int ret; - ret = commands_exec_sync(cmdbase, speaker_enable, NULL, &id); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, speaker_enable, speaker_generic_bh, &id); return ret; } @@ -3168,9 +3184,7 @@ player_speaker_disable(uint64_t id) { int ret; - ret = commands_exec_sync(cmdbase, speaker_disable, NULL, &id); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, speaker_disable, speaker_generic_bh, &id); return ret; } @@ -3185,8 +3199,6 @@ player_speaker_prevent_playback_set(uint64_t id, bool prevent_playback) param.prevent_playback = prevent_playback; ret = commands_exec_sync(cmdbase, speaker_prevent_playback_set, speaker_prevent_playback_set_bh, ¶m); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3201,8 +3213,6 @@ player_speaker_busy_set(uint64_t id, bool busy) param.busy = busy; ret = commands_exec_sync(cmdbase, speaker_busy_set, speaker_prevent_playback_set_bh, ¶m); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); return ret; } @@ -3211,13 +3221,10 @@ void player_speaker_resurrect(void *arg) { struct speaker_set_param param; - int ret; param.device_ids = (uint64_t *)arg; - ret = commands_exec_sync(cmdbase, speaker_resurrect, speaker_resurrect_bh, ¶m); - if (ret >= 0) - trigger_listener_notify(LISTENER_SPEAKER | LISTENER_VOLUME); + commands_exec_sync(cmdbase, speaker_resurrect, speaker_resurrect_bh, ¶m); } int @@ -3234,9 +3241,7 @@ player_volume_set(int vol) cmdarg.intval = vol; - ret = commands_exec_sync(cmdbase, volume_set, NULL, &cmdarg); - if (ret >= 0) - trigger_listener_notify(LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, volume_set, volume_generic_bh, &cmdarg); return ret; } @@ -3256,9 +3261,7 @@ player_volume_setrel_speaker(uint64_t id, int relvol) vol_param.spk_id = id; vol_param.volume = relvol; - ret = commands_exec_sync(cmdbase, volume_setrel_speaker, NULL, &vol_param); - if (ret >= 0) - trigger_listener_notify(LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, volume_setrel_speaker, volume_generic_bh, &vol_param); return ret; } @@ -3278,9 +3281,7 @@ player_volume_setabs_speaker(uint64_t id, int vol) vol_param.spk_id = id; vol_param.volume = vol; - ret = commands_exec_sync(cmdbase, volume_setabs_speaker, NULL, &vol_param); - if (ret >= 0) - trigger_listener_notify(LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, volume_setabs_speaker, volume_generic_bh, &vol_param); return ret; } @@ -3294,9 +3295,7 @@ player_volume_update_speaker(uint64_t id, const char *volstr) vol_param.spk_id = id; vol_param.volstr = volstr; - ret = commands_exec_sync(cmdbase, volume_update_speaker, NULL, &vol_param); - if (ret >= 0) - trigger_listener_notify(LISTENER_VOLUME); + ret = commands_exec_sync(cmdbase, volume_update_speaker, volume_generic_bh, &vol_param); return ret; } @@ -3306,9 +3305,7 @@ player_repeat_set(enum repeat_mode mode) { int ret; - ret = commands_exec_sync(cmdbase, repeat_set, NULL, &mode); - if (ret >= 0) - trigger_listener_notify(LISTENER_OPTIONS); + ret = commands_exec_sync(cmdbase, repeat_set, options_generic_bh, &mode); return ret; } @@ -3321,9 +3318,7 @@ player_shuffle_set(int enable) cmdarg.intval = enable; - ret = commands_exec_sync(cmdbase, shuffle_set, NULL, &cmdarg); - if (ret >= 0) - trigger_listener_notify(LISTENER_OPTIONS); + ret = commands_exec_sync(cmdbase, shuffle_set, options_generic_bh, &cmdarg); return ret; } @@ -3336,9 +3331,7 @@ player_consume_set(int enable) cmdarg.intval = enable; - ret = commands_exec_sync(cmdbase, consume_set, NULL, &cmdarg); - if (ret >= 0) - trigger_listener_notify(LISTENER_OPTIONS); + ret = commands_exec_sync(cmdbase, consume_set, options_generic_bh, &cmdarg); return ret; } @@ -3346,11 +3339,7 @@ player_consume_set(int enable) void player_queue_clear_history() { - int ret; - - ret = commands_exec_sync(cmdbase, playerqueue_clear_history, NULL, NULL); - if (ret >= 0) - trigger_listener_notify(LISTENER_QUEUE); + commands_exec_sync(cmdbase, playerqueue_clear_history, playerqueue_generic_bh, NULL); } void From 9308f812247b509ffdeba143a06f29853eb66d0b Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Mon, 25 May 2020 00:04:33 +0200 Subject: [PATCH 05/14] [player] Get rid of listener notifications direct from outputs Try to consolidate in player.c --- src/outputs.c | 9 --------- src/outputs.h | 3 --- src/outputs/raop.c | 6 ------ 3 files changed, 18 deletions(-) diff --git a/src/outputs.c b/src/outputs.c index a6d62df0..06557a8d 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -34,7 +34,6 @@ #include "logger.h" #include "misc.h" #include "transcode.h" -#include "listener.h" #include "db.h" #include "player.h" //TODO remove me when player_pmap is removed again #include "worker.h" @@ -683,14 +682,6 @@ outputs_cb(int callback_id, uint64_t device_id, enum output_device_state state) event_active(outputs_deferredev, 0, 0); } -// Maybe not so great, seems it would be better if integrated into the callback -// mechanism so that the notifications where at least deferred -void -outputs_listener_notify(void) -{ - listener_notify(LISTENER_SPEAKER); -} - /* ---------------------------- Called by player ---------------------------- */ diff --git a/src/outputs.h b/src/outputs.h index 26b01809..2585298c 100644 --- a/src/outputs.h +++ b/src/outputs.h @@ -275,9 +275,6 @@ outputs_quality_unsubscribe(struct media_quality *quality); void outputs_cb(int callback_id, uint64_t device_id, enum output_device_state); -void -outputs_listener_notify(void); - /* ---------------------------- Called by player ---------------------------- */ int diff --git a/src/outputs/raop.c b/src/outputs/raop.c index 87cd461d..35fe7be9 100644 --- a/src/outputs/raop.c +++ b/src/outputs/raop.c @@ -1755,10 +1755,6 @@ raop_status(struct raop_session *rs) outputs_cb(rs->callback_id, rs->device_id, state); rs->callback_id = -1; - - // Ugly... fixme... - if (rs->state == RAOP_STATE_UNVERIFIED) - outputs_listener_notify(); } static struct raop_master_session * @@ -4203,8 +4199,6 @@ raop_cb_verification_verify_step2(struct evrtsp_request *req, void *arg) raop_send_req_options(rs, raop_cb_startup_options, "verify_step2"); - outputs_listener_notify(); - return; error: From 2fa2d33602887035a4a18c219722d65564cceba0 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Tue, 26 May 2020 22:42:06 +0200 Subject: [PATCH 06/14] [outputs] Add a more standard device authentication function outputs_authorize() has two issues, one that the caller can't specify device (problem if there are two devices waiting for verification), the other that it didn't offer a standard callback, so difficult to catch failure/success. --- src/outputs.c | 13 +++++++ src/outputs.h | 6 ++++ src/outputs/raop.c | 89 ++++++++++++++++++++++++++++++++-------------- 3 files changed, 81 insertions(+), 27 deletions(-) diff --git a/src/outputs.c b/src/outputs.c index 06557a8d..bc3b6540 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -942,6 +942,19 @@ outputs_device_quality_set(struct output_device *device, struct media_quality *q return device_state_update(device, ret); } +int +outputs_device_authorize(struct output_device *device, const char *pin, output_status_cb cb) +{ + int ret; + + if (outputs[device->type]->disabled || !outputs[device->type]->device_authorize) + return -1; + + ret = outputs[device->type]->device_authorize(device, pin, callback_add(device, cb)); + + return device_state_update(device, ret); +} + void outputs_device_cb_set(struct output_device *device, output_status_cb cb) { diff --git a/src/outputs.h b/src/outputs.h index 2585298c..6b55b8b9 100644 --- a/src/outputs.h +++ b/src/outputs.h @@ -226,6 +226,9 @@ struct output_definition // Request a change of quality from the device int (*device_quality_set)(struct output_device *device, struct media_quality *quality, int callback_id); + // Authorize forked-daapd to use the device + int (*device_authorize)(struct output_device *device, const char *pin, int callback_id); + // Change the call back associated with a device void (*device_cb_set)(struct output_device *device, int callback_id); @@ -319,6 +322,9 @@ outputs_device_volume_to_pct(struct output_device *device, const char *value); int outputs_device_quality_set(struct output_device *device, struct media_quality *quality, output_status_cb cb); +int +outputs_device_authorize(struct output_device *device, const char *pin, output_status_cb cb); + void outputs_device_cb_set(struct output_device *device, output_status_cb cb); diff --git a/src/outputs/raop.c b/src/outputs/raop.c index 35fe7be9..c04abcdf 100644 --- a/src/outputs/raop.c +++ b/src/outputs/raop.c @@ -4278,20 +4278,20 @@ raop_cb_verification_setup_step3(struct evrtsp_request *req, void *arg) ret = raop_verification_response_process(3, req, rs); if (ret < 0) - goto error; + goto out; ret = verification_setup_result(&authorization_key, rs->verification_setup_ctx); if (ret < 0) { DPRINTF(E_LOG, L_RAOP, "Verification setup result error: %s\n", verification_setup_errmsg(rs->verification_setup_ctx)); - goto error; + goto out; } DPRINTF(E_LOG, L_RAOP, "Verification setup stage complete, saving authorization key\n"); device = outputs_device_get(rs->device_id); if (!device) - goto error; + goto out; free(device->auth_key); device->auth_key = strdup(authorization_key); @@ -4299,14 +4299,19 @@ raop_cb_verification_setup_step3(struct evrtsp_request *req, void *arg) // A blocking db call... :-~ db_speaker_save(device); - // The player considers this session failed, so we don't need it any more - session_cleanup(rs); + // No longer UNVERIFIED + rs->state = RAOP_STATE_STOPPED; - /* Fallthrough */ - - error: + out: verification_setup_free(rs->verification_setup_ctx); rs->verification_setup_ctx = NULL; + + // Callback to player with result + raop_status(rs); + + // We are telling the player that the device is now stopped, so we don't need + // the session any more + session_cleanup(rs); } static void @@ -4328,6 +4333,7 @@ raop_cb_verification_setup_step2(struct evrtsp_request *req, void *arg) error: verification_setup_free(rs->verification_setup_ctx); rs->verification_setup_ctx = NULL; + raop_status(rs); } static void @@ -4349,13 +4355,37 @@ raop_cb_verification_setup_step1(struct evrtsp_request *req, void *arg) error: verification_setup_free(rs->verification_setup_ctx); rs->verification_setup_ctx = NULL; + raop_status(rs); +} + +static int +raop_verification_setup(struct raop_session *rs, const char *pin) +{ + int ret; + + rs->verification_setup_ctx = verification_setup_new(pin); + if (!rs->verification_setup_ctx) + { + DPRINTF(E_LOG, L_RAOP, "Out of memory for verification setup context\n"); + return -1; + } + + ret = raop_verification_request_send(1, rs, raop_cb_verification_setup_step1); + if (ret < 0) + goto error; + + return 0; + + error: + verification_setup_free(rs->verification_setup_ctx); + rs->verification_setup_ctx = NULL; + return -1; } static void -raop_verification_setup(const char *pin) +raop_authorize(const char *pin) { struct raop_session *rs; - int ret; for (rs = raop_sessions; rs; rs = rs->next) { @@ -4369,23 +4399,27 @@ raop_verification_setup(const char *pin) return; } - rs->verification_setup_ctx = verification_setup_new(pin); - if (!rs->verification_setup_ctx) - { - DPRINTF(E_LOG, L_RAOP, "Out of memory for verification setup context\n"); - return; - } - - ret = raop_verification_request_send(1, rs, raop_cb_verification_setup_step1); - if (ret < 0) - goto error; - - return; - - error: - verification_setup_free(rs->verification_setup_ctx); - rs->verification_setup_ctx = NULL; + raop_verification_setup(rs, pin); } + +static int +raop_device_authorize(struct output_device *device, const char *pin, int callback_id) +{ + struct raop_session *rs = device->session; + int ret; + + if (!rs) + return -1; + + ret = raop_verification_setup(rs, pin); + if (ret < 0) + return -1; + + rs->callback_id = callback_id; + + return 1; +} + #else static int raop_verification_verify(struct raop_session *rs) @@ -5011,6 +5045,7 @@ struct output_definition output_raop = .metadata_send = raop_metadata_send, .metadata_purge = raop_metadata_purge, #ifdef RAOP_VERIFICATION - .authorize = raop_verification_setup, + .device_authorize = raop_device_authorize, + .authorize = raop_authorize, #endif }; From 18e75c244558e23b6c12d7193f0c3eb4614d18e2 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Tue, 26 May 2020 22:45:38 +0200 Subject: [PATCH 07/14] [player] Add player_speaker_authorize() A more straightforward function for device verification, using the new outputs_device_authorize() function. --- src/player.c | 36 +++++++++++++++++++++++++++++++++++- src/player.h | 3 +++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/player.c b/src/player.c index e80b8e5f..77e55468 100644 --- a/src/player.c +++ b/src/player.c @@ -147,6 +147,8 @@ struct speaker_attr_param bool prevent_playback; bool busy; + + const char *pin; }; struct speaker_get_param @@ -1552,7 +1554,7 @@ device_activate_cb(struct output_device *device, enum output_device_state status retval = commands_exec_returnvalue(cmdbase); if (!device) { - DPRINTF(E_WARN, L_PLAYER, "Output device disappeared during startup!\n"); + DPRINTF(E_WARN, L_PLAYER, "Output device disappeared during activation!\n"); if (retval != -2) retval = -1; @@ -2752,6 +2754,24 @@ speaker_resurrect_bh(void *arg, int *retval) return COMMAND_END; } +static enum command_state +speaker_authorize(void *arg, int *retval) +{ + struct speaker_attr_param *param = arg; + struct output_device *device; + + device = outputs_device_get(param->spk_id); + if (!device) + return COMMAND_END; + + *retval = outputs_device_authorize(device, param->pin, device_activate_cb); + + if (*retval > 0) + return COMMAND_PENDING; // async + + return COMMAND_END; +} + static enum command_state volume_set(void *arg, int *retval) { @@ -3227,6 +3247,20 @@ player_speaker_resurrect(void *arg) commands_exec_sync(cmdbase, speaker_resurrect, speaker_resurrect_bh, ¶m); } +int +player_speaker_authorize(uint64_t id, const char *pin) +{ + struct speaker_attr_param param; + int ret; + + param.spk_id = id; + param.pin = pin; + + ret = commands_exec_sync(cmdbase, speaker_authorize, speaker_generic_bh, ¶m); + + return ret; +} + int player_volume_set(int vol) { diff --git a/src/player.h b/src/player.h index 45cf104c..d913c78b 100644 --- a/src/player.h +++ b/src/player.h @@ -114,6 +114,9 @@ player_speaker_busy_set(uint64_t id, bool busy); void player_speaker_resurrect(void *arg); +int +player_speaker_authorize(uint64_t id, const char *pin); + int player_playback_start(void); From d18e49f59bee7eb150bb4348bb3c50421dc21261 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Tue, 26 May 2020 22:46:49 +0200 Subject: [PATCH 08/14] [jsonapi] Make PUT /api/outputs/x capable of device verification --- README_JSON_API.md | 3 ++- src/httpd_jsonapi.c | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/README_JSON_API.md b/README_JSON_API.md index 98409405..633f02eb 100644 --- a/README_JSON_API.md +++ b/README_JSON_API.md @@ -317,7 +317,7 @@ curl -X PUT "http://localhost:3689/api/player/seek?seek_ms=-30000" | GET | [/api/outputs](#get-a-list-of-available-outputs) | Get a list of available outputs | | PUT | [/api/outputs/set](#set-enabled-outputs) | Set enabled outputs | | GET | [/api/outputs/{id}](#get-an-output) | Get an output | -| PUT | [/api/outputs/{id}](#change-an-output) | Change an output (enable/disable or volume) | +| PUT | [/api/outputs/{id}](#change-an-output) | Change an output setting | | PUT | [/api/outputs/{id}/toggle](#toggle-an-output) | Enable or disable an output, depending on the current state | @@ -482,6 +482,7 @@ PUT /api/outputs/{id} | --------------- | --------- | -------------------- | | selected | boolean | *(Optional)* `true` to enable and `false` to disable the output | | volume | integer | *(Optional)* Volume in percent (0 - 100) | +| pin | string | *(Optional)* PIN for device verification | **Response** diff --git a/src/httpd_jsonapi.c b/src/httpd_jsonapi.c index 993aeda7..5e731e52 100644 --- a/src/httpd_jsonapi.c +++ b/src/httpd_jsonapi.c @@ -1530,6 +1530,7 @@ jsonapi_reply_outputs_put_byid(struct httpd_request *hreq) json_object* request; bool selected; int volume; + const char *pin; int ret; ret = safe_atou64(hreq->uri_parsed->path_parts[2], &output_id); @@ -1566,6 +1567,13 @@ jsonapi_reply_outputs_put_byid(struct httpd_request *hreq) ret = player_volume_setabs_speaker(output_id, volume); } + if (ret == 0 && jparse_contains_key(request, "pin", json_type_string)) + { + pin = jparse_str_from_obj(request, "pin"); + if (pin) + ret = player_speaker_authorize(output_id, pin); + } + jparse_free(request); if (ret < 0) From 3cca7784192fd7190aa06301be04d3e5a6529f90 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Tue, 26 May 2020 23:20:29 +0200 Subject: [PATCH 09/14] [player] Stop using outputs_authorize() so it can be removed Use outputs_device_authorize() instead --- src/player.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/src/player.c b/src/player.c index 77e55468..4af39718 100644 --- a/src/player.c +++ b/src/player.c @@ -1434,8 +1434,23 @@ static enum command_state device_auth_kickoff(void *arg, int *retval) { union player_arg *cmdarg = arg; + struct output_device *device; - outputs_authorize(cmdarg->auth.type, cmdarg->auth.pin); + // First find the device requiring verification + for (device = output_device_list; device; device = device->next) + { + if (device->type == cmdarg->auth.type && device->state == OUTPUT_STATE_PASSWORD) + break; + } + + if (!device) + { + *retval = -1; + return COMMAND_END; + } + + // We're async, so we don't care about return values or callbacks with result + outputs_device_authorize(device, cmdarg->auth.pin, NULL); *retval = 0; return COMMAND_END; @@ -1565,10 +1580,10 @@ device_activate_cb(struct output_device *device, enum output_device_state status if (status == OUTPUT_STATE_PASSWORD) { - DPRINTF(E_LOG, L_PLAYER, "The %s device '%s' requires a valid password\n", device->type_name, device->name); + DPRINTF(E_LOG, L_PLAYER, "The %s device '%s' requires a valid PIN or password\n", device->type_name, device->name); - status = OUTPUT_STATE_FAILED; retval = -2; + goto out; } if (status == OUTPUT_STATE_FAILED) @@ -1580,7 +1595,8 @@ device_activate_cb(struct output_device *device, enum output_device_state status goto out; } - // If we were just probing this is a no-op + // If we were just probing or doing device verification this is a no-op, since + // there is no session any more outputs_device_cb_set(device, device_streaming_cb); out: @@ -3433,8 +3449,11 @@ player_device_remove(void *device) return ret; } -static void -player_device_auth_kickoff(enum output_types type, char **arglist) + +/* ----------------------- Thread: filescanner/httpd ------------------------ */ + +void +player_raop_verification_kickoff(char **arglist) { union player_arg *cmdarg; @@ -3445,19 +3464,11 @@ player_device_auth_kickoff(enum output_types type, char **arglist) return; } - cmdarg->auth.type = type; + cmdarg->auth.type = OUTPUT_TYPE_RAOP; memcpy(cmdarg->auth.pin, arglist[0], 4); commands_exec_async(cmdbase, device_auth_kickoff, cmdarg); -} - -/* --------------------------- Thread: filescanner -------------------------- */ - -void -player_raop_verification_kickoff(char **arglist) -{ - player_device_auth_kickoff(OUTPUT_TYPE_RAOP, arglist); } From d6ec6afb5ba6f94b2bd80abf80c764803c83644c Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Tue, 26 May 2020 23:21:06 +0200 Subject: [PATCH 10/14] [outputs] Remove outputs_authorize() Replaced by outputs_device_authorize() --- src/outputs.c | 10 ---------- src/outputs.h | 6 ------ src/outputs/raop.c | 21 --------------------- 3 files changed, 37 deletions(-) diff --git a/src/outputs.c b/src/outputs.c index bc3b6540..fb1dbb30 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -1160,16 +1160,6 @@ outputs_metadata_purge(void) } } -void -outputs_authorize(enum output_types type, const char *pin) -{ - if (outputs[type]->disabled) - return; - - if (outputs[type]->authorize) - outputs[type]->authorize(pin); -} - int outputs_priority(struct output_device *device) { diff --git a/src/outputs.h b/src/outputs.h index 6b55b8b9..cb1cbd48 100644 --- a/src/outputs.h +++ b/src/outputs.h @@ -238,9 +238,6 @@ struct output_definition // Write stream data to the output devices void (*write)(struct output_buffer *buffer); - // Authorize an output with a pin-code (probably coming from the filescanner) - void (*authorize)(const char *pin); - // Called from worker thread for async preparation of metadata (e.g. getting // artwork, which might involce downloading image data). The prepared data is // saved to metadata->data, which metadata_send() can use. @@ -358,9 +355,6 @@ outputs_metadata_send(uint32_t item_id, bool startup, output_metadata_finalize_c void outputs_metadata_purge(void); -void -outputs_authorize(enum output_types type, const char *pin); - int outputs_priority(struct output_device *device); diff --git a/src/outputs/raop.c b/src/outputs/raop.c index c04abcdf..210a6753 100644 --- a/src/outputs/raop.c +++ b/src/outputs/raop.c @@ -4382,26 +4382,6 @@ raop_verification_setup(struct raop_session *rs, const char *pin) return -1; } -static void -raop_authorize(const char *pin) -{ - struct raop_session *rs; - - for (rs = raop_sessions; rs; rs = rs->next) - { - if (rs->state == RAOP_STATE_UNVERIFIED) - break; - } - - if (!rs) - { - DPRINTF(E_LOG, L_RAOP, "Got a PIN for device verification, but no device is awaiting verification\n"); - return; - } - - raop_verification_setup(rs, pin); -} - static int raop_device_authorize(struct output_device *device, const char *pin, int callback_id) { @@ -5046,6 +5026,5 @@ struct output_definition output_raop = .metadata_purge = raop_metadata_purge, #ifdef RAOP_VERIFICATION .device_authorize = raop_device_authorize, - .authorize = raop_authorize, #endif }; From 138d4825106b744ece1581a7ee763d2a919fd221 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Wed, 27 May 2020 16:34:15 +0200 Subject: [PATCH 11/14] [dummy] Add capability to mock device verification for testing --- src/outputs/dummy.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/outputs/dummy.c b/src/outputs/dummy.c index 9de6e85a..45adf728 100644 --- a/src/outputs/dummy.c +++ b/src/outputs/dummy.c @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -39,6 +40,10 @@ #include "player.h" #include "outputs.h" +// If you set this to 1 you can mock device verification with the dummy output, +// use 1234 as PIN +#define DUMMY_DEVICE_VERIFICATION_TEST 0 + struct dummy_session { enum output_device_state state; @@ -97,7 +102,7 @@ dummy_status(struct dummy_session *ds) { outputs_cb(ds->callback_id, ds->device_id, ds->state); - if (ds->state == OUTPUT_STATE_STOPPED) + if (ds->state <= OUTPUT_STATE_STOPPED) dummy_session_cleanup(ds); } @@ -113,6 +118,10 @@ dummy_device_start(struct output_device *device, int callback_id) if (!ds) return -1; + // Mock a denied connection + if (device->requires_auth && !device->auth_key) + ds->state = OUTPUT_STATE_PASSWORD; + dummy_status(ds); return 1; @@ -156,6 +165,10 @@ dummy_device_probe(struct output_device *device, int callback_id) ds->callback_id = callback_id; ds->state = OUTPUT_STATE_STOPPED; + // Mock a denied connection + if (device->requires_auth && !device->auth_key) + ds->state = OUTPUT_STATE_PASSWORD; + dummy_status(ds); return 1; @@ -175,6 +188,32 @@ dummy_device_volume_set(struct output_device *device, int callback_id) return 1; } +static int +dummy_device_authorize(struct output_device *device, const char *pin, int callback_id) +{ + struct dummy_session *ds; + + ds = dummy_session_make(device, callback_id); + if (!ds) + return -1; + + if (strcmp(pin, "1234") == 0) + { + free(device->auth_key); + device->auth_key = strdup("test"); + ds->state = OUTPUT_STATE_STOPPED; + } + else + { + ds->state = OUTPUT_STATE_PASSWORD; + } + + ds->callback_id = callback_id; + dummy_status(ds); + + return 1; +} + static void dummy_device_cb_set(struct output_device *device, int callback_id) { @@ -205,6 +244,7 @@ dummy_init(void) device->type = OUTPUT_TYPE_DUMMY; device->type_name = outputs_name(device->type); device->has_video = 0; + device->requires_auth = DUMMY_DEVICE_VERIFICATION_TEST; DPRINTF(E_INFO, L_LAUDIO, "Adding dummy output device '%s'\n", nickname); @@ -232,5 +272,6 @@ struct output_definition output_dummy = .device_flush = dummy_device_flush, .device_probe = dummy_device_probe, .device_volume_set = dummy_device_volume_set, + .device_authorize = dummy_device_authorize, .device_cb_set = dummy_device_cb_set, }; From 384b1171d907b05e1ca373ae1be3c9361547ed35 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Wed, 27 May 2020 22:23:00 +0200 Subject: [PATCH 12/14] [raop] Change device verification so we don't risk stale sesssions Before, if a user never verified the device, we would have a device->session even though the device was not streaming and was in a failed state. This solution should be more clean and in line with the overall principle that we only have a session when communicating with the device. Also includes a bit of code refactoring. --- src/outputs/raop.c | 263 ++++++++++++++++++++------------------------- 1 file changed, 119 insertions(+), 144 deletions(-) diff --git a/src/outputs/raop.c b/src/outputs/raop.c index 210a6753..a7670bf0 100644 --- a/src/outputs/raop.c +++ b/src/outputs/raop.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012-2019 Espen Jürgensen + * Copyright (C) 2012-2020 Espen Jürgensen * Copyright (C) 2010-2011 Julien BLACHE * * RAOP AirTunes v2 @@ -152,10 +152,8 @@ enum raop_state { RAOP_STATE_TEARDOWN = RAOP_STATE_F_CONNECTED | 0x03, // Session is failed, couldn't startup or error occurred RAOP_STATE_FAILED = RAOP_STATE_F_FAILED | 0x01, - // Password issue: unknown password or bad password + // Password issue: unknown password or bad password, or pending PIN from user RAOP_STATE_PASSWORD = RAOP_STATE_F_FAILED | 0x02, - // Device requires verification, pending PIN from user - RAOP_STATE_UNVERIFIED= RAOP_STATE_F_FAILED | 0x03, }; // Info about the device, which is not required by the player, only internally @@ -1726,7 +1724,7 @@ raop_status(struct raop_session *rs) switch (rs->state) { - case RAOP_STATE_PASSWORD ... RAOP_STATE_UNVERIFIED: + case RAOP_STATE_PASSWORD: state = OUTPUT_STATE_PASSWORD; break; case RAOP_STATE_FAILED: @@ -1850,28 +1848,26 @@ session_free(struct raop_session *rs) if (!rs) return; - master_session_cleanup(rs->master_session); + if (rs->master_session) + master_session_cleanup(rs->master_session); - evrtsp_connection_set_closecb(rs->ctrl, NULL, NULL); + if (rs->ctrl) + { + evrtsp_connection_set_closecb(rs->ctrl, NULL, NULL); + evrtsp_connection_free(rs->ctrl); + } - evrtsp_connection_free(rs->ctrl); + if (rs->deferredev) + event_free(rs->deferredev); - event_free(rs->deferredev); + if (rs->server_fd >= 0) + close(rs->server_fd); - close(rs->server_fd); - - if (rs->realm) - free(rs->realm); - if (rs->nonce) - free(rs->nonce); - - if (rs->session) - free(rs->session); - - if (rs->address) - free(rs->address); - if (rs->devname) - free(rs->devname); + free(rs->realm); + free(rs->nonce); + free(rs->session); + free(rs->address); + free(rs->devname); free(rs); } @@ -1986,44 +1982,106 @@ deferredev_cb(int fd, short what, void *arg) } } -static struct raop_session * -session_make(struct output_device *rd, int family, int callback_id, bool only_probe) +static int +session_connection_setup(struct raop_session *rs, struct output_device *rd, int family) { - struct raop_session *rs; - struct raop_extra *re; char *address; char *intf; unsigned short port; int ret; - re = rd->extra_device_info; - + rs->sa.ss.ss_family = family; switch (family) { case AF_INET: /* We always have the v4 services, so no need to check */ if (!rd->v4_address) - return NULL; + return -1; address = rd->v4_address; port = rd->v4_port; + + rs->timing_svc = &timing_4svc; + rs->control_svc = &control_4svc; + + ret = inet_pton(AF_INET, address, &rs->sa.sin.sin_addr); break; case AF_INET6: if (!rd->v6_address || rd->v6_disabled || (timing_6svc.fd < 0) || (control_6svc.fd < 0)) - return NULL; + return -1; address = rd->v6_address; port = rd->v6_port; + + rs->timing_svc = &timing_6svc; + rs->control_svc = &control_6svc; + + intf = strchr(address, '%'); + if (intf) + *intf = '\0'; + + ret = inet_pton(AF_INET6, address, &rs->sa.sin6.sin6_addr); + + if (intf) + { + *intf = '%'; + + intf++; + + rs->sa.sin6.sin6_scope_id = if_nametoindex(intf); + if (rs->sa.sin6.sin6_scope_id == 0) + { + DPRINTF(E_LOG, L_RAOP, "Could not find interface %s\n", intf); + + ret = -1; + break; + } + } + break; default: - return NULL; + return -1; } + if (ret <= 0) + { + DPRINTF(E_LOG, L_RAOP, "Device '%s' has invalid address (%s) for %s\n", rd->name, address, (family == AF_INET) ? "ipv4" : "ipv6"); + return -1; + } + + rs->ctrl = evrtsp_connection_new(address, port); + if (!rs->ctrl) + { + DPRINTF(E_LOG, L_RAOP, "Could not create control connection to '%s' (%s)\n", rd->name, address); + return -1; + } + + evrtsp_connection_set_base(rs->ctrl, evbase_player); + + rs->address = strdup(address); + rs->family = family; + + return 0; +} + +static struct raop_session * +session_make(struct output_device *rd, int callback_id, bool only_probe) +{ + struct raop_session *rs; + struct raop_extra *re; + int ret; + + re = rd->extra_device_info; + + CHECK_NULL(L_RAOP, rs = calloc(1, sizeof(struct raop_session))); CHECK_NULL(L_RAOP, rs->deferredev = evtimer_new(evbase_player, deferredev_cb, rs)); + rs->devname = strdup(rd->name); + rs->volume = rd->volume; + rs->state = RAOP_STATE_STOPPED; rs->only_probe = only_probe; rs->reqs_in_flight = 0; @@ -2072,77 +2130,19 @@ session_make(struct output_device *rd, int family, int callback_id, bool only_pr break; } - rs->ctrl = evrtsp_connection_new(address, port); - if (!rs->ctrl) + ret = session_connection_setup(rs, rd, AF_INET6); + if (ret < 0) { - DPRINTF(E_LOG, L_RAOP, "Could not create control connection to '%s' (%s)\n", rd->name, address); - - goto out_free_event; + ret = session_connection_setup(rs, rd, AF_INET); + if (ret < 0) + goto error; } - evrtsp_connection_set_base(rs->ctrl, evbase_player); - - rs->sa.ss.ss_family = family; - switch (family) - { - case AF_INET: - rs->timing_svc = &timing_4svc; - rs->control_svc = &control_4svc; - - ret = inet_pton(AF_INET, address, &rs->sa.sin.sin_addr); - break; - - case AF_INET6: - rs->timing_svc = &timing_6svc; - rs->control_svc = &control_6svc; - - intf = strchr(address, '%'); - if (intf) - *intf = '\0'; - - ret = inet_pton(AF_INET6, address, &rs->sa.sin6.sin6_addr); - - if (intf) - { - *intf = '%'; - - intf++; - - rs->sa.sin6.sin6_scope_id = if_nametoindex(intf); - if (rs->sa.sin6.sin6_scope_id == 0) - { - DPRINTF(E_LOG, L_RAOP, "Could not find interface %s\n", intf); - - ret = -1; - break; - } - } - - break; - - default: - ret = -1; - break; - } - - if (ret <= 0) - { - DPRINTF(E_LOG, L_RAOP, "Device '%s' has invalid address (%s) for %s\n", rd->name, address, (family == AF_INET) ? "ipv4" : "ipv6"); - - goto out_free_evcon; - } - - rs->devname = strdup(rd->name); - rs->address = strdup(address); - rs->family = family; - - rs->volume = rd->volume; - rs->master_session = master_session_make(&rd->quality, rs->encrypt); if (!rs->master_session) { DPRINTF(E_LOG, L_RAOP, "Could not attach a master session for device '%s'\n", rd->name); - goto out_free_evcon; + goto error; } // Attach to list of sessions @@ -2154,11 +2154,8 @@ session_make(struct output_device *rd, int family, int callback_id, bool only_pr return rs; - out_free_evcon: - evrtsp_connection_free(rs->ctrl); - out_free_event: - event_free(rs->deferredev); - free(rs); + error: + session_free(rs); return NULL; } @@ -3553,13 +3550,7 @@ raop_cb_pin_start(struct evrtsp_request *req, void *arg) if (ret < 0) goto error; - rs->state = RAOP_STATE_UNVERIFIED; - - raop_status(rs); - - // TODO If the user never verifies the session will remain stale - - return; + rs->state = RAOP_STATE_PASSWORD; error: session_failure(rs); @@ -4202,8 +4193,8 @@ raop_cb_verification_verify_step2(struct evrtsp_request *req, void *arg) return; error: - rs->state = RAOP_STATE_UNVERIFIED; - raop_status(rs); + rs->state = RAOP_STATE_PASSWORD; + session_failure(rs); } static void @@ -4233,11 +4224,11 @@ raop_cb_verification_verify_step1(struct evrtsp_request *req, void *arg) return; error: - rs->state = RAOP_STATE_UNVERIFIED; - raop_status(rs); - verification_verify_free(rs->verification_verify_ctx); rs->verification_verify_ctx = NULL; + + rs->state = RAOP_STATE_PASSWORD; + session_failure(rs); } static int @@ -4259,9 +4250,6 @@ raop_verification_verify(struct raop_session *rs) return 0; error: - rs->state = RAOP_STATE_UNVERIFIED; - raop_status(rs); - verification_verify_free(rs->verification_verify_ctx); rs->verification_verify_ctx = NULL; return -1; @@ -4299,7 +4287,7 @@ raop_cb_verification_setup_step3(struct evrtsp_request *req, void *arg) // A blocking db call... :-~ db_speaker_save(device); - // No longer UNVERIFIED + // No longer RAOP_STATE_PASSWORD rs->state = RAOP_STATE_STOPPED; out: @@ -4333,7 +4321,7 @@ raop_cb_verification_setup_step2(struct evrtsp_request *req, void *arg) error: verification_setup_free(rs->verification_setup_ctx); rs->verification_setup_ctx = NULL; - raop_status(rs); + session_failure(rs); } static void @@ -4355,7 +4343,7 @@ raop_cb_verification_setup_step1(struct evrtsp_request *req, void *arg) error: verification_setup_free(rs->verification_setup_ctx); rs->verification_setup_ctx = NULL; - raop_status(rs); + session_failure(rs); } static int @@ -4374,6 +4362,8 @@ raop_verification_setup(struct raop_session *rs, const char *pin) if (ret < 0) goto error; + rs->state = RAOP_STATE_PASSWORD; + return 0; error: @@ -4385,17 +4375,21 @@ raop_verification_setup(struct raop_session *rs, const char *pin) static int raop_device_authorize(struct output_device *device, const char *pin, int callback_id) { - struct raop_session *rs = device->session; + struct raop_session *rs; int ret; + // Make a session so we can communicate with the device + rs = session_make(device, callback_id, true); if (!rs) return -1; ret = raop_verification_setup(rs, pin); if (ret < 0) - return -1; - - rs->callback_id = callback_id; + { + DPRINTF(E_LOG, L_RAOP, "Could not send verification setup request to '%s' (address %s)\n", device->name, rs->address); + session_cleanup(rs); + return -1; + } return 1; } @@ -4706,26 +4700,7 @@ raop_device_start_generic(struct output_device *device, int callback_id, bool on * address and build our session URL for all subsequent requests. */ - rs = session_make(device, AF_INET6, callback_id, only_probe); - if (rs) - { - if (device->auth_key) - ret = raop_verification_verify(rs); - else if (device->requires_auth) - ret = raop_send_req_pin_start(rs, raop_cb_pin_start, "device_start"); - else - ret = raop_send_req_options(rs, raop_cb_startup_options, "device_start"); - - if (ret == 0) - return 1; - else - { - DPRINTF(E_WARN, L_RAOP, "Could not send verification or OPTIONS request on IPv6\n"); - session_cleanup(rs); - } - } - - rs = session_make(device, AF_INET, callback_id, only_probe); + rs = session_make(device, callback_id, only_probe); if (!rs) return -1; @@ -4738,7 +4713,7 @@ raop_device_start_generic(struct output_device *device, int callback_id, bool on if (ret < 0) { - DPRINTF(E_WARN, L_RAOP, "Could not send verification or OPTIONS request on IPv4\n"); + DPRINTF(E_WARN, L_RAOP, "Could not send verification or OPTIONS request to '%s' (address %s)\n", device->name, rs->address); session_cleanup(rs); return -1; } From a9a6f4b58411dadbb02ec75e0442f04f222f3c3a Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Wed, 27 May 2020 22:25:48 +0200 Subject: [PATCH 13/14] [outputs] Check for session in outputs_device_authorize() --- src/outputs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/outputs.c b/src/outputs.c index fb1dbb30..f5e7f2c8 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -950,6 +950,9 @@ outputs_device_authorize(struct output_device *device, const char *pin, output_s if (outputs[device->type]->disabled || !outputs[device->type]->device_authorize) return -1; + if (device->session) + return 0; // We are already connected to the device - no auth required + ret = outputs[device->type]->device_authorize(device, pin, callback_add(device, cb)); return device_state_update(device, ret); From 5d22f11b0e1cf9b5da6bd12393847a9685dc4326 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Wed, 27 May 2020 22:53:11 +0200 Subject: [PATCH 14/14] [player/outputs] Fix for speaker selection and volume adj (closes #1011) * Make sure that vol_adjust() bases adjustments on the same speakers that the user sees as active * If a speaker fails during activation we unselect it, so that we don't keep trying to start it, so we find a new master volume if required, and so it is possible for the user to reselect it. --- src/outputs.c | 6 +++--- src/outputs.h | 6 ++++++ src/player.c | 10 +++++----- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/outputs.c b/src/outputs.c index f5e7f2c8..8ea05e18 100644 --- a/src/outputs.c +++ b/src/outputs.c @@ -515,7 +515,7 @@ vol_adjust(void) for (device = output_device_list; device; device = device->next) { - if (device->selected && (device->volume > selected_highest)) + if (OUTPUTS_DEVICE_DISPLAY_SELECTED(device) && (device->volume > selected_highest)) selected_highest = device->volume; if (device->volume > all_highest) @@ -526,7 +526,7 @@ vol_adjust(void) for (device = output_device_list; device; device = device->next) { - if (!device->selected && (device->volume > outputs_master_volume)) + if (!OUTPUTS_DEVICE_DISPLAY_SELECTED(device) && (device->volume > outputs_master_volume)) device->volume = outputs_master_volume; device->relvol = vol_to_rel(device->volume, outputs_master_volume); @@ -537,7 +537,7 @@ vol_adjust(void) for (device = output_device_list; device; device = device->next) { - DPRINTF(E_DBG, L_PLAYER, "*** %s: abs %d rel %d selected %d\n", device->name, device->volume, device->relvol, device->selected); + DPRINTF(E_DBG, L_PLAYER, "*** %s: abs %d rel %d selected %d\n", device->name, device->volume, device->relvol, OUTPUTS_DEVICE_DISPLAY_SELECTED(device)); } #endif } diff --git a/src/outputs.h b/src/outputs.h index cb1cbd48..db9ad0d0 100644 --- a/src/outputs.h +++ b/src/outputs.h @@ -41,6 +41,12 @@ // different values can only do so within a limited range (maybe max 3 secs) #define OUTPUTS_BUFFER_DURATION 2 +// Whether the device should be *displayed* as selected is not given by +// device->selected, since that means "has the user selected the device", +// without taking into account whether it is working or available. This macro +// is a compound of the factors that determine how to display speaker selection. +#define OUTPUTS_DEVICE_DISPLAY_SELECTED(device) ((device)->selected && (device)->state >= OUTPUT_STATE_STOPPED && !(device)->busy && !(device)->prevent_playback) + // Forward declarations struct output_device; struct output_metadata; diff --git a/src/player.c b/src/player.c index 4af39718..770cf294 100644 --- a/src/player.c +++ b/src/player.c @@ -1582,6 +1582,8 @@ device_activate_cb(struct output_device *device, enum output_device_state status { DPRINTF(E_LOG, L_PLAYER, "The %s device '%s' requires a valid PIN or password\n", device->type_name, device->name); + outputs_device_deselect(device); + retval = -2; goto out; } @@ -1590,6 +1592,8 @@ device_activate_cb(struct output_device *device, enum output_device_state status { DPRINTF(E_LOG, L_PLAYER, "The %s device '%s' failed to activate\n", device->type_name, device->name); + outputs_device_deselect(device); + if (retval != -2) retval = -1; goto out; @@ -2453,11 +2457,7 @@ device_to_speaker_info(struct player_speaker_info *spk, struct output_device *de spk->relvol = device->relvol; spk->absvol = device->volume; - // We can't map device->selected directly to spk->selected, since the former - // means "has the user selected the device" (even if it isn't working or is - // unavailable), while clients that get the latter just want to know if the - // speaker plays or will be playing. - spk->selected = (device->selected && device->state >= OUTPUT_STATE_STOPPED && !device->busy && !device->prevent_playback); + spk->selected = OUTPUTS_DEVICE_DISPLAY_SELECTED(device); spk->has_password = device->has_password; spk->has_video = device->has_video;