Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[media] Create SbPlayerState utility function #4668

Merged
merged 3 commits into from
Jan 9, 2025
Merged

[media] Create SbPlayerState utility function #4668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8546f3e
[media] Create SbPlayerState utility function
osagie98 Jan 8, 2025
60035c3
Remove lambda function
osagie98 Jan 8, 2025
d09b820
formatting
osagie98 Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions media/starboard/starboard_renderer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
#include "media/base/audio_codecs.h"
#include "media/base/video_codecs.h"
#include "starboard/common/media.h"
#include "starboard/common/player.h"

namespace media {

namespace {

using ::starboard::GetMediaAudioConnectorName;
using ::starboard::GetPlayerStateName;

// In the OnNeedData(), it attempts to write one more audio access
// unit than the audio write duration. Specifically, the check
Expand Down Expand Up @@ -765,16 +767,15 @@ void StarboardRenderer::OnPlayerStatus(SbPlayerState state) {
// In case if state is changed when creation of the `player_bridge_` fails.
// We may also need this for suspend/resume support.
if (!player_bridge_) {
// TODO(b/376316272): Turn `state` into its string form and consolidate all
// player state loggings below.
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with " << state;
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with "
<< GetPlayerStateName(state);
return;
}

switch (state) {
case kSbPlayerStateInitialized:
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with "
"kSbPlayerStateInitialized.";
<< GetPlayerStateName(state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I checked the code again, and it seems that it's better to use a single LOG(INFO) << "..." in the very beginning of this function, so we can remove all the other ones. The only exception is the line to log audio_write_duration_, which can be changed to just log the audio_write_duration_.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we follow this path, we may log in if (!player_bridge_) that the function is called when player_bridge_ is nullptr, as the current log of the state will be replaced by the one in the very beginning of the function. We may also turn it into a LOG(WARNING).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up: #4679

DCHECK(!player_bridge_initialized_);
player_bridge_initialized_ = true;

Expand All @@ -784,7 +785,7 @@ void StarboardRenderer::OnPlayerStatus(SbPlayerState state) {
break;
case kSbPlayerStatePrerolling:
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with "
"kSbPlayerStatePrerolling.";
<< GetPlayerStateName(state);
DCHECK(player_bridge_initialized_);
break;
case kSbPlayerStatePresenting:
Expand All @@ -794,19 +795,19 @@ void StarboardRenderer::OnPlayerStatus(SbPlayerState state) {
HasRemoteAudioOutputs(player_bridge_->GetAudioConfigurations())
? audio_write_duration_remote_
: audio_write_duration_local_;
LOG(INFO) << "SbPlayerBridge reaches kSbPlayerStatePresenting, with audio"
<< " write duration at " << audio_write_duration_;
LOG(INFO) << "SbPlayerBridge reaches " << GetPlayerStateName(state)
<< ", with audio write duration at " << audio_write_duration_;
DCHECK(player_bridge_initialized_);
break;
case kSbPlayerStateEndOfStream:
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with "
"kSbPlayerStateEndOfStream.";
<< GetPlayerStateName(state);
DCHECK(player_bridge_initialized_);
client_->OnEnded();
break;
case kSbPlayerStateDestroyed:
LOG(INFO) << "StarboardRenderer::OnPlayerStatus() called with "
"kSbPlayerStateDestroyed.";
<< GetPlayerStateName(state);
break;
}
}
Expand Down
18 changes: 18 additions & 0 deletions starboard/common/player.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,22 @@ const char* GetPlayerOutputModeName(SbPlayerOutputMode output_mode) {
return "invalid";
}

const char* GetPlayerStateName(SbPlayerState state) {
switch (state) {
case kSbPlayerStateInitialized:
return "kSbPlayerStateInitialized";
case kSbPlayerStatePrerolling:
return "kSbPlayerStatePrerolling";
case kSbPlayerStatePresenting:
return "kSbPlayerStatePresenting";
case kSbPlayerStateEndOfStream:
return "kSbPlayerStateEndOfStream";
case kSbPlayerStateDestroyed:
return "kSbPlayerStateDestroyed";
}

SB_NOTREACHED();
return "invalid";
}

} // namespace starboard
2 changes: 2 additions & 0 deletions starboard/common/player.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ namespace starboard {

const char* GetPlayerOutputModeName(SbPlayerOutputMode output_mode);

const char* GetPlayerStateName(SbPlayerState state);

} // namespace starboard

#endif // STARBOARD_COMMON_PLAYER_H_
13 changes: 13 additions & 0 deletions starboard/common/player_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,18 @@ TEST(PlayerTest, GetPlayerOutputModeName) {
ASSERT_STREQ(GetPlayerOutputModeName(kSbPlayerOutputModeInvalid), "invalid");
}

TEST(PlayerTest, GetPlayerStateName) {
ASSERT_STREQ(GetPlayerStateName(kSbPlayerStateInitialized),
"kSbPlayerStateInitialized");
ASSERT_STREQ(GetPlayerStateName(kSbPlayerStatePrerolling),
"kSbPlayerStatePrerolling");
ASSERT_STREQ(GetPlayerStateName(kSbPlayerStatePresenting),
"kSbPlayerStatePresenting");
ASSERT_STREQ(GetPlayerStateName(kSbPlayerStateEndOfStream),
"kSbPlayerStateEndOfStream");
ASSERT_STREQ(GetPlayerStateName(kSbPlayerStateDestroyed),
"kSbPlayerStateDestroyed");
}

} // namespace
} // namespace starboard
4 changes: 3 additions & 1 deletion starboard/shared/starboard/player/filter/filter_based_player_worker_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "starboard/audio_sink.h"
#include "starboard/common/log.h"
#include "starboard/common/murmurhash2.h"
#include "starboard/common/player.h"
#include "starboard/common/string.h"
#include "starboard/shared/starboard/application.h"
#include "starboard/shared/starboard/drm/drm_system_internal.h"
Expand All @@ -35,6 +36,7 @@ namespace filter {

namespace {

using ::starboard::GetPlayerStateName;
using std::placeholders::_1;
using std::placeholders::_2;

Expand Down Expand Up @@ -432,7 +434,7 @@ void FilterBasedPlayerWorkerHandler::OnPrerolled(SbMediaType media_type) {
}

SB_DCHECK(get_player_state_cb_() == kSbPlayerStatePrerolling)
<< "Invalid player state " << get_player_state_cb_();
<< "Invalid player state " << GetPlayerStateName(get_player_state_cb_());

if (media_type == kSbMediaTypeAudio) {
SB_LOG(INFO) << "Audio prerolled.";
Expand Down
6 changes: 4 additions & 2 deletions starboard/shared/starboard/player/player_worker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "starboard/common/condition_variable.h"
#include "starboard/common/instance_counter.h"
#include "starboard/common/mutex.h"
#include "starboard/common/player.h"
#include "starboard/shared/pthread/thread_create_priority.h"

namespace starboard {
Expand All @@ -32,6 +33,7 @@ namespace player {

namespace {

using ::starboard::GetPlayerStateName;
using std::placeholders::_1;
using std::placeholders::_2;
using std::placeholders::_3;
Expand Down Expand Up @@ -278,7 +280,7 @@ void PlayerWorker::DoWriteSamples(InputBuffers input_buffers) {
player_state_ == kSbPlayerStateEndOfStream ||
player_state_ == kSbPlayerStateDestroyed) {
SB_LOG(ERROR) << "Tried to write sample when |player_state_| is "
<< player_state_;
<< GetPlayerStateName(player_state_);
return;
}
if (error_occurred_) {
Expand Down Expand Up @@ -347,7 +349,7 @@ void PlayerWorker::DoWriteEndOfStream(SbMediaType sample_type) {
if (player_state_ == kSbPlayerStateInitialized ||
player_state_ == kSbPlayerStateEndOfStream) {
SB_LOG(ERROR) << "Tried to write EOS when |player_state_| is "
<< player_state_;
<< GetPlayerStateName(player_state_);
return;
}

Expand Down
Loading