-
Notifications
You must be signed in to change notification settings - Fork 108
Release Student 8.4.1 (284) #3439
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.
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
Conversation
refs: MBL-19580 affects: Student, Teacher release note: none * Fixed asyncTaskLoader * ExoPlayer fixes * Removed unused stuff. * Changed pr-pipeline * fixed local videos --------- Co-authored-by: kristof.nemere <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR removes the RouteUtils.getMediaUri() async redirect-following logic and simplifies media URL handling across the codebase. The change reduces complexity by eliminating network calls for URL resolution, but there are some concerns about potential behavioral changes.
Issues Found
- Critical: Loss of redirect handling - The original code followed HTTP redirects to resolve final media URLs. The new code uses URLs directly, which may break media playback if Canvas uses redirects to CDNs or signed URLs (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt)
- Null safety issue - Missing null check for response body could cause NPE (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:342)
- Error handling gaps -
downloadWithHeaders()throws IOException but callers may not handle it properly (libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt:285) - State management issue -
triedMpdRetryflag in ExoPlayerHelper is never reset, could cause issues if agent is reused (libs/pandautils/src/main/java/com/instructure/pandautils/utils/ExoPlayerHelper.kt:174) - Simplified URL detection - BaseRouterActivity now checks URL extensions directly instead of following redirects, may miss media files with redirect URLs (apps/student/src/main/java/com/instructure/student/activity/BaseRouterActivity.kt:319)
Positive Changes
- Improved performance: Eliminates unnecessary HEAD requests for media URLs, reducing latency and network usage
- Code simplification: Removes async complexity from media URI resolution, making the code easier to maintain
- Better caching logic: The refactored
downloadWithHeaders()method consolidates header parsing and file download into a single request - Smart retry logic: ExoPlayerHelper adds automatic
.mpdextension retry for DASH videos, which is a nice improvement
Testing Recommendations
- Redirect scenarios: Test with Canvas media URLs that redirect to CDN endpoints
- DASH videos: Verify the
.mpdretry logic works correctly in ExoPlayerHelper - Error cases: Test network failures, malformed URLs, and missing Content-Type headers
- File format detection: Verify media playback works for URLs without file extensions
- Agent reuse: Test that ExoPlayerHelper works correctly when reused for multiple videos
Security Considerations
No significant security issues identified. The code properly uses authentication headers and follows secure practices for file handling.
Performance
Overall positive performance impact due to elimination of the HEAD request in RouteUtils.getMediaUri(). However, ensure this doesn't cause issues with media that requires redirect resolution.
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/utils/ExoPlayerHelper.kt
Show resolved
Hide resolved
apps/student/src/main/java/com/instructure/student/activity/BaseRouterActivity.kt
Show resolved
Hide resolved
libs/pandautils/src/main/java/com/instructure/pandautils/loaders/OpenMediaAsyncTaskLoader.kt
Show resolved
Hide resolved
adamNagy56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
kdeakinstructure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA 👍
No description provided.