Dual output recording #4794

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

Jump to bottom

Open

michelinewu wants to merge 47 commits into master

from mw_do_recording_mvp_2

Open

Dual output recording #4794

michelinewu wants to merge 47 commits into master from mw_do_recording_mvp_2

Conversation

michelinewu

Copy link

Contributor

@michelinewu michelinewu commented Nov 17, 2023

No description provided.

michelinewu added 25 commits

November 16, 2023 13:44

@michelinewu


 Vertical display recording in dual output mode with fixed settings.

3ce1a99

@michelinewu


 Move recording to new API.

99b8c61

@michelinewu


 WIP: map simple recording.

69e1f88

@michelinewu


 Add checkbox for vertical recording.

ad25131

@michelinewu


 Migrate simple recording to new API.

69d7d42

@michelinewu


 Fixed simple recording signals. Advanced recording WIP.

111a195

@michelinewu


 Refactor recording signal handling.

968cb32

@michelinewu


 Mapped output settings to advanced recording.

bcaefea

@michelinewu


 Refactor create recording.

edf3ae5

@michelinewu


 WIP: Recordings show in recording history.

84d2831

@michelinewu


 WIP: Recording history shows both recordings.

23d5307

@michelinewu


 Fix show recordings in recording history from cherry pick commit auto...

3acaa29

... merge.

@michelinewu


 Add vertical recording toggle to go live window.

4ba6f6c

@michelinewu


 Allow record vertical as valid dual output destination.

8f374e6

@michelinewu


 Stream horizontal and record vertical.

00ea81f

@michelinewu


 Show both recordings in the recording history.

ebaabed

@michelinewu


 Fix recording file names missing in recording history.

c9cebfb

@michelinewu


 WIP: Recording.

896c3bd

@michelinewu


 Streaming and recording horizontal display while recording vertical l...

1cd98c5

...ogic handled.

@michelinewu


 Fixed dual output stream and record logic.

a9bbb47

@michelinewu


 WIP: hide recording button when dual output dual stream.

600bed3

@michelinewu


 Fix close go live window after start stream in dual output mode.

32942f9

@michelinewu


 Finesse icons, buttons, and confirmation message for vertical recording.

1e47b32

@michelinewu


 WIP: Recording dual output mode test.

738d690

@michelinewu


 Recording tests passing.

cb0ef5c

@bundlemon BundleMon

Copy link

bundlemon bot commented Nov 17, 2023 •

edited

Loading

BundleMon

Files updated (1)

Status	Path	Size	Limits
✅	renderer.(hash).js	6.63MB (+30.74KB +0.45%)	-

Unchanged files (3)

Status	Path	Size	Limits
✅	vendors~renderer.(hash).js	4.65MB	-
✅	updater.js	115.28KB	-
✅	guest-api.js	40.19KB	-

Total files change +30.74KB +0.26%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

michelinewu added 4 commits

November 17, 2023 10:53

@michelinewu


 Fix null error in tests.

c21cb02

@michelinewu


 Fixed call on potentially null object.

db9053e

@michelinewu


 Fix selective recording test.

ac090fb

@michelinewu


 WIP: recording tests.

816bb2c

michelinewu added 17 commits

November 17, 2023 14:28

@michelinewu

WIP

8b36c78

@michelinewu


 Merge branch 'master' into mw_do_recording_mvp_2

3e1ef2f

@michelinewu

WIP

e0a792f

@michelinewu


 Merge branch 'master' into mw_do_recording_mvp_2

19c70aa

@michelinewu


 Always show recording button and add error for attempting to stream p...

b5a1736

...rimary to vertical and record vertical.

@michelinewu


 Fix recording via api test.

777fa64

@michelinewu


 Fix for strictnulls test.

49d6dcf

@michelinewu


 WIP: Dual output recording tests.

d242a9a

@michelinewu


 Fix for reactivity for record vertical toggle.

564fde2

@michelinewu


 Split dual output recording tests.

d43c660

@michelinewu


 WIP: Recording tests.

a0e9f8b

@michelinewu


 Proposed workaround for same settings as stream crash in dual output ...

075d351

...mode.

@michelinewu


 Show single stream chat when vertical is second destination.

be3654e

@michelinewu


 Single output mode uses old API, dual output mode uses new API.

3e97c93

