-
Notifications
You must be signed in to change notification settings - Fork 206
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
Camera view updated to better respond to state #1437
Conversation
In the previous camera view code, the frame didn't respond well to losses of connection, camera aspect ratio changes, etc. Instead of changing the stream src, we can layer the camera view on top of the loading image, so that any time the camera signal is lost, even if it's because PhotonVision is restarting or you lose connection to the robot, the loading image is still in memory. This also fixes the responsiveness to connection loss.
Not blocking this PR: does this open the door to not trying to load both streams while looking only at one? Right now if you open devtools you can see both streams are being loaded even if only one is visible. |
This PR as is doesn't solve that problem, but if it's something you want to solve, looks like a fairly simple change. |
…into users/stjust/camera-view
Latest commit fixes this. |
No way it was as simple as that, great stuff. I'm out this weekend but will try to draft someone else to try it out |
I can tonight ~5 hrs. |
Looks good to me! |
Put your name on the approval button then :) |
@@ -81,10 +81,10 @@ const performanceRecommendation = computed<string>(() => { | |||
</v-row> | |||
<v-divider style="border-color: white" /> | |||
<v-row class="stream-viewer-container pa-3"> | |||
<v-col v-show="value.includes(0)" class="stream-view"> |
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 should be v-show due to often dynamic rendering. Doing v-if adds more overhead and is incorrect for our use case. Image content can be lazy loaded based on if it is rendered though to avoid the double loading issue.
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.
Maybe use intersection API within photon-camera-stream itself and stop frame request cycle if notInView && backendConnected
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.
In my opinion v-if seems fine given it just cleanly fixes the double load problem.
This space is the root cause of failures in #1437
--- a/photon-client/src/components/app/photon-camera-stream.vue
+++ b/photon-client/src/components/app/photon-camera-stream.vue
@@ -1,5 +1,5 @@
<script setup lang="ts">
-import { computed, inject, ref, onBeforeUnmount } from "vue";
+import { computed, inject, ref, onBeforeUnmount, watchEffect } from "vue";
import { useCameraSettingsStore } from "@/stores/settings/CameraSettingsStore";
import { useStateStore } from "@/stores/StateStore";
import loadingImage from "@/assets/images/loading.svg";
@@ -73,6 +73,20 @@ onBeforeUnmount(() => {
if (!mjpgStream.value) return;
mjpgStream.value["src"] = emptyStreamSrc;
});
+
+watchEffect((onCleanup) => {
+ if (!mjpgStream.value) return;
+
+ const observer = new IntersectionObserver(([entry]) => {
+ mjpgStream.value.src = entry.isIntersecting ? streamSrc.value : emptyStreamSrc;
+ });
+
+ observer.observe(mjpgStream.value);
+
+ onCleanup(() => {
+ observer.disconnect();
+ });
+});
</script>
<template> --- a/photon-client/src/components/dashboard/CamerasCard.vue
+++ b/photon-client/src/components/dashboard/CamerasCard.vue
@@ -81,10 +81,10 @@ const performanceRecommendation = computed<string>(() => {
</v-row>
<v-divider style="border-color: white" />
<v-row class="stream-viewer-container pa-3">
- <v-col v-if="value.includes(0)" class="stream-view">
+ <v-col v-show="value.includes(0)" class="stream-view">
<photon-camera-stream id="input-camera-stream" stream-type="Raw" style="width: 100%; height: auto" />
</v-col>
- <v-col v-if="value.includes(1)" class="stream-view">
+ <v-col v-show="value.includes(1)" class="stream-view">
<photon-camera-stream id="output-camera-stream" stream-type="Processed" style="width: 100%; height: auto" />
</v-col>
</v-row> |
@stephenjust is this what you had in mind? |
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.
I'm happy with or without observability!
Those diffs above do not fix the unnecessary stream loading, because the image is still in the DOM with a valid src value when you use v-show. |
Browsers don't load images with display: none. https://web.dev/articles/browser-level-image-lazy-loading#carousel |
This may be true, but given that these images are mjpg streams, once they start loading, they keep loading until they are removed from the DOM or the image src is changed. So if I use v-show, when I show the second stream it will continue to load the stream even once it's hidden again. Using v-if forces it to be removed from the DOM so the browser knows it can stop loading new frames. |
So should we ship this? I'm good with it, just haven't tested yet |
In the previous camera view code, the frame didn't respond well to losses of connection, camera aspect ratio changes, etc.
Instead of changing the stream src, we can layer the camera view on top of the loading image, so that any time the camera signal is lost, even if it's because PhotonVision is restarting or you lose connection to the robot, the loading image is still in memory. This also fixes the responsiveness to connection loss.
This also only loads a stream if it is meant to be visible in the UI.