Title: Refactor audio streaming, remove API delays [feat]#2
Conversation
… handling [feat] Details: 1. Refactor `BiliAudioSource` to utilize `DownloadManager` for unified network streaming and caching, replacing legacy file-based buffering. 2. Remove artificial request delays from `SearchApi`, `LibraryApi`, `DiscoverApi`, and `VideoDetailApi` to improve performance. 3. Implement WBI key invalidation and retry logic in `AudioStream` to automatically resolve 403/412 signature errors. 4. Update `DownloadManager` to simplify cache states (remove buffering status) and introduce `ResourceException` for better error reporting. 5. Enhance `PlayerProvider` and `AudioPlayerService` to detect fatal resource errors (e.g., 403/404) and auto-skip invalid songs. 6. Add `updateCid` to `DatabaseService` and implement logic to update CIDs in the database when resolved during search or playback. 7. Clean up `SettingsProvider` by deprecating request delay configuration. Type: Feature. Test: N/A. Related Issue: N/A.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the audio streaming and caching logic, moving from a file-based buffering system to a more efficient direct network streaming approach within BiliAudioSource. The removal of artificial API delays across various files is a great performance enhancement. The introduction of WBI key invalidation and retry logic in AudioStreamApi is a solid improvement for handling API signature errors.
My review focuses on further improving the robustness of the new error handling and refining some of the new logic. Specifically, I've pointed out a potential bug in the error detection in BiliAudioSource, suggested tightening exception handling in AudioStreamApi, and recommended consolidating some duplicated logic for CID resolution. I've also noted a minor code cleanup opportunity and a place where a comment would improve code clarity.
Overall, these are excellent changes that modernize a core part of the application and improve its performance and resilience.
| if (e.toString().contains("Fatal Resource")) { | ||
| onFatalError?.call(bvid, _realCid, e.toString()); | ||
| } |
There was a problem hiding this comment.
The error handling here relies on checking the string representation of the exception (e.toString().contains("Fatal Resource")). This is brittle and might not work as expected if the exception message changes. Since DownloadManager throws a specific ResourceException for these cases, it would be more robust to catch that specific exception type.
| if (e.toString().contains("Fatal Resource")) { | |
| onFatalError?.call(bvid, _realCid, e.toString()); | |
| } | |
| if (e is ResourceException) { | |
| onFatalError?.call(bvid, _realCid, e.toString()); | |
| } |
| } catch (e) { | ||
| print('Error fetching audio stream: $e'); | ||
| if (!isRetry) { | ||
| // 如果是网络异常，也可以尝试刷新一下（虽然主要是 WBI 问题，但防一手） | ||
| return await _getAudioStreamInternal( | ||
| bvid, | ||
| cid, | ||
| qn, | ||
| preferredQuality, | ||
| isRetry: true, | ||
| ); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The catch (e) block is too broad and will catch any type of exception, including programming errors. This could lead to unintended retries for non-recoverable errors. It's better to catch specific network-related exceptions like DioException or SocketException to make the retry logic more robust and predictable.
| import 'package:shared_preferences/shared_preferences.dart'; | ||
| import 'package:utopia_music/providers/auth_provider.dart'; | ||
| import 'package:utopia_music/providers/settings_provider.dart'; | ||
| import 'package:utopia_music/providers/settings_provider.dart'; |
There was a problem hiding this comment.
This import of settings_provider.dart is a duplicate of the one on the previous line. Removing it will clean up the code.
| Future<void> _handlePlaybackError(Map<String, dynamic> error) async { | ||
| if (!_autoSkipInvalid) return; | ||
|
|
||
| await Future.delayed(const Duration(milliseconds: 300)); |
There was a problem hiding this comment.
This Future.delayed introduces a hardcoded delay. For better maintainability, please add a comment explaining why this delay is necessary. For example, it might be to prevent race conditions or to allow the UI to update before proceeding with error handling.
| if (_realCid == 0) { | ||
| try { | ||
| print("BiliAudioSource: CID is 0, fetching..."); | ||
| _realCid = await _searchApi.fetchCid(bvid); | ||
| print("BiliAudioSource: Resolved CID: $_realCid"); | ||
| } catch (e) { | ||
| print("BiliAudioSource: Failed to resolve CID: $e"); | ||
| } | ||
|
|
||
| } | ||
| final targetCid = _realCid == 0 ? initCid : _realCid; |
There was a problem hiding this comment.
The CID resolution logic appears to be duplicated. BiliAudioSource attempts to resolve the CID here, and if it fails, DownloadManager.checkLocalResource and DownloadManager.getNetworkStream will attempt to resolve it again. To improve clarity and avoid redundant API calls, it would be better to consolidate the CID resolution. BiliAudioSource could be made solely responsible for ensuring it has a valid CID. If resolution fails here, it should probably throw an exception immediately rather than proceeding with a CID of 0.
1 participant
Details:
BiliAudioSourceto utilizeDownloadManagerfor unified network streaming and caching, replacing legacy file-based buffering.SearchApi,LibraryApi,DiscoverApi, andVideoDetailApito improve performance.AudioStreamto automatically resolve 403/412 signature errors.DownloadManagerto simplify cache states (remove buffering status) and introduceResourceExceptionfor better error reporting.PlayerProviderandAudioPlayerServiceto detect fatal resource errors (e.g., 403/404) and auto-skip invalid songs.updateCidtoDatabaseServiceand implement logic to update CIDs in the database when resolved during search or playback.SettingsProviderby deprecating request delay configuration. Type: Feature.Test: N/A.
Related Issue: N/A.