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.

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

Allow consuming stream data from a client app #372

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ferjm
Copy link
Contributor

@ferjm ferjm commented Jul 14, 2020

This is required to implement servo/servo#26861

Tested with 2d and 3D content.

@ferjm ferjm marked this pull request as draft July 14, 2020 13:05
@ferjm ferjm changed the title [WIP] Allow consuming stream data from a client app Allow consuming stream data from a client app Jul 30, 2020
@ferjm ferjm marked this pull request as ready for review July 30, 2020 10:05
@ferjm
Copy link
Contributor Author

ferjm commented Jul 30, 2020

r? Manishearth

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I'm not a fan of this approach, it seems like we're overloading media capture streams with this mode, whereas I'd prefer to not affect that code path.

I don't think push_data should live on the stream either.

What I would imagine we do here is that we have a ClientStreamSource object that contains an appsrc, and you can obtain a stream from it using create_video_from. This is how this API is used elsewhere, GStreamerMediaStream doesn't contain explicit logic for any stream producer, instead it lets you create a stream from an element.

@@ -245,12 +245,20 @@ impl Backend for GStreamerBackend {
media_capture::create_audioinput_stream(set)
}

fn create_videoinput_stream(&self, set: MediaTrackConstraintSet) -> Option<MediaStreamId> {
fn create_videoinput_stream(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be doing this in create_videoinput_stream, that's specifically for screen capture input. We should have a separate create_capture_stream IMO.

self.video_app_source = Some(source.clone());
}

pub fn create_video_from(source: gst::Element, size: Option<Size2D<u32>>) -> MediaStreamId {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be doing decoding within create_video_from, it's the responsibility of the client to do that (see webrtc, all media streams are raw, this was a deliberate choice and without it we will have problems)

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #390) made this pull request unmergeable. Please resolve the merge conflicts.

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

Successfully merging this pull request may close these issues.

None yet

3 participants