Play media streams #227

ferjm · 2019-03-29T09:26:52Z

No description provided.

backends/gstreamer/src/media_stream_source.rs

Manishearth · 2019-03-29T20:48:36Z

backends/gstreamer/src/media_stream_source.rs

+            gst_log!(self.cat, "Setting stream");
+
+            // Append a proxysink to the media stream pipeline.
+            let pipeline = stream.pipeline_or_new();


Can we just use attach_to_pipeline here? Is there a reason there needs to be a proxysrc/sink? The GstreamerMediaStream may already arise from a proxysrc element.

What's the benefit of using attach_to_pipeline over the current approach?

In any case, I am not sure if this is possible. The GstPlayer pipeline's (playbin) source is read-only, so I am not sure if I can modify the pipeline directly. I can try if you think it is a better option though.

What's the benefit of using attach_to_pipeline over the current approach?

It avoids an extra proxy.

You don't add it to the playbin's pipeline, you add it to ServoMediaStreamSrc::constructed. May need a new version of attach_to_pipeline for attach_to_bin.

Right now the code adds a proxysrc to ServoMediaStreamSrcs bin, instead we should attach the entire MediaStream pipeline to it.

This doesn't have to be done in this PR, and I can try to do it later if you want.

I guess setting up the ghost pads is trickier if you want to do this, since you don't have the streams during construction. Hmm.

Ok, I see.

I think that should be fine as long as we set the ghost pad before setting the player's uri.

Oh, as in we just don't connect the ghost pad to anything? That could work, I'm not sure if ghost pads can do that.

We need to connect the ghost pad to something and I thought that that something could be a fakesrc or such. But it does not seem to work :\

Yeah. ANyway, we don't have to solve this yet.

Manishearth · 2019-03-29T21:48:24Z

I can verify that the audio reaches the other end of the proxysink. Unsure where it's getting dropped.

bors-servo · 2019-04-01T06:29:06Z

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

ferjm · 2019-04-01T14:27:38Z

I can hear sound now with the current patch.

This is not finished yet though. We need to share the mediastream with multiple consumers at the same time (i.e. player, webrtc), so I need to make the mediastream pipeline include a tee so we can have one src pad per consumer.

philn · 2019-04-01T14:38:16Z

backends/gstreamer/src/player.rs

+                        .dynamic_cast::<gst::Pipeline>()
+                        .unwrap();
+                    let clock = gst::SystemClock::obtain();
+                    playbin.set_base_time(clock.get_time());


This base time might not be exactly the same as the one set on the mediastream pipeline. You'd need to share the value, somehow.

I think we'll probably try to set basetime zero everywhere to be consistent?

Eventually all of the pipelines here should be able to talk to each other through streams.

Manishearth · 2019-04-02T05:21:10Z

Is it possible to have multiple proxysrcs associated with a single proxysink? If so we won't need the tee element, instead we can unify all mediastreams as a single proxysrc element.

(This is particularly nice for the webrtc case, where we have a proxysrc element already)

Of course, for non-gst streams we will have to use an appsrc-proxysrc pipeline, but we don't have any of those yet.

ferjm · 2019-04-02T05:40:15Z

Is it possible to have multiple proxysrcs associated with a single proxysink? If so we won't need the tee element, instead we can unify all mediastreams as a single proxysrc element.

That would be nice, and I also thought about this, but @philn told me that proxysrcs have a 1:1 relation with proxysinks, so AFAICT we still need the tee if we want to share streams between different pipelines.


        Play media streams


        Share base time


        End play_media_stream example


        Flatten backends and player directory structures


        rustfmt

ferjm · 2019-04-02T08:39:05Z

Unfortunately I wasn't able to successfully run the webrtc example yet, so I'm going to leave the part to allow multiple consumers of a media stream for a follow-up.

r? @Manishearth

Manishearth

I'd already basically reviewed this when debugging it, r+

Manishearth · 2019-04-02T08:56:47Z

backends/gstreamer/media_stream.rs

            pipeline.set_start_time(gst::ClockTime::none());
-            pipeline.set_base_time(gst::ClockTime::from_nseconds(0));
-            pipeline.use_clock(Some(&gst::SystemClock::obtain()));
+            pipeline.set_base_time(*BACKEND_BASE_TIME);


