Skip to content
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.

Sign up for GitHub

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

RFC: refactor RTCView to better handle track changes #1618

Open
saghul opened this issue Sep 6, 2024 · 5 comments
Open

RFC: refactor RTCView to better handle track changes #1618

saghul opened this issue Sep 6, 2024 · 5 comments
Labels
confirmed The issue has been confirmed enhancement Enhancement of an existing feature

Comments

@saghul
Copy link
Member

saghul commented Sep 6, 2024

This is something I've had on the back of my head for a while.

Right now one renders a view like so:

<RTCView
	mirror={true}
	objectFit={'cover'}
	streamURL={stream.toURL()}
	zOrder={0}
/>

If that stream doesn't have a video track we render back, which is fine, but it doesn't work well in case you add a track later, or in case a track is removed while it was being rendered. Well, yes, we'll render back indeed, which I guess is not great.

What I suggest is we create a wrapper component which takes a stream object rather than a URL, to start with:

<RTCView
	mirror={true}
	objectFit={'cover'}
	srcObject={stream}
	zOrder={0}
/>

While we're here, we might as well call it like the DOM <video> element prop!

So now that we have that, we can check if there is a video track, store it in state and if there is one, get the stream URL and return the native component.

We can then add listeners to internal events that would let us know if a track has been added / removed and act accordingly.

When there is no track we can render null or a fallback view we can get as a prop.

Keeping backwards compatibility should be easy, since we could still take the streamURL prop and if set just skip all logic and return the native component.

Thoughts?

/cc @davidliu
@8BallBomBom 8BallBomBom added the enhancement Enhancement of an existing feature label Sep 6, 2024
@8BallBomBom
Copy link
Member

8BallBomBom commented Sep 6, 2024 •
edited
Loading

Sounds like a much better way to handle things 😃

@davidliu
Copy link
Collaborator

davidliu commented Sep 6, 2024

Yeah that seems pretty reasonable. 👍

@davidliu
Copy link
Collaborator

Copying this comment from the PIP issue since it's slightly related if we wanted to do fallback view on the main RTCVideoView.

Relevant notes:

  • Can't directly pass a React component as a prop so it'd need to be passed in as a child node. Can work around this with with a functional component as in the linked comment.
  • Since PIP view also utilizes this mechanism, will need to organize the views (i.e. first view is main view fallback, second view is PIP view fallback).
  • Android's RTCVideoViewManager will need to switch from a SimpleViewManager to a ViewGroupManager and override the addView methods there to handle the fallback mechanism

@saghul
Copy link
Member Author

saghul commented Sep 10, 2024

Copying this comment from the PIP issue since it's slightly related if we wanted to do fallback view on the main RTCVideoView.

I suspected ;-)

  • Can't directly pass a React component as a prop so it'd need to be passed in as a child node. Can work around this with with a functional component as in the linked comment.

Sounds alright to me.

  • Since PIP view also utilizes this mechanism, will need to organize the views (i.e. first view is main view fallback, second view is PIP view fallback).

Do we need a "normal" view fallback?

  • Android's RTCVideoViewManager will need to switch from a SimpleViewManager to a ViewGroupManager and override the addView methods there to handle the fallback mechanism

I'd skip that for now and focus this PR in iOS. It will already be complex enough...

@davidliu
Copy link
Collaborator

Do we need a "normal" view fallback?

I think it'd probably be fine with just a single fallback to cover both, though I found you can slap a regular view into the PIP just fine, so I'm shunting the fallback view directly in. Would need a separate instance of the fallback view for the main view, or go back to the original plan of doing screen grabs (for now I'll be leaving this as is).

@8BallBomBom 8BallBomBom added the confirmed The issue has been confirmed label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed The issue has been confirmed enhancement Enhancement of an existing feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@davidliu @saghul @8BallBomBom