@michelinewu


 Recording tests. WIP: Streaming and Recording Dual Output.

2fd1b8e

@michelinewu


 Fix missing data name in stream options.

870e517

@michelinewu


 Skip test.

65f40f7

blackxored

blackxored requested changes

Dec 5, 2023

View reviewed changes

Copy link

Contributor

@blackxored blackxored left a comment

Choose a reason for hiding this comment

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

Most of the changes requested are cosmetic. The missing translations are probably the most important. The RxJS behavior following after, but it seems to be working correctly, so definitely up to you whether to refactor would provide value.

app/components-react/root/StudioEditor.tsx Outdated Show resolved Hide resolved

app/components-react/root/StudioFooter.tsx

<>REC</>

)}

</span>

<span>{recordingStopping ? <i className="fa fa-spinner fa-pulse" /> : <>REC</>}</span>

Copy link

Contributor

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

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

Nitpick: Should REC be translated?

Copy link

Contributor Author

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

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

This is how it was originally but you bring up a good point. The limitation is that there is only room for three characters on the button. Let's bring it up to the team?

Copy link

Contributor

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

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

yea the character limit is the issue here, as well as i'm not certain other languages have the same shorthand that we do for this term. i think looking into an icon replacement could have value here, as there is a generally accepted icon for recording

app/components-react/windows/go-live/StreamOptions.tsx

const { Option } = Select;

const recordingQualities = [

Copy link

Contributor

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

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

I believe these should be translated.

Copy link

Contributor Author

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

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

Done

app/services/streaming/streaming.ts

// Refactor when migrate to new API, sleep to allow state to update

await new Promise(resolve => setTimeout(resolve, 300));

this.toggleRecording();

signalChanged.unsubscribe();

Copy link

Contributor

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

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

If I remember correctly RxJS didn't play nicely when unsubscribing to a subject inside a subscribe function. I believe this could be achieved with take(1) or takeWhile(condition) and then subscribe which would automatically unsubscribe for us.
Bear in mind is been a while so API might have changed or even this assumption.

this.signalInfoChanged.pipe(
 take(1)
 ).subscribe(...)

I believe the filter (if takeWhile isn't needed after all, it might) and the delay could also be achieved with operators, but that's not a real concern, just the subscribe/unsubscribe behavior is.

Copy link

Contributor Author

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

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

Just for tracking, discussed

app/services/streaming/streaming.ts Outdated Show resolved Hide resolved

app/services/streaming/streaming.ts

if (this.state.verticalStreamingStatus !== EStreamingState.Offline) {

this.SET_VERTICAL_STREAMING_STATUS(EStreamingState.Offline);

}

signalChanged.unsubscribe();

Copy link

Contributor

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

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

RxJS unsubscribe comment earlier.

Copy link

Contributor Author

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

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

Discussed

app/services/streaming/streaming.ts

}

}, 2000),

);

recordingStopped.unsubscribe();

Copy link

Contributor

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

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

RxJS unsubscribe comment earlier.

Copy link

Contributor Author

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

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

Discussed

app/services/streaming/streaming.ts Outdated Show resolved Hide resolved

@michelinewu


 Code review refactors and fixed recording test.

7dbb848

gettinToasty

gettinToasty approved these changes

Dec 5, 2023

View reviewed changes

app/components-react/windows/go-live/StreamOptions.m.less

:global(.ant-form-item-label) {

display: none;

}

Copy link

Contributor

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

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

not worth requiring a change here but just a lil useful thing-- we have a boolean property nowrap on all of our react inputs that gets rid of the label portion

app/components-react/windows/go-live/useGoLiveSettings.ts

}

return true;

}

Copy link

Contributor

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

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

just to follow the logic chain here, users can't stream and record to vertical simultaneously?

@michelinewu michelinewu added the blocked label

May 2, 2024

Labels

blocked

3 participants

@michelinewu @blackxored @gettinToasty

Dual output recording #4794

Are you sure you want to change the base?

Dual output recording #4794

Uh oh!

Conversation

@michelinewu michelinewu commented Nov 17, 2023

Uh oh!

bundlemon bot commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BundleMon

Uh oh!

@blackxored blackxored left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@blackxored blackxored Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@michelinewu michelinewu Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

@gettinToasty gettinToasty Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bundlemon bot commented Nov 17, 2023 •

edited

Loading