VPLAY-12337 : Crash issue is observed while switch between different … #876
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds extensive debug logging and a debug simulation feature to diagnose a crash issue occurring during asset switching. The changes instrument the PreCache thread lifecycle and playlist download operations with timing metrics and state information.
Changes:
- Added debug logging with developer's name prefix ("HariPriya") throughout PreCache thread operations
- Added timing measurements for thread join operations and network I/O calls
- Introduced a debug-only environment variable feature to simulate slow network conditions
- Enhanced logging at thread lifecycle boundaries and critical state transitions
Comments suppressed due to low confidence (2)
priv_aamp.cpp:10362
- The code uses legacy C-style usleep() instead of modern C++ std::this_thread::sleep_for(). For consistency with the debug delay code added in this PR (line 10339) and modern C++ practices, replace usleep() calls with std::this_thread::sleep_for().
usleep(1000000);
priv_aamp.cpp:10283
- Thread lifecycle events (start/exit) are logged at WARN level. These are informational events, not warnings. Consider using AAMPLOG_INFO for normal thread lifecycle events and reserve WARN for actual problematic conditions.
AAMPLOG_WARN("PreCachePlaylistDownloadTask: Thread started");
| { | ||
| std::lock_guard<std::mutex> guard(mMutexPlaystart); | ||
| waitforplaystart.notify_all(); | ||
| AAMPLOG_WARN("HariPriya Stop: notified PreCache thread, isDestructing=%d", isDestructing); |
There was a problem hiding this comment.
Log messages contain the developer's name 'HariPriya'. This is unprofessional and makes logs harder to search and filter. Remove the name prefix from all log messages (lines 8154, 8159, 8162, 8166, 10328, 10338, 10345, 10370) and use descriptive, context-specific prefixes instead.
| { | ||
| std::unique_lock<std::mutex> lock(mMutexPlaystart); | ||
| AAMPLOG_WARN("PreCachePlaylistDownloadTask: Waiting for playstart notification"); | ||
| waitforplaystart.wait(lock); |
There was a problem hiding this comment.
Condition variable wait() is called without a predicate, making it vulnerable to spurious wakeups. This could lead to premature continuation if the thread wakes up before the actual playstart notification. Use wait() with a predicate or manually check the condition in a loop after waking.
| // DEBUG: Simulate slow network to reproduce GC deadlock issue | ||
| // Set AAMP_SIMULATE_SLOW_PRECACHE_MS env var to inject delay (e.g., export AAMP_SIMULATE_SLOW_PRECACHE_MS=5000) | ||
| const char* delayEnv = getenv("AAMP_SIMULATE_SLOW_PRECACHE_MS"); | ||
| if (delayEnv) | ||
| { | ||
| int delayMs = atoi(delayEnv); | ||
| if (delayMs > 0) | ||
| { | ||
| AAMPLOG_WARN("HariPriya PreCachePlaylistDownloadTask: DEBUG - Simulating slow network with %d ms delay", delayMs); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(delayMs)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Debug/test simulation code should not be committed to production code. This artificial delay mechanism should either be removed or wrapped in a DEBUG or ENABLE_DEBUG_FEATURES preprocessor conditional. The environment variable check adds overhead to every playlist download in production builds.
| const char* delayEnv = getenv("AAMP_SIMULATE_SLOW_PRECACHE_MS"); | ||
| if (delayEnv) | ||
| { | ||
| int delayMs = atoi(delayEnv); |
There was a problem hiding this comment.
Using atoi() without input validation could lead to undefined behavior if the environment variable contains non-numeric values. Use strtol() with proper error checking, or at minimum validate that delayEnv is not empty and contains only digits before calling atoi().
| } | ||
| if(mPreCachePlaylistThreadId.joinable()) | ||
| { | ||
| auto joinStartTime = NOW_STEADY_TS_MS; |
There was a problem hiding this comment.
The 'auto' keyword is used for type deduction of timing variables. For clarity and consistency with the codebase's use of explicit types for timing values (seen elsewhere as 'long long'), explicitly declare the type as 'long long' instead of using 'auto'.
…assets Reason for change: added probable fix for local storage ; instead of using join() use detach() so that thread will not be blocked Test Procedure: updated in the ticket Priority: P1 Risks: Low Signed-off-by: haripriya_molakalapalli <haripriya_molakalapalli@comcast.com>
molakalapalliharipriya
force-pushed
the
feature/VPLAY-12337
branch
from
bcabd12 to
0dced84
Compare
…assets
Reason for change: debug crash issue
Test Procedure: updated in the ticket
Priority: P1
Risks: Low