Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Decouple UI from the frontend library #110

Merged
merged 88 commits into from
Feb 22, 2023

Conversation

hmuurine
Copy link
Collaborator

@hmuurine hmuurine commented Feb 20, 2023

Summary

This PR splits the Frontend/library code into two parts:

  1. library that provides an API for establishing Pixel Streaming sessions
  2. ui-library that contains all the UI components like settings overlay panel, connection strength indicator, buttons, etc.

The library code is intended to be used as a library through the Javascript/Typescript API, and it can be used programmatically without the default UI implemented in ui-library. This allows the developers to bring their own UI if they wish to customize the user experience, or even start a Pixel Streaming session without any overlay UI.

The wish is to keep the library API stable and try to not make breaking changes if possible. If new non-breaking features are introduced to the API, it would be great if it was reflected in the version numbering following semantic versioning. Breaking changes should increase the major version number, while non-breaking changes increase only the minor or patch version number.

Sample UI applications

There are now two sample UI applications in Frontend/impelementations/EpicGames:

  1. player.html / player.ts that displays the default UI using library and ui-library together
    Screenshot 2023-02-20 at 18 26 16
  2. uiless.html / uiless.ts that uses only library to start a plain Pixel Streaming session without any additional UI components
    Screenshot 2023-02-20 at 18 26 32

The two sample applications are meant to demonstrate how the library can be used through the JS API either with or without the default UI overlays.

Public API

The Pixel Streaming library can now be configured and used through its public JS/TS API. Here's an example demonstrating the API usage without the default UI:

// Initial configuration with auto-connect and auto-play enabled:
const config = new Config({
	initialSettings: {
		AutoPlayVideo: true,
		AutoConnect: true,
		ss: "ws://localhost:80",
		StartVideoMuted: true,
	}
});
// Initialize the PixelStreaming library, providing a parent div where the user interaction event handlers
// and the video element are attached to:
const pixelStreaming = new PixelStreaming(config, { videoElementParent: document.getElementById("videoParentElement")});

// If browser denies auto-play, the library emits a `playStreamRejected` event. Register a click-to-play
// handler that tries to start the video in user mouse click context, which should not be denied by browser:
pixelStreaming.addEventListener("playStreamRejected", () => {
	const parent = document.getElementById("videoParentElement");
	parent.onclick = () => {
		pixelStreaming.play();
		parent.onclick = undefined;
	}
});

This demonstrates a number of the new API features:

  • Setting initial config programmatically, not only through UI clicks or URL parameters
  • Starting a Pixel Streming session without the default UI
  • Registering event handlers that can be used as the basis of a custom UI

The default UI uses this same public API and the events emitted by the library to implement its functionality, so it should be versatile enough to implement similar functionality in a custom UI. The events are JS-only and generic enough to be technology-independent, so it should be possible to implement e.g. a React UI using this same API. A number of events are emitted at various stages of establishing the connection, when detecting errors, when receiving new statistics, when connection quality changes, etc.

Implementation principles

When decoupling the UI code out from the rest of the library I tried to follow these principles:

  • Whenever possible, I tried to move the UI code as-is from library to ui-library without any changes
  • The places where the old code manipulated the UI state directly were replaced with emitting an event
  • The events are Typescript type-safe, so VSCode intellisense offers you only valid types and even recognizes the correct function signature that the event handler requires for each event type
  • The UI library listens to these events and performs UI state manipulation accordingly
  • If the UI needs to modify settings, connect/disconnect/reconnect etc., it will use the public function API provided by the pixelStreaming object. Added the minimal API that fulfilled the needs of the sample UI and nothing extra
  • Trying to use the same coding style as used in the other parts of the repository, like using Typescript classes

The biggest changes that couldn't be performed by simply moving code around are in Config/*.ts. These files were all split into two: library code stores the data in JS variables, whereas ui-library code handles rendering the checkboxes, text fields etc. The UI side updates the state of the UI controls whenever receiving settingsChanged events from the library.

Test plan

