Skip to content

Commit

Permalink
Cherry pick PR #1559: [media] Refine HandlerResult use (#1628)
Browse files Browse the repository at this point in the history 
Refer to the original PR: #1559

Reduces code duplication and errors surround the use of
PlayerWorker::Handler::HandlerResult.

b/291788496

Co-authored-by: Austin Osagie <[email protected]>
  • Loading branch information
@cobalt-github-releaser-bot @osagie98
cobalt-github-releaser-bot and osagie98 authored Sep 25, 2023
1 parent 72f63a3 commit 320aafb
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 71 deletions.
48 changes: 24 additions & 24 deletions starboard/shared/starboard/player/filter/filter_based_player_worker_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::Init(
std::string error_message =
FormatString("Required channel %d is greater than maximum channel %d",
required_audio_channels, supported_audio_channels);
return {false, std::move(error_message.c_str())};
return HandlerResult{false, error_message};
}
}

Expand All @@ -149,7 +149,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::Init(
std::string error_message =
FormatString("Failed to create player components with error: %s.",
components_error_message.c_str());
return {false, std::move(error_message.c_str())};
return HandlerResult{false, error_message};
}
media_time_provider_ = player_components_->GetMediaTimeProvider();
audio_renderer_ = player_components_->GetAudioRenderer();
Expand Down Expand Up @@ -189,7 +189,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::Init(

update_job_token_ = Schedule(update_job_, kUpdateInterval);

return {true};
return HandlerResult{true};
}

HandlerResult FilterBasedPlayerWorkerHandler::Seek(SbTime seek_to_time,
Expand All @@ -199,7 +199,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::Seek(SbTime seek_to_time,
SB_LOG(INFO) << "Seek to " << seek_to_time << ", and media time provider is "
<< media_time_provider_;
if (!media_time_provider_) {
return {false, "Invalid media time provider"};
return HandlerResult{false, "Invalid media time provider"};
}

if (seek_to_time < 0) {
Expand All @@ -214,7 +214,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::Seek(SbTime seek_to_time,
media_time_provider_->Seek(seek_to_time);
audio_prerolled_ = false;
video_prerolled_ = false;
return {true};
return HandlerResult{true};
}

HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
Expand All @@ -230,19 +230,19 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
*samples_written = 0;
if (input_buffers.front()->sample_type() == kSbMediaTypeAudio) {
if (!audio_renderer_) {
return {false, "Invalid audio renderer."};
return HandlerResult{false, "Invalid audio renderer."};
}

if (audio_renderer_->IsEndOfStreamWritten()) {
SB_LOG(WARNING) << "Try to write audio sample after EOS is reached";
} else {
if (!audio_renderer_->CanAcceptMoreData()) {
return {true};
return HandlerResult{true};
}
for (const auto& input_buffer : input_buffers) {
if (input_buffer->drm_info()) {
if (!SbDrmSystemIsValid(drm_system_)) {
return {false, "Invalid DRM system."};
return HandlerResult{false, "Invalid DRM system."};
}
DumpInputHash(input_buffer);
SbDrmSystemPrivate::DecryptStatus decrypt_status =
Expand All @@ -253,10 +253,10 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
InputBuffers(input_buffers.begin(),
input_buffers.begin() + *samples_written));
}
return {true};
return HandlerResult{true};
}
if (decrypt_status == SbDrmSystemPrivate::kFailure) {
return {false, "Sample decryption failure."};
return HandlerResult{false, "Sample decryption failure."};
}
}
DumpInputHash(input_buffer);
Expand All @@ -268,19 +268,19 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
SB_DCHECK(input_buffers.front()->sample_type() == kSbMediaTypeVideo);

if (!video_renderer_) {
return {false, "Invalid video renderer."};
return HandlerResult{false, "Invalid video renderer."};
}

if (video_renderer_->IsEndOfStreamWritten()) {
SB_LOG(WARNING) << "Try to write video sample after EOS is reached";
} else {
if (!video_renderer_->CanAcceptMoreData()) {
return {true};
return HandlerResult{true};
}
for (const auto& input_buffer : input_buffers) {
if (input_buffer->drm_info()) {
if (!SbDrmSystemIsValid(drm_system_)) {
return {false, "Invalid DRM system."};
return HandlerResult{false, "Invalid DRM system."};
}
DumpInputHash(input_buffer);
SbDrmSystemPrivate::DecryptStatus decrypt_status =
Expand All @@ -291,10 +291,10 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
InputBuffers(input_buffers.begin(),
input_buffers.begin() + *samples_written));
}
return {true};
return HandlerResult{true};
}
if (decrypt_status == SbDrmSystemPrivate::kFailure) {
return {false, "Sample decryption failure."};
return HandlerResult{false, "Sample decryption failure."};
}
}
DumpInputHash(input_buffer);
Expand All @@ -304,7 +304,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteSamples(
}
}

return {true};
return HandlerResult{true};
}

