Feat: When hovering over an event's thumbs, play the event using HLS.#4839
Feat: When hovering over an event's thumbs, play the event using HLS.#4839IgorA100 wants to merge 18 commits into
Conversation
… there is an index.m3u8 file. (events.php)
|
@connortechnology |
There was a problem hiding this comment.
Pull request overview
This PR enhances thumbnail hover previews for recorded events by attempting HLS playback first (when an index.m3u8 manifest is available), with fallbacks to MP4 and then MJPEG, while also displaying the active player type/status during playback.
Changes:
- Add HLS-based playback on event thumbnail hover with fallback logic (HLS → MP4 (short events) → MJPEG) and status display updates.
- Extend event list AJAX responses to include HLS source URLs and event duration metadata.
- Update
Event::getStreamSrc()to routemode=mp4hlsto a dedicatedview_hlsendpoint.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| web/skins/classic/js/skin.js | Adds hover playback orchestration for HLS/MP4/MJPEG and injects player-status info into the overlay UI. |
| web/includes/Event.php | Adjusts stream URL generation to support mp4hls via view_hls. |
| web/ajax/events.php | Adds data-video-hls-src and duration attributes for thumbnails, enabling HLS-first hover playback decisions. |
Comments suppressed due to low confidence (3)
web/skins/classic/js/skin.js:1506
getCookie('zmWatchMuted')is a string, so!!getCookie(...)will incorrectly mute when the cookie is'false'. This breaks the user setting for RTSP2Web thumbnails; use an explicit string comparison instead of boolean coercion.
const video = document.createElement('video');
video.removeAttribute('controls');
video.style.cssText = 'width: 100%; height: 100%;';
video.autoplay = true;
video.muted = !!getCookie('zmWatchMuted');
video.playsInline = true;
web/skins/classic/js/skin.js:1608
- The HLS hover player sets
video.muted = !!getCookie('zmWatchMuted'), but the cookie value is'true'/'false'(string).!!'false'is true, so hover playback will always be muted whenever the cookie exists. Use an explicit comparison to'true'or preserve the prior!== 'false'behavior.
video.style.cssText = 'width: 100%; height: 100%;';
video.autoplay = false;
video.muted = !!getCookie('zmWatchMuted');
video.playsInline = true;
container.appendChild(video);
web/skins/classic/js/skin.js:1692
- The MP4 fallback player uses
video.muted = !!getCookie('zmWatchMuted'), which treats the cookie string'false'as truthy and forces muting. Use explicit string-to-boolean handling for this cookie value.
const video = document.createElement('video');
const previewRate = getPreviewRate();
video.src = src;
video.autoplay = true;
video.muted = !!getCookie('zmWatchMuted');
video.playsInline = true;
video.playbackRate = previewRate;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (file_exists($event->Path() . '/index.m3u8')) | ||
| $videoAttr .= ' data-video-hls-src="'.$event->getStreamSrc(array('mode'=>'mp4hls'), '&').'"'; | ||
| $videoAttr .= ' data-video-duration-secs="'.$event->EndDateTimeSecs() - $event->StartDateTimeSecs().'"'; |
There was a problem hiding this comment.
e.g., rely on DefaultVideo() === 'index.m3u8'
This isn't the best option, as it only works for incomplete events. For completed events, DefaultVideo() will always contain the video file name, not the m3u8 manifest.
cache results
PHP automatically caches the file_exists() result. I'm not sure I need to create a new, custom caching system.
or carry an HLS-available flag from the DB/indexer).
I don't know how to do this. It seems to me that this information isn't stored anywhere in the DB/indexer.
There was a problem hiding this comment.
Alternatively, we can always (without checking for the file's existence) provide a link to 'index.m3u8', but in this case, \views\view_hls.php will generate a 404 page and this error will be displayed in the browser console. I don't know how critical this is, but I don't like any errors, especially in large quantities.
There was a problem hiding this comment.
Maybe in \views\view_hls.php we should send a 204 status instead of a 404 if the 'index.m3u8' file is missing?
There was a problem hiding this comment.
DefaultVideo exists precisely because of this hitting the fs is slow thing. I think the c++ side should be changed so that DefaultVideo is always the m3u8.
OR as copilot says, we add another db column to say whether the m3u8 exists. but the m3u8 should always exist so I think that is redundant.
There was a problem hiding this comment.
but then every event will have it set to index.m3u8. Redundant.
There was a problem hiding this comment.
but the m3u8 should always exist
I'm having trouble generating m3u8 files on two servers.
I haven't figured out why yet.
I think the c++ side should be changed so that DefaultVideo is always the m3u8.
Probably not always, but only if m3u8 actually exists.
I have a solution in mind:
-
\views\view_hls.phpwill return a 204 status code instead of 404. This will at least avoid errors in both the browser console and PHP logs.
The absence of the m3u8 manifest file, I think, shouldn't generate a global 404 error.
I think the [204 No Content] status would be more accurate. -
web/ajax/events.phpwill always generate a link to"index.m3u8", which will avoid loading the file system. -
In JS, I'll use
XMLHttpRequestto check the response status. If it's not "200", then there's a problem with"index.m3u8"and we'll switch to the fallback.
I'll make a commit soon.
There was a problem hiding this comment.
I made new commits according to the algorithm described above.
No issues were found.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
web/skins/classic/js/skin.js:1522
Hls.Events.ERRORcallbacks receive(event, data), but this handler only declares a single parameter and logs it; that will log the event name and lose the usefuldatapayload (type/details/fatal), making debugging harder. Update the handler signature to accept both parameters and logdata(or at leastdata.details/type/fatal).
if (Hls.isSupported()) {
const hls = new Hls();
hls.loadSource(hlsUrl);
hls.attachMedia(video);
hls.on(Hls.Events.ERROR, function(e) {
console.error(e);
hls.destroy();
video.remove();
fallbackToMjpeg();
});
…valid, the return status code is 204 instead of 404. (view_hls.php) This will avoid errors in the browser console and PHP logs.
….m3u8" manifest file, even if the physical file is missing. (events.php) This will reduce the load on the file system.
…server response status and load the manifest only if the response status is 200. (skin.js) This will avoid unnecessary errors.
…atusBar. (skin.js) Also: When executing createVideoElement(), autoplay is always false, and after the video is created, we now start playback via thumbnailVideoPlay().
|
Ready for re-checking |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
web/skins/classic/js/skin.js:1523
hls.on(Hls.Events.ERROR, function(e) { ... })only receives the event name string; the Hls.js ERROR callback is(event, data). As written,console.error(e)won’t print the actual error details, reducing the usefulness of the new logging. Accept both params and logdata(and optionallydata.type/details/fatal) before falling back.
if (Hls.isSupported()) {
const hls = new Hls();
hls.loadSource(hlsUrl);
hls.attachMedia(video);
hls.on(Hls.Events.ERROR, function(e) {
console.error(e);
hls.destroy();
video.remove();
fallbackToMjpeg();
});
| } | ||
|
|
||
| $videoAttr .= ' data-video-hls-src="'.$event->getStreamSrc(array('mode'=>'mp4hls'), '&').'"'; | ||
| $videoAttr .= ' data-video-duration-secs="'.$event->EndDateTimeSecs() - $event->StartDateTimeSecs().'"'; |
There was a problem hiding this comment.
Logically, Copilot is correct, but practically everything works without problems.
For some reason, EndDateTimeSecs() always returns the correct time. Apparently, for an unfinished event, the current time is returned. I couldn't quickly figure out why this was happening in \includes\Event.php.
@connortechnology Maybe you can say something?
There was a problem hiding this comment.
It depends on the SQL used to populate the Event object. In some places is uses NOW(), in others it uses a subquery on Frames to determine length. I think the latter is the way to go generally. If we use now and the event in incomplete due to crash, it might be years old. I think Ideally we would use event->Duration() and have code in Duration() that figures it out.
…tting to the page body, save the message in the logs as Warning, since we're passing status code 204. (view_hls.php) If the manifest is m3u8, then we don't write anything in the logs; we simply pass status code 204.
| video.play().then(() => { | ||
| if (infoStatusBar && currentMode) infoStatusBar.innerHTML = ' [' + currentMode + '] '; | ||
| console.debug(currentMode + " video player started playing"); | ||
| if (eventStart && statusBar) updateTimeWallClock(video, eventStart, statusBar); | ||
| }) | ||
| .catch((er) => { | ||
| if (er.name === 'NotAllowedError' && !video.muted) { | ||
| video.muted = true; | ||
| video.play().then(() => { | ||
| if (infoStatusBar && currentMode) infoStatusBar.innerHTML = ' [' + currentMode + '] '; | ||
| console.debug(currentMode + " video player started playing after muting"); | ||
| if (eventStart && statusBar) updateTimeWallClock(video, eventStart, statusBar); | ||
| }) |
…e video. (skin.js) - Pass the eventStart and statusBar parameters to createRtsp2webStream() - Don't call playEventHLS() for Frames pages
video.autoplay = falseinstead oftrue, and then usevideo.play().then(_ => {. This will allow for more correct handling of promises and a repeat playback attempt if muted=false is an issue.