Is this change necessary? As I understand it the base times just need to match, they don't need to be the actual time things started.

To be clear: I don't want you to undo this change, I think it's a good step forward to use some semblance of a real base time, I'm more curious if BACKEND_BASE_TIME is really necessary (or if we can get away with using 0 everywhere)

I'll defer the answer to @philn :)

The base time can remain at 0 only if both pipelines start playback at the same time, which is not the case here, AFAIU? This is mentioned in the proxy docs (example): https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-plugins/html/gst-plugins-bad-plugins-proxysrc.html

Manishearth · 2019-04-02T08:57:04Z

I'll look into getting this working with the webrtc example.

Manishearth · 2019-04-02T08:58:46Z

If you want multiple media stream consumers, perhaps try it with video + 2x autovideosink?

ferjm · 2019-04-02T13:29:17Z

@bors-servo r=Manishearth

bors-servo · 2019-04-02T13:29:19Z

📌 Commit 9a963a1 has been approved by Manishearth

bors-servo · 2019-04-02T13:29:26Z

⌛ Testing commit 9a963a1 with merge 3fb362c...


        Auto merge of #227 - ferjm:media.streams, r=Manishearth

Play media streams

bors-servo · 2019-04-02T13:41:47Z

☀️ Test successful - checks-travis
Approved by: Manishearth
Pushing 3fb362c to master...

Manishearth reviewed Mar 29, 2019

View changes

backends/gstreamer/src/media_stream_source.rs Outdated Show resolved Hide resolved

ferjm force-pushed the ferjm:media.streams branch 2 times, most recently from 02cb4d0 to acda3e4 Mar 29, 2019

Manishearth reviewed Mar 29, 2019

View changes

Manishearth mentioned this pull request Mar 31, 2019

Flatten directory structures #229

Merged

ferjm force-pushed the ferjm:media.streams branch from aac752a to c3d14b7 Apr 1, 2019

philn reviewed Apr 1, 2019

View changes

ferjm added 2 commits Mar 29, 2019

Play media streams

6bb635f

Share base time

Loading status checks…

8780452

ferjm force-pushed the ferjm:media.streams branch from c3d14b7 to 8780452 Apr 2, 2019

ferjm added 3 commits Apr 2, 2019

End play_media_stream example

9a1c19a

Flatten backends and player directory structures

7d18094

rustfmt

Loading status checks…

9a963a1

ferjm changed the title ~~[WIP] Play media streams~~ Play media streams Apr 2, 2019

Manishearth approved these changes Apr 2, 2019

View changes

bors-servo merged commit 9a963a1 into servo:master Apr 2, 2019
2 checks passed

2 checks passed

Travis CI - Pull Request Build Passed
Details

homu Test successful
Details

ferjm deleted the ferjm:media.streams branch Apr 2, 2019

ferjm mentioned this pull request Apr 3, 2019

Support playback of streams with multiple tracks #234

Open

servo / media

Play media streams #227

Play media streams #227

ferjm commented Mar 29, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 29, 2019

bors-servo commented Apr 1, 2019

ferjm commented Apr 1, 2019

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 2, 2019

ferjm commented Apr 2, 2019

ferjm commented Apr 2, 2019

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 2, 2019 •

edited

Manishearth commented Apr 2, 2019

ferjm commented Apr 2, 2019

bors-servo commented Apr 2, 2019

bors-servo commented Apr 2, 2019

bors-servo commented Apr 2, 2019

servo / media

Join GitHub today

Play media streams #227

Play media streams #227

Conversation

ferjm commented Mar 29, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 29, 2019

bors-servo commented Apr 1, 2019

ferjm commented Apr 1, 2019

This comment has been minimized.

This comment has been minimized.

Manishearth Apr 2, 2019 • edited

Manishearth commented Apr 2, 2019

ferjm commented Apr 2, 2019

ferjm commented Apr 2, 2019

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Apr 2, 2019 • edited

Manishearth commented Apr 2, 2019

ferjm commented Apr 2, 2019

bors-servo commented Apr 2, 2019

bors-servo commented Apr 2, 2019

bors-servo commented Apr 2, 2019

Manishearth Apr 2, 2019 •

edited

Manishearth commented Apr 2, 2019 •

edited