-
Notifications
You must be signed in to change notification settings - Fork 0
Add Listen Mode sample and update samples.json with new entry #147
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 Listen Mode sample and update samples.json with new entry #147
Conversation
Summary of ChangesHello @sebastianpiq, 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 sample application that showcases a 'Listen Mode' for alternative media presentations within a DASH stream. This mode allows for flexible insertion of alternative content into a live stream, initially without a fixed duration. The key innovation is the ability to later send a status update to the manifest, dynamically adding a 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 introduces a 'Listen Mode' sample for Alternative Media Presentations, which is a valuable addition. The implementation effectively uses a manifest response interceptor to dynamically manage events. My review includes suggestions for improving robustness and maintainability, such as adding input validation for user-provided values, ensuring unique event IDs, removing unused code, and refactoring duplicated logic for event creation. These changes will make the sample more resilient and easier to maintain.
| function loadPlayer() { | ||
| const manifestUrl = document.getElementById('manifestUrl').value; | ||
| const alternativeUrl = document.getElementById('alternativeUrl').value; | ||
| const timeOffset = parseInt(document.getElementById('timeOffset').value); |
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.
The value from the timeOffset input is parsed with parseInt without validation. If a user enters non-numeric text (e.g., 'abc'), parseInt will return NaN. This NaN value is then used to calculate presentationTime, which also becomes NaN, causing the event logic to fail. You should validate the parsed value to ensure it's a number and handle the invalid input case gracefully.
| const timeOffset = parseInt(document.getElementById('timeOffset').value); | |
| const timeOffset = parseInt(document.getElementById('timeOffset').value, 10); | |
| if (isNaN(timeOffset)) { | |
| showStatus('Invalid Time Offset. Please enter a valid number.', 'danger'); | |
| return; | |
| } |
samples/alternative/listen-mode.html
Outdated
| let eventId = 0; | ||
| const DEFAULT_DURATION = 10000; // 10 seconds | ||
| const DEFAULT_RETURN_OFFSET = 10000; // 10 seconds | ||
| const DEFAULT_MAX_DURATION = 10000; // 10 seconds |
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.
| statusDiv.style.display = 'block'; | ||
| } | ||
|
|
||
| function updateManifestViewer() { |
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.
The logic for creating the alternative MPD event is duplicated in updateManifestViewer() (which builds an XML string for display) and addManifestResponseInterceptor() (which builds DOM elements to inject into the manifest). This can lead to inconsistencies if one is updated and the other is not.
Consider refactoring this into a single function that creates the event structure as DOM elements. addManifestResponseInterceptor can then directly use these elements, and updateManifestViewer can serialize and highlight them. This would centralize the event creation logic and improve maintainability.
samples/alternative/listen-mode.html
Outdated
| showStatus(`Status update sent. Main content will return soon.`, 'warning'); | ||
| } | ||
|
|
||
| function addManifestResponseInterceptor(manifestUrl) { |
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.
The manifestUrl parameter is passed to the addManifestResponseInterceptor function but it is not used within the function. It can be removed from the function signature and from the call at line 343 to simplify the code.
| function addManifestResponseInterceptor(manifestUrl) { | |
| function addManifestResponseInterceptor() { |
| const presentationTime = Date.now() + (timeOffset * 1000); | ||
|
|
||
| replaceEvent = { | ||
| id: eventId, |
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.
The eventId is always assigned the value 0. If the player is reloaded by clicking the button again, a new event will be created with the same ID. While this might work in this specific sample because the old event stream is removed before adding a new one, it's better practice to ensure unique event IDs per the DASH-IF specification. Consider incrementing eventId after each use to guarantee uniqueness.
| id: eventId, | |
| id: eventId++, |
36d6359
into
feature/alternative-media-presentations
No description provided.