Default UI

  • Started Cirrus signaling server
  • Started the Pixel Streaming Unreal Engine demo application
  • Built library, ui-library and implementations/EpicGames and started the sample application
  • Navigated to /player.html
  • Opened settings panel (that was moved to ui-library side) and verified that it still works. Modified a couple of settings including signaling server URL (text field) and auto-play / muted settings (checkboxes / toggles) and verified that the settings were persisted in browser URL
  • Reloaded the page and verified that the persisted URL parameters were loaded on page load. This was demonstrated by auto-connect feature being run immediately on page refresh
  • Verified that the toggle buttons like Restart and Show FPS worked as expected
  • Verified that the Information panel also worked by opening it and seeing that statistics and Latency test functions worked
  • Verified that Fullscreen button worked by clicking it
  • Verified that the user input events were correctly transmitted to the UE application by focusing on the video element and using mouse and keyboard and seeing that things changed at UE application side
Screen.Recording.2023-02-20.at.19.06.44.10mb.mp4

The plain Pixel Streaming UI with no overlays

  • Navigated to /uiless.html in Firefox
  • Verified that it auto-connected to the Pixel Streaming session
  • Verified that there were no default UI elements on the screen
  • Focused on the stream and verified that mouse and keyboard events were transmitted to the UE application
Screen.Recording.2023-02-20.at.19.09.43.mp4
  • Navigated to /uiless.html in Chrome that does not permit autoplay with the default settings
  • Verified that the custom click-to-play handler works by clicking the white background and seeing that the UE application was displayed

Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed all the changes now. I will be doing a QA pass on it too to look for runtime issues today :)

@lukehb
Copy link
Contributor

lukehb commented Feb 21, 2023

Did a few commits to change some styling things. But also working on fixing a bug(s) where MaxBitrate is getting set to 0 even though initial settings from UE sends a non-zero value. First bug was values are sent as bps but treated internally as kbps, so a division needs to happen on receipt. The second is that the setting of this value needs to be split out from the structure containing Min/Max bitrate and FPS.

Basically this bit of code is the problem child I am talking about:

this.config._addOnNumericSettingChangedListener(
            NumericParameters.WebRTCMaxBitrate,
            (newValue: number) => {
                Logger.Log(
                    Logger.GetStackTrace(),
                    '--------  Sending web rtc settings  --------',
                    7
                );
                const webRtcSettings: WebRTCSettings = {
                    FPS: this.config.getNumericSettingValue(
                        NumericParameters.WebRTCFPS
                    ),
                    MinBitrate:
                        this.config.getNumericSettingValue(
                            NumericParameters.WebRTCMinBitrate
                        ) * 1000, /* kbps to bps */
                    MaxBitrate: newValue * 1000 /* kbps to bps */
                };
                this.webRtcController.sendWebRtcSettings(webRtcSettings);
                Logger.Log(
                    Logger.GetStackTrace(),
                    '-------------------------------------------',
                    7
                );
            }
        );

The sendWebRtcSettings method is just a laziness and should be split into three separate methods because setting all three in this way causes the value change to trigger a sent back to UE before it has even been set internally in the frontend.

tldr; I did not get time to fix the bug today, but should tomorrow

@hmuurine
Copy link
Collaborator Author

Added a few commits:

1. Fixed Cirrus' scripts to get this building on my machine. Just had to link the base `library` when building the `ui-library`

Good catch! Seems to have been broken in one of the recent commits where I removed linking from the basic build and build-dev scripts. There's now build-all script that includes linking, so as an alternative could use that.

2. Exposed all of the `EventEmitter` public methods on the `PixelStreaming` to simplify API usage eg. `pixelStreaming.events.addEventListener` => `pixelStreaming.addEventListener`

Nice, the event functions at top level look so much cleaner!

3. Allow settings to take a default onChange listener. This provides a neater solution than what's originally in the [config](https://github.com/EpicGames/PixelStreamingInfrastructure/blob/master/Frontend/library/src/Config/Config.ts#L478) where we manually set the label. These default listeners allow us to update the labels during construction as the listener added [here](https://github.com/EpicGames/PixelStreamingInfrastructure/pull/110/files#diff-169272ef727cdf9ecc521e42dc12b124d9c619259c274aa18e7088de657af935R187) isn't added yet.

Looks good to me!

@hmuurine hmuurine force-pushed the decouple-ui-from-frontend-library branch from 68c3e94 to 353f565 Compare February 21, 2023 14:00
@hmuurine
Copy link
Collaborator Author

(Force-pushed to rewrite the latest commit with a signature.)

@hmuurine
Copy link
Collaborator Author

hmuurine commented Feb 21, 2023

