Skip to content

Commit 12cf2a4

Browse files
kjyounkjyoun
authored
starboard: Harden thread safety checks and simplify ThreadChecker (#7499)
This change improves thread safety and robustness by enabling thread checks in release builds and simplifying the underlying ThreadChecker implementation. Key changes: - `ThreadChecker` is no longer a no-op in release builds. It has been simplified to always bind to its creation thread, removing complex atomic and lazy-initialization logic. - All usages of `SB_DCHECK(thread_checker_.CalledOnValidThread())` have been converted to `SB_CHECK`. This elevates the check from a debug-only assertion to a fatal error in all build types, ensuring that any threading violations are caught immediately. - `JobQueue` is updated to use the new, simpler ThreadChecker, removing its own redundant thread ID tracking. Bug: 435425692
1 parent 3d88005 commit 12cf2a4

File tree

8 files changed

+44
-83
lines changed

8 files changed

+44
-83
lines changed

‎starboard/android/shared/audio_decoder_passthrough.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class AudioDecoderPassthrough : public AudioDecoder {
4040

4141
// AudioDecoder methods.
4242
void Initialize(const OutputCB& output_cb, const ErrorCB& error_cb) override {
43-
SB_DCHECK(thread_checker_.CalledOnValidThread());
43+
SB_CHECK(thread_checker_.CalledOnValidThread());
4444
SB_DCHECK(!output_cb_);
4545
SB_DCHECK(output_cb);
4646

@@ -49,7 +49,7 @@ class AudioDecoderPassthrough : public AudioDecoder {
4949

5050
void Decode(const InputBuffers& input_buffers,
5151
const ConsumedCB& consumed_cb) override {
52-
SB_DCHECK(thread_checker_.CalledOnValidThread());
52+
SB_CHECK(thread_checker_.CalledOnValidThread());
5353
SB_DCHECK(!input_buffers.empty());
5454
SB_DCHECK(consumed_cb);
5555
SB_DCHECK(output_cb_);
@@ -76,15 +76,15 @@ class AudioDecoderPassthrough : public AudioDecoder {
7676
}
7777

7878
void WriteEndOfStream() override {
79-
SB_DCHECK(thread_checker_.CalledOnValidThread());
79+
SB_CHECK(thread_checker_.CalledOnValidThread());
8080
SB_DCHECK(output_cb_);
8181

8282
decoded_audios_.push(new DecodedAudio);
8383
output_cb_();
8484
}
8585

8686
scoped_refptr<DecodedAudio> Read(int* samples_per_second) override {
87-
SB_DCHECK(thread_checker_.CalledOnValidThread());
87+
SB_CHECK(thread_checker_.CalledOnValidThread());
8888
SB_DCHECK(samples_per_second);
8989
SB_DCHECK(!decoded_audios_.empty());
9090

@@ -96,7 +96,7 @@ class AudioDecoderPassthrough : public AudioDecoder {
9696
}
9797

9898
void Reset() override {
99-
SB_DCHECK(thread_checker_.CalledOnValidThread());
99+
SB_CHECK(thread_checker_.CalledOnValidThread());
100100

101101
decoded_audios_ = std::queue<scoped_refptr<DecodedAudio>>(); // Clear
102102
}

‎starboard/android/shared/audio_sink_min_required_frames_tester.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ MinRequiredFramesTester::MinRequiredFramesTester(int max_required_frames,
5454
destroying_(false) {}
5555

5656
MinRequiredFramesTester::~MinRequiredFramesTester() {
57-
SB_DCHECK(thread_checker_.CalledOnValidThread());
57+
SB_CHECK(thread_checker_.CalledOnValidThread());
5858
destroying_.store(true);
5959
if (tester_thread_) {
6060
test_complete_cv_.notify_one();
@@ -69,7 +69,7 @@ void MinRequiredFramesTester::AddTest(
6969
int sample_rate,
7070
const OnMinRequiredFramesReceivedCallback& received_cb,
7171
int default_required_frames) {
72-
SB_DCHECK(thread_checker_.CalledOnValidThread());
72+
SB_CHECK(thread_checker_.CalledOnValidThread());
7373
// MinRequiredFramesTester doesn't support to add test after starts.
7474
SB_DCHECK(!tester_thread_);
7575

@@ -78,7 +78,7 @@ void MinRequiredFramesTester::AddTest(
7878
}
7979

8080
void MinRequiredFramesTester::Start() {
81-
SB_DCHECK(thread_checker_.CalledOnValidThread());
81+
SB_CHECK(thread_checker_.CalledOnValidThread());
8282
// MinRequiredFramesTester only supports to start once.
8383
SB_DCHECK(!tester_thread_);
8484

‎starboard/android/shared/video_frame_tracker.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include <cmath>
1919
#include <cstdint>
2020
#include <cstdlib>
21+
#include <list>
2122
#include <mutex>
2223
#include <vector>
2324

@@ -75,7 +76,7 @@ int64_t VideoFrameTracker::seek_to_time() const {
7576
}
7677

7778
void VideoFrameTracker::OnInputBuffer(int64_t timestamp) {
78-
SB_DCHECK(thread_checker_.CalledOnValidThread());
79+
SB_CHECK(thread_checker_.CalledOnValidThread());
7980

8081
if (frames_to_be_rendered_.empty()) {
8182
frames_to_be_rendered_.push_back(timestamp);
@@ -112,7 +113,7 @@ void VideoFrameTracker::OnFrameRendered(int64_t frame_timestamp) {
112113
}
113114

114115
void VideoFrameTracker::Seek(int64_t seek_to_time) {
115-
SB_DCHECK(thread_checker_.CalledOnValidThread());
116+
SB_CHECK(thread_checker_.CalledOnValidThread());
116117

117118
// Ensure that all dropped frames before seeking are captured.
118119
UpdateDroppedFrames();
@@ -122,13 +123,13 @@ void VideoFrameTracker::Seek(int64_t seek_to_time) {
122123
}
123124

124125
int VideoFrameTracker::UpdateAndGetDroppedFrames() {
125-
SB_DCHECK(thread_checker_.CalledOnValidThread());
126+
SB_CHECK(thread_checker_.CalledOnValidThread());
126127
UpdateDroppedFrames();
127128
return dropped_frames_;
128129
}
129130

130131
void VideoFrameTracker::UpdateDroppedFrames() {
131-
SB_DCHECK(thread_checker_.CalledOnValidThread());
132+
SB_CHECK(thread_checker_.CalledOnValidThread());
132133

133134
{
134135
std::lock_guard lock(rendered_frames_mutex_);

‎starboard/nplb/player_test_fixture.cc‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,13 @@ SbPlayerTestFixture::SbPlayerTestFixture(
232232
}
233233

234234
SbPlayerTestFixture::~SbPlayerTestFixture() {
235-
SB_DCHECK(thread_checker_.CalledOnValidThread());
235+
SB_CHECK(thread_checker_.CalledOnValidThread());
236236

237237
TearDown();
238238
}
239239

240240
void SbPlayerTestFixture::Seek(const int64_t time) {
241-
SB_DCHECK(thread_checker_.CalledOnValidThread());
241+
SB_CHECK(thread_checker_.CalledOnValidThread());
242242
SB_DCHECK(SbPlayerIsValid(player_));
243243

244244
ASSERT_FALSE(error_occurred_);
@@ -256,7 +256,7 @@ void SbPlayerTestFixture::Seek(const int64_t time) {
256256
}
257257

258258
void SbPlayerTestFixture::Write(const GroupedSamples& grouped_samples) {
259-
SB_DCHECK(thread_checker_.CalledOnValidThread());
259+
SB_CHECK(thread_checker_.CalledOnValidThread());
260260
SB_DCHECK(SbPlayerIsValid(player_));
261261
SB_DCHECK(!audio_end_of_stream_written_);
262262
SB_DCHECK(!video_end_of_stream_written_);
@@ -331,15 +331,15 @@ void SbPlayerTestFixture::Write(const GroupedSamples& grouped_samples) {
331331
}
332332

333333
void SbPlayerTestFixture::WaitForPlayerPresenting() {
334-
SB_DCHECK(thread_checker_.CalledOnValidThread());
334+
SB_CHECK(thread_checker_.CalledOnValidThread());
335335
SB_DCHECK(SbPlayerIsValid(player_));
336336

337337
ASSERT_FALSE(error_occurred_);
338338
ASSERT_NO_FATAL_FAILURE(WaitForPlayerState(kSbPlayerStatePresenting));
339339
}
340340

341341
void SbPlayerTestFixture::WaitForPlayerEndOfStream() {
342-
SB_DCHECK(thread_checker_.CalledOnValidThread());
342+
SB_CHECK(thread_checker_.CalledOnValidThread());
343343
SB_DCHECK(SbPlayerIsValid(player_));
344344
SB_DCHECK(!audio_dmp_reader_ || audio_end_of_stream_written_);
345345
SB_DCHECK(!video_dmp_reader_ || video_end_of_stream_written_);
@@ -355,7 +355,7 @@ int64_t SbPlayerTestFixture::GetCurrentMediaTime() const {
355355
}
356356

357357
void SbPlayerTestFixture::SetAudioWriteDuration(int64_t duration) {
358-
SB_DCHECK(thread_checker_.CalledOnValidThread());
358+
SB_CHECK(thread_checker_.CalledOnValidThread());
359359
SB_DCHECK_GT(duration, 0);
360360
audio_write_duration_ = duration;
361361
}
@@ -435,7 +435,7 @@ void SbPlayerTestFixture::OnError(SbPlayer player,
435435
}
436436

437437
void SbPlayerTestFixture::Initialize() {
438-
SB_DCHECK(thread_checker_.CalledOnValidThread());
438+
SB_CHECK(thread_checker_.CalledOnValidThread());
439439

440440
// Initialize drm system.
441441
if (!key_system_.empty()) {
@@ -474,7 +474,7 @@ void SbPlayerTestFixture::Initialize() {
474474
}
475475

476476
void SbPlayerTestFixture::TearDown() {
477-
SB_DCHECK(thread_checker_.CalledOnValidThread());
477+
SB_CHECK(thread_checker_.CalledOnValidThread());
478478

479479
// We should always destroy |player_| and |drm_system_|, no matter if there's
480480
// any unexpected player error.
@@ -521,7 +521,7 @@ void SbPlayerTestFixture::WriteAudioSamples(
521521
int64_t timestamp_offset,
522522
int64_t discarded_duration_from_front,
523523
int64_t discarded_duration_from_back) {
524-
SB_DCHECK(thread_checker_.CalledOnValidThread());
524+
SB_CHECK(thread_checker_.CalledOnValidThread());
525525
SB_DCHECK(SbPlayerIsValid(player_));
526526
SB_DCHECK(audio_dmp_reader_);
527527
SB_DCHECK_GE(start_index, 0);
@@ -550,7 +550,7 @@ void SbPlayerTestFixture::WriteAudioSamples(
550550

551551
void SbPlayerTestFixture::WriteVideoSamples(int start_index,
552552
int samples_to_write) {
553-
SB_DCHECK(thread_checker_.CalledOnValidThread());
553+
SB_CHECK(thread_checker_.CalledOnValidThread());
554554
SB_DCHECK_GE(start_index, 0);
555555
SB_DCHECK_GT(samples_to_write, 0);
556556
SB_DCHECK(SbPlayerIsValid(player_));
@@ -566,7 +566,7 @@ void SbPlayerTestFixture::WriteVideoSamples(int start_index,
566566
}
567567

568568
void SbPlayerTestFixture::WriteEndOfStream(SbMediaType media_type) {
569-
SB_DCHECK(thread_checker_.CalledOnValidThread());
569+
SB_CHECK(thread_checker_.CalledOnValidThread());
570570
SB_DCHECK(SbPlayerIsValid(player_));
571571

572572
if (media_type == kSbMediaTypeAudio) {
@@ -586,7 +586,7 @@ void SbPlayerTestFixture::WriteEndOfStream(SbMediaType media_type) {
586586
}
587587

588588
void SbPlayerTestFixture::WaitAndProcessNextEvent(int64_t timeout) {
589-
SB_DCHECK(thread_checker_.CalledOnValidThread());
589+
SB_CHECK(thread_checker_.CalledOnValidThread());
590590

591591
auto event = callback_event_queue_.GetTimed(timeout);
592592

@@ -629,7 +629,7 @@ void SbPlayerTestFixture::WaitAndProcessNextEvent(int64_t timeout) {
629629
}
630630

631631
void SbPlayerTestFixture::WaitForDecoderStateNeedsData(const int64_t timeout) {
632-
SB_DCHECK(thread_checker_.CalledOnValidThread());
632+
SB_CHECK(thread_checker_.CalledOnValidThread());
633633

634634
bool old_can_accept_more_audio_data = can_accept_more_audio_data_;
635635
bool old_can_accept_more_video_data = can_accept_more_video_data_;
@@ -648,7 +648,7 @@ void SbPlayerTestFixture::WaitForDecoderStateNeedsData(const int64_t timeout) {
648648

649649
void SbPlayerTestFixture::WaitForPlayerState(const SbPlayerState desired_state,
650650
const int64_t timeout) {
651-
SB_DCHECK(thread_checker_.CalledOnValidThread());
651+
SB_CHECK(thread_checker_.CalledOnValidThread());
652652

653653
if (HasReceivedPlayerState(desired_state)) {
654654
return;

‎starboard/shared/starboard/player/job_queue.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ void ResetCurrentThreadJobQueue() {
6464

6565
} // namespace
6666

67-
JobQueue::JobQueue() : thread_id_(SbThreadGetId()) {
68-
SB_DCHECK(SbThreadIsValidId(thread_id_));
67+
JobQueue::JobQueue() {
6968
SetCurrentThreadJobQueue(this);
7069
}
7170

@@ -159,7 +158,7 @@ bool JobQueue::BelongsToCurrentThread() const {
159158
// The ctor already ensures that the current JobQueue is the only JobQueue of
160159
// the thread, checking for thread id is more light-weighted then calling
161160
// JobQueue::current() and compare the result with |this|.
162-
return thread_id_ == SbThreadGetId();
161+
return thread_checker_.CalledOnValidThread();
163162
}
164163

165164
// static

‎starboard/shared/starboard/player/job_queue.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
#include "starboard/common/log.h"
2626
#include "starboard/common/time.h"
2727
#include "starboard/shared/internal_only.h"
28-
#include "starboard/thread.h"
28+
#include "starboard/shared/starboard/thread_checker.h"
2929

3030
#ifndef __cplusplus
3131
#error "Only C++ files can include this header."
@@ -169,7 +169,7 @@ class JobQueue {
169169
// be run.
170170
bool TryToRunOneJob(bool wait_for_next_job);
171171

172-
const SbThreadId thread_id_;
172+
ThreadChecker thread_checker_;
173173
std::mutex mutex_;
174174
std::condition_variable condition_;
175175
int64_t current_job_token_ = JobToken::kInvalidToken + 1;

‎starboard/shared/starboard/thread_checker.h‎

Lines changed: 5 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -18,63 +18,23 @@
1818
#include <atomic>
1919

2020
#include "build/build_config.h"
21+
#include "starboard/common/log.h"
2122
#include "starboard/thread.h"
2223

2324
namespace starboard {
2425

25-
#if BUILDFLAG(COBALT_IS_RELEASE_BUILD)
26-
27-
class ThreadChecker {
28-
public:
29-
enum Type { kSetThreadIdOnCreation, kSetThreadIdOnFirstCheck };
30-
31-
explicit ThreadChecker(Type type = kSetThreadIdOnCreation) {}
32-
33-
void Detach() {}
34-
35-
bool CalledOnValidThread() const { return true; }
36-
};
37-
38-
#else // BUILDFLAG(COBALT_IS_RELEASE_BUILD)
39-
4026
class ThreadChecker {
4127
public:
42-
enum Type { kSetThreadIdOnCreation, kSetThreadIdOnFirstCheck };
43-
44-
explicit ThreadChecker(Type type = kSetThreadIdOnCreation) {
45-
if (type == kSetThreadIdOnCreation) {
46-
thread_id_ = SbThreadGetId();
47-
} else {
48-
thread_id_ = kSbThreadInvalidId;
49-
}
28+
ThreadChecker() : thread_id_(SbThreadGetId()) {
29+
SB_CHECK(SbThreadIsValidId(thread_id_));
5030
}
5131

52-
// Detached the thread checker from its current thread. The thread checker
53-
// will re-attach to a thread when CalledOnValidThread() is called again.
54-
// This essentially reset the thread checker to a status just like that it
55-
// was created with 'kSetThreadIdOnFirstCheck'.
56-
void Detach() {
57-
// This is safe as when this function is called, it is expected that it
58-
// won't be called on its current thread.
59-
thread_id_ = kSbThreadInvalidId;
60-
}
61-
62-
bool CalledOnValidThread() const {
63-
SbThreadId current_thread_id = SbThreadGetId();
64-
SbThreadId stored_thread_id = kSbThreadInvalidId;
65-
thread_id_.compare_exchange_weak(stored_thread_id, current_thread_id,
66-
std::memory_order_relaxed,
67-
std::memory_order_relaxed);
68-
return stored_thread_id == kSbThreadInvalidId ||
69-
stored_thread_id == current_thread_id;
70-
}
32+
bool CalledOnValidThread() const { return thread_id_ == SbThreadGetId(); }
7133

7234
private:
73-
mutable std::atomic<SbThreadId> thread_id_;
35+
const SbThreadId thread_id_;
7436
};
7537

76-
#endif // BUILDFLAG(COBALT_IS_RELEASE_BUILD)
77-
7838
// Alias to prevent breaking the RDK build on CI.
7939
// See https://paste.googleplex.com/4776103591936000
8040
// TODO: b/441955897 - Remove this alias once RDK build on CI is updated

0 commit comments

Comments
 (0)