From 06f7619f7a2201d0081fd0fe923b91a5f9f3d65e Mon Sep 17 00:00:00 2001 From: Szymon Jastrzebski Date: Thu, 21 Dec 2017 07:32:11 +0100 Subject: [PATCH] [MediaController] Fix for bug onChange events In server side, all onchange*request events should be fired only after requesting server state change from client side. Clients should receive on*changed events only after server state change. [Verification] TCT passed 100% Change-Id: I85e5eda12acf6de74bc653a9ccc8a283862eeb16 Signed-off-by: Szymon Jastrzebski --- src/mediacontroller/mediacontroller_client.cc | 31 ++------ src/mediacontroller/mediacontroller_server.cc | 70 ++++++++++++++----- src/mediacontroller/mediacontroller_server.h | 5 ++ 3 files changed, 63 insertions(+), 43 deletions(-) diff --git a/src/mediacontroller/mediacontroller_client.cc b/src/mediacontroller/mediacontroller_client.cc index 950ab6f8..43e3c896 100644 --- a/src/mediacontroller/mediacontroller_client.cc +++ b/src/mediacontroller/mediacontroller_client.cc @@ -278,11 +278,6 @@ void MediaControllerClient::OnServerStatusUpdate(const char* server_name, mc_ser ScopeLogger(); MediaControllerClient* client = static_cast(user_data); - if (!client->server_status_listener_) { - LoggerD("No server status listener registered, skipping"); - return; - } - // server state std::string state_str; PlatformResult result = Types::PlatformEnumToString(Types::kMediaControllerServerState, @@ -305,10 +300,16 @@ PlatformResult MediaControllerClient::SetPlaybackInfoListener(const JsonCallback ScopeLogger(); int failed_setter = 0; SCOPE_EXIT { + // Lambda function used to clean all set listeners (in case of failure of + // SetPlaybackInfoListener method). + // The purpose of this lambda is to unset as many setters as we can in case of failure. + int (*unsetters[])(mc_client_h) = { mc_client_unset_playback_update_cb, mc_client_unset_shuffle_mode_update_cb, mc_client_unset_repeat_mode_update_cb, /*mc_client_unset_metadata_update_cb the last unsetter will never be used*/}; + + // This loop is no-op in case of success. for (int i = 0; i < failed_setter; ++i) { auto ret = unsetters[i](handle_); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { @@ -374,11 +375,6 @@ void MediaControllerClient::OnPlaybackUpdate(const char* server_name, mc_playbac ScopeLogger(); MediaControllerClient* client = static_cast(user_data); - if (!client->playback_info_listener_) { - LoggerD("No playback info listener registered, skipping"); - return; - } - // playback state std::string state; PlatformResult result = Types::ConvertPlaybackState(playback, &state); @@ -411,11 +407,6 @@ void MediaControllerClient::OnShuffleModeUpdate(const char* server_name, mc_shuf ScopeLogger(); MediaControllerClient* client = static_cast(user_data); - if (!client->playback_info_listener_) { - LoggerD("No playback info listener registered, skipping"); - return; - } - picojson::value data = picojson::value(picojson::object()); picojson::object& data_o = data.get(); @@ -431,11 +422,6 @@ void MediaControllerClient::OnRepeatModeUpdate(const char* server_name, mc_repea ScopeLogger(); MediaControllerClient* client = static_cast(user_data); - if (!client->playback_info_listener_) { - LoggerD("No playback info listener registered, skipping"); - return; - } - picojson::value data = picojson::value(picojson::object()); picojson::object& data_o = data.get(); @@ -451,11 +437,6 @@ void MediaControllerClient::OnMetadataUpdate(const char* server_name, mc_metadat ScopeLogger(); MediaControllerClient* client = static_cast(user_data); - if (!client->playback_info_listener_) { - LoggerD("No playback info listener registered, skipping"); - return; - } - picojson::value data = picojson::value(picojson::object()); picojson::object& data_o = data.get(); diff --git a/src/mediacontroller/mediacontroller_server.cc b/src/mediacontroller/mediacontroller_server.cc index 76aab28c..dd6449a2 100644 --- a/src/mediacontroller/mediacontroller_server.cc +++ b/src/mediacontroller/mediacontroller_server.cc @@ -27,17 +27,15 @@ namespace extension { namespace mediacontroller { -namespace { -// The privileges that are required in Application API -const std::string kInternalCommandSendPlaybackPosition = "__internal_sendPlaybackPosition"; -const std::string kInternalCommandSendShuffleMode = "__internal_sendShuffleMode"; -const std::string kInternalCommandSendRepeatMode = "__internal_sendRepeatMode"; -} // namespace - using common::PlatformResult; using common::ErrorCode; -MediaControllerServer::MediaControllerServer() : handle_(nullptr) { +MediaControllerServer::MediaControllerServer() + : handle_(nullptr), + playback_state_(MC_PLAYBACK_STATE_STOPPED), + position_(0ULL), + shuffle_mode_(MC_SHUFFLE_MODE_OFF), + repeat_mode_(MC_REPEAT_MODE_OFF) { ScopeLogger(); } @@ -82,6 +80,11 @@ PlatformResult MediaControllerServer::SetPlaybackState(const std::string& state) return result; } + if (static_cast(state_int) == playback_state_) { + LoggerD("No change in playback state requested, skipping"); + return PlatformResult(ErrorCode::NO_ERROR); + } + int ret = mc_server_set_playback_state(handle_, static_cast(state_int)); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( @@ -89,6 +92,8 @@ PlatformResult MediaControllerServer::SetPlaybackState(const std::string& state) ("mc_server_set_playback_state() error: %d, message: %s", ret, get_error_message(ret))); } + playback_state_ = static_cast(state_int); + ret = mc_server_update_playback_info(handle_); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( @@ -102,6 +107,11 @@ PlatformResult MediaControllerServer::SetPlaybackState(const std::string& state) PlatformResult MediaControllerServer::SetPlaybackPosition(double position) { ScopeLogger(); + if (static_cast(position) == position_) { + LoggerD("No change in playback position requested, skipping"); + return PlatformResult(ErrorCode::NO_ERROR); + } + int ret = mc_server_set_playback_position(handle_, static_cast(position)); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( @@ -109,6 +119,8 @@ PlatformResult MediaControllerServer::SetPlaybackPosition(double position) { ("mc_server_set_playback_position() error: %d, message: %s", ret, get_error_message(ret))); } + position_ = static_cast(position); + ret = mc_server_update_playback_info(handle_); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( @@ -122,26 +134,42 @@ PlatformResult MediaControllerServer::SetPlaybackPosition(double position) { PlatformResult MediaControllerServer::SetShuffleMode(bool mode) { ScopeLogger(); - int ret = mc_server_update_shuffle_mode(handle_, mode ? MC_SHUFFLE_MODE_ON : MC_SHUFFLE_MODE_OFF); + mc_shuffle_mode_e shuffle_mode = mode ? MC_SHUFFLE_MODE_ON : MC_SHUFFLE_MODE_OFF; + if (shuffle_mode == shuffle_mode_) { + LoggerD("No change in shuffle mode requested, skipping"); + return PlatformResult(ErrorCode::NO_ERROR); + } + + int ret = mc_server_update_shuffle_mode(handle_, shuffle_mode); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( ErrorCode::UNKNOWN_ERR, "Error updating shuffle mode", ("mc_server_update_shuffle_mode() error: %d, message: %s", ret, get_error_message(ret))); } + shuffle_mode_ = shuffle_mode; + return PlatformResult(ErrorCode::NO_ERROR); } PlatformResult MediaControllerServer::SetRepeatMode(bool mode) { ScopeLogger(); - int ret = mc_server_update_repeat_mode(handle_, mode ? MC_REPEAT_MODE_ON : MC_REPEAT_MODE_OFF); + mc_repeat_mode_e repeat_mode = mode ? MC_REPEAT_MODE_ON : MC_REPEAT_MODE_OFF; + if (repeat_mode == repeat_mode_) { + LoggerD("No change in repeat mode requested, skipping"); + return PlatformResult(ErrorCode::NO_ERROR); + } + + int ret = mc_server_update_repeat_mode(handle_, repeat_mode); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { return LogAndCreateResult( ErrorCode::UNKNOWN_ERR, "Error updating repeat mode", ("mc_server_update_repeat_mode() error: %d, message: %s", ret, get_error_message(ret))); } + repeat_mode_ = repeat_mode; + return PlatformResult(ErrorCode::NO_ERROR); } @@ -289,11 +317,17 @@ PlatformResult MediaControllerServer::SetChangeRequestPlaybackInfoListener( ScopeLogger(); int failed_setter = 0; SCOPE_EXIT { + // Lambda function used to clean all set listeners (in case of failure of + // SetPlaybackInfoListener method). + // The purpose of this lambda is to unset as many setters as we can in case of failure. + int (*unsetters[])(mc_server_h) = { mc_server_unset_playback_state_command_received_cb, mc_server_unset_playback_position_command_received_cb, mc_server_unset_shuffle_mode_command_received_cb, /*mc_server_unset_repeat_mode_command_received_cb the last unsetter will never be used*/}; + + // This loop is no-op in case of success. for (int i = 0; i < failed_setter; ++i) { auto ret = unsetters[i](handle_); if (MEDIA_CONTROLLER_ERROR_NONE != ret) { @@ -389,8 +423,8 @@ void MediaControllerServer::OnPlaybackStateCommand(const char* client_name, MediaControllerServer* server = static_cast(user_data); - if (!server->change_request_playback_info_listener_) { - LoggerD("No change request playback info listener registered, skipping"); + if (server->playback_state_ == state_e) { + LoggerD("The media playback state did not change, skipping"); return; } @@ -418,8 +452,8 @@ void MediaControllerServer::OnPlaybackPositionCommand(const char* client_name, MediaControllerServer* server = static_cast(user_data); - if (!server->change_request_playback_info_listener_) { - LoggerD("No change request playback info listener registered, skipping"); + if (server->position_ == position) { + LoggerD("The position did not change, skipping"); return; } @@ -438,8 +472,8 @@ void MediaControllerServer::OnShuffleModeCommand(const char* client_name, mc_shu MediaControllerServer* server = static_cast(user_data); - if (!server->change_request_playback_info_listener_) { - LoggerD("No change request playback info listener registered, skipping"); + if (server->shuffle_mode_ == mode) { + LoggerD("The shuffle mode did not change, skipping"); return; } @@ -458,8 +492,8 @@ void MediaControllerServer::OnRepeatModeCommand(const char* client_name, mc_repe MediaControllerServer* server = static_cast(user_data); - if (!server->change_request_playback_info_listener_) { - LoggerD("No change request playback info listener registered, skipping"); + if (server->repeat_mode_ == mode) { + LoggerD("The repeat mode did not change, skipping"); return; } diff --git a/src/mediacontroller/mediacontroller_server.h b/src/mediacontroller/mediacontroller_server.h index 31105602..679bb4bf 100644 --- a/src/mediacontroller/mediacontroller_server.h +++ b/src/mediacontroller/mediacontroller_server.h @@ -51,6 +51,11 @@ class MediaControllerServer { mc_server_h handle_; JsonCallback change_request_playback_info_listener_; + mc_playback_states_e playback_state_; + unsigned long long position_; + mc_shuffle_mode_e shuffle_mode_; + mc_repeat_mode_e repeat_mode_; + JsonCallback command_listener_; static void OnPlaybackStateCommand(const char* client_name, mc_playback_states_e state_e, -- 2.34.1