HandlerResult FilterBasedPlayerWorkerHandler::WriteEndOfStream(
Expand All @@ -314,7 +314,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteEndOfStream(
if (sample_type == kSbMediaTypeAudio) {
if (!audio_renderer_) {
SB_LOG(INFO) << "Audio EOS enqueued when renderer is NULL.";
return {false, "Audio EOS enqueued when renderer is NULL."};
return HandlerResult{false, "Audio EOS enqueued when renderer is NULL."};
}
if (audio_renderer_->IsEndOfStreamWritten()) {
SB_LOG(WARNING) << "Try to write audio EOS after EOS is enqueued";
Expand All @@ -325,7 +325,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteEndOfStream(
} else {
if (!video_renderer_) {
SB_LOG(INFO) << "Video EOS enqueued when renderer is NULL.";
return {false, "Video EOS enqueued when renderer is NULL."};
return HandlerResult{false, "Video EOS enqueued when renderer is NULL."};
}
if (video_renderer_->IsEndOfStreamWritten()) {
SB_LOG(WARNING) << "Try to write video EOS after EOS is enqueued";
Expand All @@ -335,7 +335,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::WriteEndOfStream(
}
}

return {true};
return HandlerResult{true};
}

HandlerResult FilterBasedPlayerWorkerHandler::SetPause(bool pause) {
Expand All @@ -345,7 +345,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::SetPause(bool pause) {
<< ", and media time provider is " << media_time_provider_;

if (!media_time_provider_) {
return {false, "Invalid media time provider."};
return HandlerResult{false, "Invalid media time provider."};
}

paused_ = pause;
Expand All @@ -356,7 +356,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::SetPause(bool pause) {
media_time_provider_->Play();
}
Update();
return {true};
return HandlerResult{true};
}

HandlerResult FilterBasedPlayerWorkerHandler::SetPlaybackRate(
Expand All @@ -370,12 +370,12 @@ HandlerResult FilterBasedPlayerWorkerHandler::SetPlaybackRate(
playback_rate_ = playback_rate;

if (!media_time_provider_) {
return {false, "Invalid media time provider."};
return HandlerResult{false, "Invalid media time provider."};
}

media_time_provider_->SetPlaybackRate(playback_rate_);
Update();
return {true};
return HandlerResult{true};
}

void FilterBasedPlayerWorkerHandler::SetVolume(double volume) {
Expand Down Expand Up @@ -413,7 +413,7 @@ HandlerResult FilterBasedPlayerWorkerHandler::SetBounds(const Bounds& bounds) {
}
}

return {true};
return HandlerResult{true};
}

void FilterBasedPlayerWorkerHandler::OnError(SbPlayerError error,
Expand Down
60 changes: 21 additions & 39 deletions starboard/shared/starboard/player/player_worker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,17 +161,24 @@ void PlayerWorker::UpdatePlayerState(SbPlayerState player_state) {
}

void PlayerWorker::UpdatePlayerError(SbPlayerError error,
HandlerResult result,
const std::string& error_message) {
SB_DCHECK(!result.success);
std::string complete_error_message = error_message;
if (!result.error_message.empty()) {
complete_error_message += " Error: " + result.error_message;
}

SB_LOG(WARNING) << "Encountered player error " << error
<< " with message: " << error_message;
<< " with message: " << complete_error_message;
// Only report the first error.
if (error_occurred_.exchange(true)) {
return;
}
if (!player_error_func_) {
return;
}
player_error_func_(player_, context_, error, error_message.c_str());
player_error_func_(player_, context_, error, complete_error_message.c_str());
}

// static
Expand Down Expand Up @@ -200,8 +207,8 @@ void PlayerWorker::DoInit() {
SB_DCHECK(job_queue_->BelongsToCurrentThread());

Handler::UpdatePlayerErrorCB update_player_error_cb;
update_player_error_cb =
std::bind(&PlayerWorker::UpdatePlayerError, this, _1, _2);
update_player_error_cb = std::bind(&PlayerWorker::UpdatePlayerError, this, _1,
HandlerResult{false}, _2);
HandlerResult result = handler_->Init(
player_, std::bind(&PlayerWorker::UpdateMediaInfo, this, _1, _2, _3),
std::bind(&PlayerWorker::player_state, this),
Expand All @@ -210,11 +217,8 @@ void PlayerWorker::DoInit() {
if (result.success) {
UpdatePlayerState(kSbPlayerStateInitialized);
} else {
std::string error_message = "Failed to initialize PlayerWorker.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, error_message);
UpdatePlayerError(kSbPlayerErrorDecode, result,
"Failed to initialize PlayerWorker.");
}
}

Expand All @@ -241,11 +245,7 @@ void PlayerWorker::DoSeek(SbTime seek_to_time, int ticket) {

HandlerResult result = handler_->Seek(seek_to_time, ticket);
if (!result.success) {
std::string error_message = "Failed seek.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, error_message);
UpdatePlayerError(kSbPlayerErrorDecode, result, "Failed seek.");
return;
}

Expand Down Expand Up @@ -288,11 +288,7 @@ void PlayerWorker::DoWriteSamples(InputBuffers input_buffers) {
HandlerResult result =
handler_->WriteSamples(input_buffers, &samples_written);
if (!result.success) {
std::string error_message = "Failed to write sample.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, error_message);
UpdatePlayerError(kSbPlayerErrorDecode, result, "Failed to write sample.");
return;
}
if (samples_written == input_buffers.size()) {
Expand Down Expand Up @@ -360,23 +356,16 @@ void PlayerWorker::DoWriteEndOfStream(SbMediaType sample_type) {

HandlerResult result = handler_->WriteEndOfStream(sample_type);
if (!result.success) {
std::string error_message = "Failed to write end of stream.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, error_message);
UpdatePlayerError(kSbPlayerErrorDecode, result,
"Failed to write end of stream.");
}
}

void PlayerWorker::DoSetBounds(Bounds bounds) {
SB_DCHECK(job_queue_->BelongsToCurrentThread());
HandlerResult result = handler_->SetBounds(bounds);
if (!result.success) {
std::string error_message = "Failed to set bounds";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, "Failed to set bounds");
UpdatePlayerError(kSbPlayerErrorDecode, result, "Failed to set bounds.");
}
}

Expand All @@ -385,11 +374,7 @@ void PlayerWorker::DoSetPause(bool pause) {

HandlerResult result = handler_->SetPause(pause);
if (!result.success) {
std::string error_message = "Failed to set pause.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, error_message);
UpdatePlayerError(kSbPlayerErrorDecode, result, "Failed to set pause.");
}
}

Expand All @@ -398,11 +383,8 @@ void PlayerWorker::DoSetPlaybackRate(double playback_rate) {

HandlerResult result = handler_->SetPlaybackRate(playback_rate);
if (!result.success) {
std::string error_message = "Failed to set playback rate.";
if (!result.error_message.empty()) {
error_message += " Error: " + result.error_message;
}
UpdatePlayerError(kSbPlayerErrorDecode, "Failed to set playback rate.");
UpdatePlayerError(kSbPlayerErrorDecode, result,
"Failed to set playback rate.");
}
}

Expand Down
18 changes: 10 additions & 8 deletions starboard/shared/starboard/player/player_worker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ class PlayerWorker {
// All functions of this class will be called from the JobQueue thread.
class Handler {
public:
// Stores the success status of Handler operations. If |success| is false,
// |error_message| may be set with details of the error.
struct HandlerResult {
bool success;
std::string error_message;
};

typedef PlayerWorker::Bounds Bounds;
typedef ::starboard::shared::starboard::player::InputBuffer InputBuffer;
typedef ::starboard::shared::starboard::player::InputBuffers InputBuffers;
Expand All @@ -76,13 +83,6 @@ class PlayerWorker {
const std::string& error_message)>
UpdatePlayerErrorCB;

// Stores the success status of Handler operations. If |success| is false,
// |error_message| may be set with details of the error.
typedef struct HandlerResult {
bool success;
std::string error_message;
} HandlerResult;

Handler() = default;
virtual ~Handler() {}

Expand Down Expand Up @@ -195,7 +195,9 @@ class PlayerWorker {

SbPlayerState player_state() const { return player_state_; }
void UpdatePlayerState(SbPlayerState player_state);
void UpdatePlayerError(SbPlayerError error, const std::string& message);
void UpdatePlayerError(SbPlayerError error,
Handler::HandlerResult result,
const std::string& message);

static void* ThreadEntryPoint(void* context);
void RunLoop();
Expand Down

0 comments on commit 320aafb

Please sign in to comment.