Skip to content

Commit

Permalink
Fix infinite loading spinner in MJPEG streaming mode on Chrome (#1306)
Browse files Browse the repository at this point in the history
Resolves tiny-pilot/tinypilot-pro#765.

The issue slipped through via
https://github.com/tiny-pilot/tinypilot/pull/1270/files, specifically
due to the fact that we switched to using one permanent/persistent
`MediaStream` instance for the `video.srcObject` attribute.

An empty `MediaStream` instance (i.e., one that doesn’t contain tracks)
apparently makes Chrome’s loading indicator (on the browser tab) spin
infinitely. We can’t, however, observe this behaviour on Firefox, so it
might be that this is just some weird Chrome-specific glitch that we
need to account for. From a purely technical point of view, this
behaviour doesn’t seem to make immediate sense, since it’s explicitly
allowed for a [`MediaStream` instance to not contain any
tracks](https://developer.mozilla.org/en-US/docs/web/api/mediastream/mediastream).
So it’s not really clear what Chrome is waiting for.

Code-wise, the lazy-initialization is a bit unfortunate, because it’s
less robust and requires us to guard our methods with defensive checks.
I’ve added a prominent comment to document the problem. We might be able
to refactor this later and make the code nicer, but for now this should
do it. @mtlynch I tagged you for review, due to potential urgency – feel
obviously free to defer.

Due to the complexity of the `<remote-screen>` component, [I isolated
the issue via this
branch](https://github.com/tiny-pilot/tinypilot/compare/infinite-spinning-demo?expand=1)
to verify the root cause:


https://user-images.githubusercontent.com/83721279/217886452-863b24e1-6801-447f-9ab0-cc3527105ea9.mov
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1306"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
  • Loading branch information
jotaen4tinypilot authored Feb 9, 2023
1 parent 43eb80c commit 7329336
Showing 1 changed file with 17 additions and 2 deletions.
19 changes: 17 additions & 2 deletions app/templates/custom-elements/remote-screen.html
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,13 @@
image: this.shadowRoot.getElementById("mjpeg-output"),
};

// Initialize the WebRTC media stream.
this.elements.video.srcObject = new MediaStream();
// Note that we can’t assign a persistent `MediaStream` object to the
// `srcObject` attribute that we would retain over the lifecycle of
// this class, because otherwise Chrome shows an infinite loading
// spinner for the browser tab. Therefore, we need to initialize the
// source object lazily whenever we want to access it, and clear it
// when we don’t need it anymore.
this.elements.video.srcObject = null;

this._addScreenEventListeners(this.elements.video);
this._addScreenEventListeners(this.elements.image);
Expand Down Expand Up @@ -347,6 +352,11 @@
*/
async enableWebrtcStreamTrack(mediaStreamTrack) {
const video = this.elements.video;
if (!video.srcObject) {
// Lazy-initialize the media stream. (See comment in
// `connectedCallback`.)
video.srcObject = new MediaStream();
}
const stream = video.srcObject;

// Ensure that the stream doesn't contain multiple tracks of the same
Expand Down Expand Up @@ -401,6 +411,9 @@
*/
disableWebrtcStreamTrack(mediaStreamTrack) {
const video = this.elements.video;
if (!video.srcObject) {
return;
}
const stream = video.srcObject;
stream.removeTrack(mediaStreamTrack);
if (stream.getVideoTracks().length === 0) {
Expand Down Expand Up @@ -488,6 +501,8 @@
}
this.elements.image.src = "/stream?advance_headers=1";
this.webrtcEnabled = false;
// Clean up the media stream. (See comment in `connectedCallback`.)
this.elements.video.srcObject = null;
this.dispatchEvent(new VideoStreamingModeChangedEvent("MJPEG"));
}

Expand Down

0 comments on commit 7329336

Please sign in to comment.