Closed
Bug 711628
Opened 13 years ago
Closed 12 years ago
Implement MediaStreamArray class for PeerConnection's localStreams/remoteStreams
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla19
People
(Reporter: jesup, Assigned: peterv)
References
Details
(Whiteboard: [WebRTC] [blocking-webrtc+])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Per discussion in #content, use a scriptableHelper, and then make MediaStreamArray to provide the localStreams/remoteStreams arrays required by the webidl (which says they have to be 'live').
bholley jesup: I _think_ you can get away with subclassing nsArraySH, rather than nsGenericArraySH, and just implementing GetItemAt
See also https://developer.mozilla.org/en/Mozilla_DOM_Hacking_Guide
In the much longer future, bug 580070 should provide easier ways to do this, but that's much further out.
Reporter | ||
Comment 1•13 years ago
|
||
This is a work-in-progress, and note the naming of nsWebRTCMediaStream.h is to avoid nsMediaStream.h in content; the name will change before checkin (or roc's changes for mediastreams will land and I'll be able to remove my mediastream stub).
However, this should show how I'm planning to implement the "live array return" using nsArraySH, as recommended.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 582905 [details] [diff] [review]
WIP patch to add MediaStreamList's and use them in IDL
I realize this is incomplete and rough, but I figured I'd run it past you while it's still relatively fresh in your minds. Did I miss/forget anything obvious?
(yes, I know I need to vet the license boilerplates - they were just copied largely from the code I Cargo-Culted off of.)
Attachment #582905 -
Flags: feedback?(khuey)
Attachment #582905 -
Flags: feedback?(bobbyholley+bmo)
Attachment #582905 -
Flags: feedback?(Ms2ger)
Comment 3•13 years ago
|
||
Comment on attachment 582905 [details] [diff] [review]
WIP patch to add MediaStreamList's and use them in IDL
This generally looks like the right approach.
Attachment #582905 -
Flags: feedback?(bobbyholley+bmo) → feedback+
Reporter | ||
Updated•13 years ago
|
Attachment #582905 -
Flags: feedback?(khuey)
Attachment #582905 -
Flags: feedback?(Ms2ger)
Assignee | ||
Comment 4•12 years ago
|
||
Boris, could you take a look at the binding stuff?
Assignee: rjesup → peterv
Attachment #582905 -
Attachment is obsolete: true
Attachment #661340 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Reporter | ||
Comment 5•12 years ago
|
||
Note to bz - this probably applies against Alder, as PeerConnection/etc aren't on m-c yet.
Comment 6•12 years ago
|
||
Comment on attachment 661340 [details] [diff] [review]
v1
>+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
>+ return mLocalSourceStreams[index].get();
Why do you need the .get()?
>+ return mRemoteSourceStreams[index].get();
Likewise.
r=me
Attachment #661340 -
Flags: review?(bzbarsky) → review+
Comment 7•12 years ago
|
||
What's left to do here on the bug Peter? Can we finalize the patch and get it landed. It contains some parts we really would like to have for our first Mochitests for WebRTC. Thanks!
Component: Video/Audio → WebRTC
QA Contact: jsmith
Whiteboard: [WebRTC]
Reporter | ||
Updated•12 years ago
|
Whiteboard: [WebRTC] → [WebRTC] [blocking-webrtc+]
Assignee | ||
Comment 8•12 years ago
|
||
Target Milestone: --- → mozilla19
Comment 9•12 years ago
|
||
Push backed out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2dbcd6d16b43
https://hg.mozilla.org/integration/mozilla-inbound/rev/5540b310d435
Target Milestone: mozilla19 → ---
Comment 10•12 years ago
|
||
Well after the backout, we got a build failure relating to one of the files added by this bug's patch:
https://tbpl.mozilla.org/php/getParsedLog.php?id=16047049&tree=Mozilla-Inbound
make[7]: *** No rule to make target `src/peerconnection/MediaStreamList.cpp', needed by `src/peerconnection/MediaStreamList.o'. Stop.
Looks like this requires a clobber to clean up after a backout, or something (?)
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Looks like this requires a clobber to clean up after a backout, or something
I think that was bug 800847.
Relanded with the interface in a MOZ_WEBRTC condition.
http://hg.mozilla.org/integration/mozilla-inbound/rev/bbcf489988a4
Comment 12•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 13•12 years ago
|
||
Validated through automation and a couple of manual sanity checks.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•