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

WebRTC data channels support #26752

Merged
merged 27 commits into from Jun 30, 2020
Merged

WebRTC data channels support #26752

merged 27 commits into from Jun 30, 2020

Conversation

ferjm
Copy link
Member

@ferjm ferjm commented Jun 2, 2020

This depends on servo/media#350 and contains the basic pieces to make a simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work

@highfive
Copy link

highfive commented Jun 2, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/RTCDataChannel.webidl, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/webidls/RTCError.webidl, components/script/dom/mod.rs, components/script/dom/rtcerrorevent.rs and 7 more
  • @KiChjang: components/script/dom/webidls/RTCDataChannel.webidl, components/script/dom/webidls/RTCPeerConnection.webidl, components/script/dom/webidls/RTCError.webidl, components/script/dom/mod.rs, components/script/dom/rtcerrorevent.rs and 7 more

@highfive
Copy link

highfive commented Jun 2, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@ferjm ferjm changed the title Datachannel Basic WebRTC data channels support Jun 2, 2020
Copy link
Member

@Manishearth Manishearth left a comment

Overall seems good. Some of this will change when the backend uses something less synchronous

"open",
"closing",
"closed"
};
Copy link
Member

@Manishearth Manishearth Jun 3, 2020

Choose a reason for hiding this comment

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

newline at eof

"sdp-syntax-error",
"hardware-encoder-not-available",
"hardware-encoder-error"
};
Copy link
Member

@Manishearth Manishearth Jun 3, 2020

Choose a reason for hiding this comment

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

newline at eof


dictionary RTCErrorEventInit : EventInit {
required RTCError error;
};
Copy link
Member

@Manishearth Manishearth Jun 3, 2020

Choose a reason for hiding this comment

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

newline at eof

};
let error = RTCError::new(&self.global(), &init, message);
let event = RTCErrorEvent::new(&self.global(), atom!("error"), false, false, &error);
event.upcast::<Event>().fire(self.upcast());
Copy link
Member

@Manishearth Manishearth Jun 3, 2020

Choose a reason for hiding this comment

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

this should be setting a realm, yes?

@ferjm ferjm changed the title Basic WebRTC data channels support [WIP] WebRTC data channels support Jun 11, 2020
@ferjm ferjm force-pushed the datachannel branch 2 times, most recently from 02db793 to 7614e78 Compare Jun 12, 2020
@ferjm ferjm changed the title [WIP] WebRTC data channels support WebRTC data channels support Jun 12, 2020
@ferjm
Copy link
Member Author

ferjm commented Jun 12, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this issue Jun 12, 2020
WebRTC data channels support

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26212

This depends on servo/media#350 and contains the basic pieces to make a  simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work
@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2020

Trying commit 7614e78 with merge 2af09dd...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2020

💔 Test failed - status-taskcluster

@ferjm
Copy link
Member Author

ferjm commented Jun 16, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this issue Jun 16, 2020
WebRTC data channels support

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26212

This depends on servo/media#350 and contains the basic pieces to make a  simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

Trying commit 1ecba0a with merge 5204e0a...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

💔 Test failed - status-taskcluster

@ferjm
Copy link
Member Author

ferjm commented Jun 16, 2020

I don't understand why this is failing on Taskcluster. It builds perfectly on my linux machine. The binaries that we use in Taskcluster seems to be the ones installed by ./mach bootstrap-gstreamer, which AFAICT contain the missing symbols TC is complaining about:

Screenshot 2020-06-16 at 13 01 54

@jdm @Manishearth any idea of what I may be missing here?

@ferjm ferjm force-pushed the datachannel branch 2 times, most recently from f7b6b7e to f63d911 Compare Jun 16, 2020
@ferjm
Copy link
Member Author

ferjm commented Jun 16, 2020

CC @sdroege

Let me summarize. AFAICT:

  • We are using the binaries that ./mach bootstrap-gstreamer fetches here
  • These binaries have a libgstwebrtc-1.0.so exporting a gst_webrtc_data_channel_state_get_type symbol, as shown here
  • We are passing -lgstwebrtc-1.0 to the link command
  • But it is still complaining about the undefined reference to gst_webrtc_data_channel_state_get_type

Copy link
Member

@Manishearth Manishearth left a comment

Looks good, some remaining comments (and there's still a realm comment that's not addressed).

r=me once you figure out the taskcluster stuff

Some(channel_id),
);

self.register_data_channel(channel_id, &channel);
Copy link
Member

@Manishearth Manishearth Jun 16, 2020

Choose a reason for hiding this comment

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

The RTCDC constructor already does this

@@ -177,7 +178,7 @@ def linux_tidy_unit():
.with_script("""
./mach test-tidy --no-progress --all
python3 ./mach test-tidy --no-progress --all --no-wpt
python3 ./mach build --dev
python3 ./mach build --dev -vv
Copy link
Member

@Manishearth Manishearth Jun 16, 2020

Choose a reason for hiding this comment

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

should be removed before landing

@@ -158,7 +158,8 @@ def linux_tidy_unit_untrusted():
./mach test-tidy --no-progress --all
./mach test-tidy --no-progress --self-test
./mach bootstrap-gstreamer
./mach build --dev
nm -gD /repo/support/linux/gstreamer/gst/lib/libgstwebrtc-1.0.so
./mach build --dev -vv
Copy link
Member

@Manishearth Manishearth Jun 16, 2020

Choose a reason for hiding this comment

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

should be removed before landing

bors-servo added a commit that referenced this issue Jun 30, 2020
WebRTC data channels support

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26212

This depends on servo/media#350 and contains the basic pieces to make a  simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

💔 Test failed - status-taskcluster

@ferjm
Copy link
Member Author

ferjm commented Jun 30, 2020

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

📌 Commit c2968fa has been approved by Manishearth

@jdm
Copy link
Member

jdm commented Jun 30, 2020

@bors-servo try=linux

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Trying commit c2968fa with merge c727a4e...

bors-servo added a commit that referenced this issue Jun 30, 2020
WebRTC data channels support

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26212

This depends on servo/media#350 and contains the basic pieces to make a  simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

☀️ Test successful - status-taskcluster
State: approved=Manishearth try=True

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit c2968fa with merge 4ac2dfe...

bors-servo added a commit that referenced this issue Jun 30, 2020
WebRTC data channels support

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #26212

This depends on servo/media#350 and contains the basic pieces to make a  simple test like https://ferjm.github.io/samples/src/content/datachannel/basic/ work
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Jun 30, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit c2968fa with merge 4b034ed...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 4b034ed to master...

@bors-servo bors-servo merged commit 4b034ed into servo:master Jun 30, 2020
2 checks passed
@ferjm ferjm deleted the datachannel branch Jun 30, 2020
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.

5 participants