Release Teacher 2.3.1 (86) #3440
Conversation
refs: MBL-19580 affects: Student, Teacher release note: none
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
This PR removes the RouteUtils.getMediaUri() functionality and refactors media handling to simplify video playback. While the intention to streamline the code is good, there are several concerns that need to be addressed:
Critical Issues
-
Bug in BaseRouterActivity.kt:319 - Removed
RouteUtils.getMediaUri()call means the code now checks file extensions on redirect URLs instead of the actual media URLs. Canvas media URLs like/media_objects_iframe/123?type=videowon't have.mpd,.m3u8, or.mp4extensions, causing media files to incorrectly not open internally. -
Performance issue in OpenMediaAsyncTaskLoader.kt:285-325 - The new
downloadWithHeaders()method establishes HTTP connection and prepares to download the response body BEFORE checking if the file is already cached. This wastes bandwidth and time for cached files. The original implementation checked cache first, then only downloaded if needed. -
Logic issue in OpenMediaAsyncTaskLoader.kt:158 - For remote URLs, the intent data is set to
Uri.parse(url)(the raw URL) before downloading. This may cause the app to try opening the URL directly instead of waiting for the local file download to complete.
Positive Changes
✅ ExoPlayerHelper.kt automatic retry - The new logic to automatically retry with .mpd extension when encountering UnrecognizedInputFormatException is a clever workaround for DASH videos without proper extensions.
✅ Code simplification - Removing the coroutine-based RouteUtils.getMediaUri() simplifies the async handling in multiple places.
✅ Unified download logic - The new downloadWithHeaders() method consolidates header parsing and downloading in one place.
Recommendations
-
Keep URL resolution logic: The
RouteUtils.getMediaUri()functionality was solving a real problem - Canvas redirects don't have file extensions in the URL. Either keep this function or implement proper URL resolution before checking extensions. -
Fix cache checking: Move file existence check BEFORE making network requests. Consider using HEAD requests to get headers without downloading when you need metadata for cached files.
-
Test thoroughly: This changes core media handling across all three apps. Test with:
- Canvas media redirects
- Direct media URLs
- Cached vs non-cached files
- Different media types (.mpd, .m3u8, .mp4)
- DASH videos with and without proper extensions
Minor Issues
- Missing newline at end of OpenMediaAsyncTaskLoader.kt (line 469)
- Consider adding debug logging when ExoPlayer retries with
.mpdextension
Test Coverage
The removal of tests in RouteUtilsTest.kt is concerning - those tests were validating important redirect and content-type handling logic. Consider adding equivalent tests for the new implementation to prevent regressions.
No description provided.