-
Notifications
You must be signed in to change notification settings - Fork 0
add status update functional test #145
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
add status update functional test #145
Conversation
Summary of ChangesHello @cotid-qualabs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new functional test to validate the behavior of Alternative MPD events with Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a new functional test for the alternative MPD status update feature in live streams. The test logic is sound, but there are a few areas for improvement in the implementation to enhance maintainability and clarity. I've provided specific suggestions to address these points.
|
|
||
| // Create the replace event WITHOUT maxDuration initially | ||
| const replaceEvent = { | ||
| schemeIdUri: 'urn:mpeg:dash:event:alternativeMPD:replace:2025', |
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.
For better maintainability and to avoid magic strings, please use the predefined constants from Constants.js.
- On this line and line 69,
urn:mpeg:dash:event:alternativeMPD:replace:2025should beConstants.ALTERNATIVE_MPD.URIS.REPLACE. - On line 75,
'update'should beConstants.ALTERNATIVE_MPD.STATUS.UPDATE. - On lines 154 and 167,
'replace'should beConstants.ALTERNATIVE_MPD.MODES.REPLACE.
| schemeIdUri: 'urn:mpeg:dash:event:alternativeMPD:replace:2025', | |
| schemeIdUri: Constants.ALTERNATIVE_MPD.URIS.REPLACE, |
| const duration = 15000; // 15 seconds default duration | ||
| const earliestResolutionTimeOffset = 3000; |
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.
| let existingEventStream = manifest.Period[0].EventStream.find( | ||
| stream => stream.schemeIdUri === 'urn:mpeg:dash:event:alternativeMPD:replace:2025' | ||
| ); | ||
|
|
||
| if (existingEventStream) { | ||
| // Add the status update event to the existing EventStream | ||
| existingEventStream.Event.push(statusUpdateEvent.Event[0]); | ||
| } else { | ||
| // Add as a new EventStream (this creates the update scenario) | ||
| manifest.Period[0].EventStream.push(statusUpdateEvent); | ||
| } |
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.
This logic for adding the status update event is a bit confusing. The comment on line 57 (// Keep existing EventStreams...) is misleading because a fresh manifest is retrieved, so existingEventStream will always be undefined in this test's flow. The else block is always executed.
To improve clarity, you can simplify this block to directly add the new event stream, with a comment explaining why this works for an update.
| let existingEventStream = manifest.Period[0].EventStream.find( | |
| stream => stream.schemeIdUri === 'urn:mpeg:dash:event:alternativeMPD:replace:2025' | |
| ); | |
| if (existingEventStream) { | |
| // Add the status update event to the existing EventStream | |
| existingEventStream.Event.push(statusUpdateEvent.Event[0]); | |
| } else { | |
| // Add as a new EventStream (this creates the update scenario) | |
| manifest.Period[0].EventStream.push(statusUpdateEvent); | |
| } | |
| // Add the status update event as a new EventStream. The player will process this | |
| // as an update because the event has status='update' and a matching ID to an active event. | |
| manifest.Period[0].EventStream.push(statusUpdateEvent); |
|
|
||
| // Verify that alternative content terminated early due to maxDuration from status update | ||
| expect(actualAlternativeDuration).to.be.lessThan(10); // Much less than original 15s | ||
| expect(actualAlternativeDuration).to.be.lessThan(12); |
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.
…ve-media-presentations/status-update-functional-test
1d8f5fd
into
feature/alternative-media-presentations
No description provided.