dash support #1047
dash support #1047
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1047 +/- ##
================================================
- Coverage 61.4635% 61.4291% -0.0344%
================================================
Files 115 117 +2
Lines 9347 9502 +155
Branches 1145 1156 +11
================================================
+ Hits 5745 5837 +92
- Misses 3602 3665 +63 ☔ View full report in Codecov by Sentry. |
1 failed test on run #820 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| An uncaught error was detected outside of a test |
Screenshots
Video
|
|
Review all test suite changes for PR #1047 ↗︎
WalkthroughThe service has been updated to support DASH streaming, expanding its video format capabilities. This includes handling DASH manifest files, enabling video playback of DASH content, and integrating DASH streaming into the existing browser interface. The update ensures compatibility with video controls and seamless streaming of DASH alongside other formats. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai can you generate some unit tests for the mpd parsing stuff in server/services/dash.ts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
client/package.jsonis excluded by:!**/*.jsonserver/package.jsonis excluded by:!**/*.jsonyarn.lockis excluded by:!**/*.lock
Files selected for processing (11)
- client/src/components/AddPreview.vue (1 hunks)
- client/src/components/players/OmniPlayer.vue (1 hunks)
- client/src/components/players/PlyrPlayer.vue (3 hunks)
- common/constants.ts (1 hunks)
- server/infoextractor.ts (2 hunks)
- server/mime.ts (2 hunks)
- server/services/dash.ts (1 hunks)
- server/services/parsing/iso8601.ts (1 hunks)
- server/services/youtube.ts (3 hunks)
- server/tests/unit/services/parsing/iso8601.spec.ts (1 hunks)
- server/tests/unit/services/youtube.spec.ts (1 hunks)
Additional comments: 14
common/constants.ts (1)
- 10-10: The addition of "dash" to the
ALL_VIDEO_SERVICESarray is consistent with the PR's objective to add DASH support.server/tests/unit/services/parsing/iso8601.spec.ts (1)
- 3-14: The test cases provided for the
parseIso8601Durationfunction cover a variety of ISO 8601 duration formats, which is good for ensuring the function's correctness across different formats.server/mime.ts (2)
20-20: The addition of the "application/dash+xml" MIME type is correct and aligns with the PR's goal to support DASH content.
32-32: The update to the
isSupportedMimeTypefunction to include "application/dash+xml" is correct and necessary for the DASH support.client/src/components/players/PlyrPlayer.vue (3)
11-11: The import of
dashjsis correct and necessary for DASH support within thePlyrPlayercomponent.40-40: The declaration of the
dashvariable is correct and aligns with the initialization of the DASH player later in the code.252-293: The conditional logic and event handling for the DASH video format are correctly implemented. The use of
dashjsto initialize and manage DASH content is appropriate.client/src/components/AddPreview.vue (1)
- 220-224: The addition of test entries for DASH streaming URLs is consistent with the PR's objective to support DASH content. This will be useful for development and testing purposes.
client/src/components/players/OmniPlayer.vue (1)
- 77-77: The modification to include the
dashservice in the conditional check forsource.serviceis correct and necessary for theOmniPlayercomponent to handle DASH streams.server/infoextractor.ts (2)
29-29: The import statement for
DashVideoAdapteris correctly placed and follows the existing pattern of importing service adapters.86-88: The conditional inclusion of
DashVideoAdapterin theadaptersarray based on theenabledconfiguration is a standard pattern used for other adapters in this file. This ensures that theDashVideoAdapteris only used when DASH support is enabled in the configuration.server/tests/unit/services/youtube.spec.ts (1)
- 533-538: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-1]
The removal of the
parseVideoLengthfunction and its associated test cases from theYoutubeservice is consistent with the changes in theyoutube.tsfile where theparseVideoLengthmethod was replaced withparseIso8601Duration. This change should be verified to ensure that the new function is covered by tests and that the removal of the old tests does not decrease the test coverage.server/services/youtube.ts (2)
18-18: The import of
parseIso8601Durationis correctly placed and follows the existing pattern of importing utility functions.547-547: The update to use
parseIso8601Durationfor parsing video lengths is correct and aligns with the removal of theparseVideoLengthmethod. This change centralizes the logic for parsing ISO 8601 durations and should be verified to ensure that it is correctly parsing all expected formats.Verification successful
The implementation of
parseIso8601Durationiniso8601.tsshows that the function is designed to parse ISO 8601 durations into seconds. The regex used in the function is capable of matching durations that include days (\d+D), hours (\d+H), minutes (\d+M), and seconds (\d+S). The function also correctly accounts for the possibility of any of these components being absent, except for the period designatorT, which is required if any time elements are present.The function handles optional days, hours, minutes, and seconds, and it defaults to 0 if any of these are not present. It then calculates the total duration in seconds based on the parsed values.
Given the implementation details, it appears that the function should correctly parse all expected ISO 8601 duration formats, including those with only minutes and seconds, which were specifically mentioned in the review comment.
Based on the provided implementation, the update to use
parseIso8601Durationfor parsing video lengths inyoutube.tsis verified to be correctly handling the expected ISO 8601 duration formats.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that parseIso8601Duration is correctly parsing all expected formats. rg --type js "parseIso8601Duration" --files-with-matches | xargs rg "PT\\d+M\\d+S"Length of output: 2868
Script:
#!/bin/bash # View the implementation of parseIso8601Duration to verify handling of various ISO 8601 duration formats. cat server/services/parsing/iso8601.tsLength of output: 733
/**
* Unit tests for DashVideoAdapter's MPD parsing methods
*/
import { DashVideoAdapter } from './dash';
import { Video } from '../../common/models/video';
import { UnsupportedVideoType } from '../exceptions';
import { parseIso8601Duration } from './parsing/iso8601';
import axios from 'axios';
import { DashMPD } from '@liveinstantly/dash-mpd-parser';
jest.mock('axios');
jest.mock('./parsing/iso8601');
describe('DashVideoAdapter', () => {
let adapter: DashVideoAdapter;
let mockAxiosGet: jest.Mock;
let mockParseIso8601Duration: jest.Mock;
let mockDashMPD: jest.Mock;
beforeEach(() => {
adapter = new DashVideoAdapter();
mockAxiosGet = axios.get as jest.Mock;
mockParseIso8601Duration = parseIso8601Duration as jest.Mock;
mockDashMPD = DashMPD as jest.Mock;
});
describe('handleMpd', () => {
it('should fetch and parse MPD data', async () => {
const mockMPDData = '<MPD></MPD>';
const mockManifest = { MPD: {} };
mockAxiosGet.mockResolvedValue({ data: mockMPDData });
const mockParse = jest.fn();
mockDashMPD.mockImplementation(() => ({ parse: mockParse, getJSON: () => mockManifest }));
const url = new URL('http://example.com/video.mpd');
await adapter.handleMpd(url);
expect(mockAxiosGet).toHaveBeenCalledWith(url.href);
expect(mockParse).toHaveBeenCalledWith(mockMPDData);
});
});
describe('parseMpdManifest', () => {
it('should throw UnsupportedVideoType for live streams', () => {
const manifest = { MPD: { '@profiles': 'isoff-live' } };
const url = new URL('http://example.com/video.mpd');
expect(() => {
adapter.parseMpdManifest(url, manifest);
}).toThrow(UnsupportedVideoType);
});
it('should parse duration and return a Video object', () => {
const durationRaw = 'PT1H2M3S';
const durationInSeconds = 3723;
mockParseIso8601Duration.mockReturnValue(durationInSeconds);
const manifest = { MPD: { '@mediaPresentationDuration': durationRaw } };
const url = new URL('http://example.com/video.mpd');
const video = adapter.parseMpdManifest(url, manifest);
expect(mockParseIso8601Duration).toHaveBeenCalledWith(durationRaw);
expect(video).toBeInstanceOf(Video);
expect(video.length).toEqual(durationInSeconds);
});
});
// Additional tests can be added here for other methods like extractTitle, etc.
});@dy3c, I've generated a basic structure for unit tests for the MPD parsing functionality in Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Let's find the file 'dash.ts' to understand the structure and contents related to mpd parsing.
fd dash.ts --exec cat {}
Length of output: 3522 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- server/services/dash.ts (1 hunks)
- server/tests/unit/services/dash.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/services/dash.ts
Additional comments: 2
server/tests/unit/services/dash.spec.ts (2)
47-54: The test case "should throw UnsupportedVideoType for live streams" is well-structured and correctly tests the expected behavior when live stream manifests are encountered. It's important to ensure that all possible
@profilesvalues that indicate a live stream are covered by this test.70-82: The test case "should extract a title from ProgramInformation" is concise and tests the specific scenario well. Ensure that the test covers edge cases, such as missing
ProgramInformationorTitle, to verify that the method handles such scenarios gracefully.
closes #1036