While documenting the EventEmitter events, noticed that webRtcConnected event was never emitted. Added some code that taps into oniceconnectionstatechange event handler and emits webRtcConnected the first time it sees iceConnectionState "connected" or "completed". The change is in commit b2fb702

Needed to check for both states since according to https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/iceconnectionstatechange_event#usage_notes "connected" can be sometimes skipped. And on the other hand we cannot wait for "completed" because it might happen several seconds after the connection has already been established.

I'm not fully happy with the extra local context boolean I had to add there to keep track if the event has been already dispatched, but that was the simplest way to ensure that the event is emitted at most once per startSession() call.

@lukehb
Copy link
Contributor

lukehb commented Feb 22, 2023

While documenting the EventEmitter events, noticed that webRtcConnected event was never emitted. Added some code that taps into oniceconnectionstatechange event handler and emits webRtcConnected the first time it sees iceConnectionState "connected" or "completed". The change is in commit b2fb702

Needed to check for both states since according to https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/iceconnectionstatechange_event#usage_notes "connected" can be sometimes skipped. And on the other hand we cannot wait for "completed" because it might happen several seconds after the connection has already been established.

I'm not fully happy with the extra local context boolean I had to add there to keep track if the event has been already dispatched, but that was the simplest way to ensure that the event is emitted at most once per startSession() call.

Yeah, it is annoying that icestatechange works like that. Anyways, I just reviewed the commit, lgtm!

@lukehb
Copy link
Contributor

lukehb commented Feb 22, 2023

Also @hmuurine thanks for the documentation commits!

@lukehb
Copy link
Contributor

lukehb commented Feb 22, 2023

Did a few commits to change some styling things. But also working on fixing a bug(s) where MaxBitrate is getting set to 0 even though initial settings from UE sends a non-zero value. First bug was values are sent as bps but treated internally as kbps, so a division needs to happen on receipt. The second is that the setting of this value needs to be split out from the structure containing Min/Max bitrate and FPS.

Basically this bit of code is the problem child I am talking about:

this.config._addOnNumericSettingChangedListener(
            NumericParameters.WebRTCMaxBitrate,
            (newValue: number) => {
                Logger.Log(
                    Logger.GetStackTrace(),
                    '--------  Sending web rtc settings  --------',
                    7
                );
                const webRtcSettings: WebRTCSettings = {
                    FPS: this.config.getNumericSettingValue(
                        NumericParameters.WebRTCFPS
                    ),
                    MinBitrate:
                        this.config.getNumericSettingValue(
                            NumericParameters.WebRTCMinBitrate
                        ) * 1000, /* kbps to bps */
                    MaxBitrate: newValue * 1000 /* kbps to bps */
                };
                this.webRtcController.sendWebRtcSettings(webRtcSettings);
                Logger.Log(
                    Logger.GetStackTrace(),
                    '-------------------------------------------',
                    7
                );
            }
        );

The sendWebRtcSettings method is just a laziness and should be split into three separate methods because setting all three in this way causes the value change to trigger a sent back to UE before it has even been set internally in the frontend.

tldr; I did not get time to fix the bug today, but should tomorrow

Okay this is addressed in b912071

Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Reviewed. QA'd. Bug fixed. I'm landing this!

@AntiF0
Copy link

AntiF0 commented Mar 1, 2023

Is there more detailed document of using frontend library package beacuse now i have to browse the source code to find the usage of a function, or if it is existing, where can i find it?
Thanks.

@lukehb
Copy link
Contributor

lukehb commented Mar 1, 2023

@AntiF0 The docs/usage examples are coming soon. We will have these in time for 5.2, but for now we are actively still documenting. You will note the new frontend is only in master (our dev branch) and 5.2 (which is pre-release).

Please be patient, docs are coming.

In the meantime I have two things for you:

  • If you go into Frontend/library you can do npx typedoc src/pixelstreamingfrontend.ts to generate API docs
  • I will open an issue where I encourage you to collaborate with us on the sort of examples you would like to see for the frontend docs.

@hmuurine For vis.

@AntiF0
Copy link

AntiF0 commented Mar 2, 2023

@lukehb Thanks a lot for you and your team's efforts on this project, but I am only a beginner in programming and may not be able to do this.

@lukehb
Copy link
Contributor

lukehb commented Mar 2, 2023

@AntiF0 Understood, we will handle this then. Expect more docs in the next few weeks/months.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Decouple UI and business logic from Frontend library
4 participants