The default bug view has changed. See this FAQ.

Write MediaEngine backend for Desktop based on WebRTC code

VERIFIED FIXED in mozilla16

Status

()

Core
WebRTC
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: jesup, Assigned: anant)

Tracking

(Blocks: 3 bugs, {dev-doc-needed})

Trunk
mozilla16
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

(Whiteboard: [qa!], URL)

Attachments

(3 attachments, 9 obsolete attachments)

25.21 KB, patch
roc
: review+
jesup
: review+
Details | Diff | Splinter Review
8.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
21.48 KB, patch
jst
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Implement WebRTC navigator.getUserMedia(), per the moving-target spec (see URL)
(Reporter)

Comment 1

5 years ago
This bug is for the internal implementation - hook up to MediaStreams, etc.  Front-end UI is a separate issue
Depends on: 664918
No longer depends on: 688178
(Reporter)

Updated

5 years ago
Blocks: 729522
(Reporter)

Updated

5 years ago
Blocks: 731429
Component: Video/Audio → WebRTC
QA Contact: video.audio → webrtc-general
(Assignee)

Comment 2

5 years ago
Created attachment 605310 [details] [diff] [review]
Very preliminary take on getUserMedia

This is a very preliminary take on a getUserMedia implementation. Posted mostly as an internal checkpoint, unlikely to be useful to anyone in its current state.
Assignee: nobody → anant
Status: NEW → ASSIGNED
Created attachment 608565 [details] [diff] [review]
patch for still photo on android
Depends on: 738528
Comment on attachment 608565 [details] [diff] [review]
patch for still photo on android

moving this patch to bug 738528
Attachment #608565 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Blocks: 742832

Updated

5 years ago
Blocks: 746388
blocking-kilimanjaro: --- → +
Whiteboard: [k9o:p2:fx17?]
(Assignee)

Updated

5 years ago
Blocks: 749318
No longer depends on: 738528
(Reporter)

Updated

5 years ago
Depends on: 749889
blocking-kilimanjaro: + → ---
Whiteboard: [k9o:p2:fx17?]
No longer blocks: 746388
(Assignee)

Updated

5 years ago
Depends on: 750943
(Assignee)

Comment 5

5 years ago
Repurposing this to focus only on the desktop backend for MediaEngine being described in bug 750943. Injection of DOM API is being done via bug 752353, while UI & MediaStream hookups were already being tracked elsewhere.
Summary: Implement WebRTC navigator.getUserMedia() → Write MediaEngine backend for Desktop based on WebRTC code

Updated

5 years ago
Blocks: 748835

Updated

5 years ago
Assignee: anant → snandaku

Comment 6

5 years ago
Created attachment 628257 [details] [diff] [review]
WebRTC media engine backend patch v1.0

Comment 7

5 years ago
Created attachment 628258 [details]
DOM update from Anant's patch for WebRTC backend

Comment 8

5 years ago
Updated 2 patches 
1. First version of WebRTC backend support for Abstract Media Engine with video capture and audio playback rendered through their respective media-streams
2. DOM GetUserMedia update to support Patch 1

Please note that Patch 1 (WebRTC media engine backend patch v1.0) has Fallback media engine code as well.

Updated

5 years ago
Attachment #605310 - Flags: feedback?(roc)
Attachment #605310 - Flags: feedback?(rjesup)
Attachment #605310 - Flags: feedback?(anant)
(Assignee)

Comment 9

5 years ago
The DOM binding and fallback media engine have their own bugs, and that code has already been reviewed. Can you split off only the WebRTC backend portion of the patch and re-upload to this bug? To make it easy, you can use hg patch queues. You can get started pretty easily like so:

$ git clone git://github.com/anantn/gum.git
$ ln -s gum alder/.hg/patches
$ cd alder && hg qpush -a
$ hg qnew webrtc-backend
... apply your changes for webrtc backend only ...
$ hg qref

Then upload the file alder/.hg/patches/webrtc-backend to this bug.
(Assignee)

Comment 10

5 years ago
Comment on attachment 605310 [details] [diff] [review]
Very preliminary take on getUserMedia

Also, you applied the flags on the wrong patch :) Let's reflag when you have the new attachment for the WebRTC parts only.
Attachment #605310 - Flags: feedback?(roc)
Attachment #605310 - Flags: feedback?(rjesup)
Attachment #605310 - Flags: feedback?(anant)
(Assignee)

Comment 11

5 years ago
(In reply to Anant Narayanan [:anant] from comment #9)
> $ ln -s gum alder/.hg/patches

This should actually be gum/patches instead of gum, I missed the extra directory in there.

Comment 12

5 years ago
(In reply to Anant Narayanan [:anant] from comment #11)
> (In reply to Anant Narayanan [:anant] from comment #9)
> > $ ln -s gum alder/.hg/patches
> 
> This should actually be gum/patches instead of gum, I missed the extra
> directory in there.

ure Anant. Btw I had to introduce Shutdown() function to MediaEngine Interface as well as MediaManager code in DOm bindings ... How do i get those changes reviewed

Comment 13

5 years ago
(In reply to Anant Narayanan [:anant] from comment #11)
> (In reply to Anant Narayanan [:anant] from comment #9)
> > $ ln -s gum alder/.hg/patches
> 
> This should actually be gum/patches instead of gum, I missed the extra
> directory in there.

ure Anant. Btw I had to introduce Shutdown() function to MediaEngine Interface as well as MediaManager code in DOm bindings ... How do i get those changes reviewed
(Assignee)

Comment 14

5 years ago
(In reply to Suhas from comment #13)
> ure Anant. Btw I had to introduce Shutdown() function to MediaEngine
> Interface as well as MediaManager code in DOm bindings ... How do i get
> those changes reviewed

Why can't we put the shutdown code in the MediaEngine destructor?

Comment 15

5 years ago
(In reply to Anant Narayanan [:anant] from comment #14)
> (In reply to Suhas from comment #13)
> > ure Anant. Btw I had to introduce Shutdown() function to MediaEngine
> > Interface as well as MediaManager code in DOm bindings ... How do i get
> > those changes reviewed
> 
> Why can't we put the shutdown code in the MediaEngine destructor?

Right now, the Shutdown() is added to MediaEngineVideoSource and MediaEngineAudioSource interfaces in MediaEngine.h ..

MediaEngine holds pointer to the engine instances and Sources hold ref-counted interface pointers ... We need to release all the ref-counted interface pointers before we delete engine instances held at the MediaEngine ..

The above implies, ordering of the destructor calls .. MediaEngineVideoSource/MediaEngineAudioSource followed by MediaEngine to be free of any memory leaks ...

Hence, Shutdown() in these interfaces , provides the client ( MediaManager.cpp) to cleanup the Webrtc interfaces pointers held in MediaEngineVideo/AudioSource before it destroys the MediaEngine
(Assignee)

Comment 16

5 years ago
(In reply to Suhas from comment #15)
> (In reply to Anant Narayanan [:anant] from comment #14)
> > Why can't we put the shutdown code in the MediaEngine destructor?
> 
> The above implies, ordering of the destructor calls ..
> MediaEngineVideoSource/MediaEngineAudioSource followed by MediaEngine to be
> free of any memory leaks ...
> 
> Hence, Shutdown() in these interfaces , provides the client (
> MediaManager.cpp) to cleanup the Webrtc interfaces pointers held in
> MediaEngineVideo/AudioSource before it destroys the MediaEngine

I'm not fully understanding. Why isn't order maintained when the caller (MediaManager in this case) deletes the sources before the engine, either by releasing its reference to it or explicitly calling delete? The semantics of MediaEngine::Enumerate means the caller is responsible for the returned MediaEngineSources (because the array in which they are populated is allocated by the caller).

Comment 17

5 years ago
(In reply to Anant Narayanan [:anant] from comment #16)
> (In reply to Suhas from comment #15)
> > (In reply to Anant Narayanan [:anant] from comment #14)
> > > Why can't we put the shutdown code in the MediaEngine destructor?
> > 
> > The above implies, ordering of the destructor calls ..
> > MediaEngineVideoSource/MediaEngineAudioSource followed by MediaEngine to be
> > free of any memory leaks ...
> > 
> > Hence, Shutdown() in these interfaces , provides the client (
> > MediaManager.cpp) to cleanup the Webrtc interfaces pointers held in
> > MediaEngineVideo/AudioSource before it destroys the MediaEngine
> 
> I'm not fully understanding. Why isn't order maintained when the caller
> (MediaManager in this case) deletes the sources before the engine, either by
> releasing its reference to it or explicitly calling delete? The semantics of
> MediaEngine::Enumerate means the caller is responsible for the returned
> MediaEngineSources (because the array in which they are populated is
> allocated by the caller).

Agree completely .. I was bit hesitant on someone (say a nsITimer) holding a ref-counted reference to MediaEngineVideo/Audio source which may not render the order of destruction as we wanted .. But your comment on the responsibility of the caller is true .. I will go ahead and make the appropriate changes and re-patch it later today as per your suggestions  .. thx
Keywords: dev-doc-needed

Comment 18

5 years ago
Created attachment 628543 [details]
GUM Webrtc Backend v1

Updated

5 years ago
Attachment #628257 - Attachment is obsolete: true

Updated

5 years ago
Attachment #628258 - Attachment is obsolete: true
Attachment #628258 - Attachment is patch: false

Updated

5 years ago
Attachment #628543 - Attachment description: GUM Webrt Backend v1 → GUM Webrtc Backend v1

Comment 19

5 years ago
Comment on attachment 628543 [details]
GUM Webrtc Backend v1

Split the earlier patch to include only WebRTC Backend GUM updates. This is version 1 of such an implementation
Attachment #628543 - Flags: feedback?(roc)
Attachment #628543 - Flags: feedback?(rjesup)
Attachment #628543 - Flags: feedback?(anant)
Comment on attachment 628543 [details]
GUM Webrtc Backend v1

Review of attachment 628543 [details]:
-----------------------------------------------------------------

I think this probably belongs in its own directory, maybe content/media/webrtc.

The MediaStreams integration looks reasonable at first glance. The main issue is having separate audio and video streams. That won't give us A/V sync.

::: content/media/MediaEngineWebrtc.cpp
@@ +532,5 @@
> +  mSource = aStream;
> +
> +  AudioSegment* segment = new AudioSegment();
> +  segment->Init(CHANNELS);
> +  segment->InsertNullDataAtStart(1);

Why are you inserting silence here?

@@ +631,5 @@
> +
> +  // allocate shared buffer of lenght bytes
> +  int buff_size = AUDIO_SAMPLE_LENGTH * sizeof(short);
> +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(buff_size);
> +  buffer->AddRef();

What's this extra AddRef for?

You can avoid copying here by creating the buffer first and then calling ExternalPlayoutGetData to write directly into the buffer.
(Reporter)

Comment 21

5 years ago
Bug/issue:
Given the following example, the video will be delayed (and chew memory) by however long you take to click on 'ok'.  We can't use elem.currentTime = elem.seekable.end() or some such to skip to the current end with a mediastream src, either (or so it appears).

We need to avoid this problem, and avoid any unintentional pauses before starting the consuming element causing a persistent buffering/delay.

Note that the new 'consuming' state in another bug doesn't currently check if the element is paused/not-playing-yet - another option is to make it check that too.  This might also let us use pause() on a video element showing a live mediastream, and use the listener to pause delivery of frames (buffering/delaying) to the paused element.

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
  <title>gum test</title>
</head>
<body>
  <video id="self" width="100%"></video>
</body>
<script>
  window.navigator.mozGetUserMedia({video:true}, function(stream) {
    console.log(stream);
    var self = document.getElementById("self");
    self.src = stream;
    alert("got stream!");
    self.play();
  }, function(err) {
    alert("error! " + err);
  })
</script>
</html>
You can't seek in a MediaStream, by design.

You can call SourceMediaStream::HaveEnoughBuffered and DispatchWhenNotEnoughBuffered to throttle production of video and audio data. That's definitely worth doing.

That will leave you with the problem of having a small amount of "old" buffered data hanging around and playing when playback finally starts, before the buffers are filled with new data. To avoid that we need new internal API. Maybe we should have a flag on SourceMediaStream to indicate whether it's supposed to produce "live" data; when that flag is set, and the stream is completely blocked, we can flush away its buffers (and when we add processing, buffers of downstream consumers as well).
(Reporter)

Comment 23

5 years ago
Well, we can't always throttle the source production of frames.  Contrived example:

stream = navigator.getUserMedia(...);
newstream = new MediaStream(stream.audioTracks, stream.videotracks);
video1.src = stream;
video2.src = newstream;
video1.play();
alert("foo");
video2.play();

We need to throw away frames in newstream or video2 until play() is called, even if the stream isn't a live stream (sourced from another video element, etc).

Comment 24

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 628543 [details]
> GUM Webrtc Backend v1
> 
> Review of attachment 628543 [details]:
> -----------------------------------------------------------------
> 
> I think this probably belongs in its own directory, maybe
> content/media/webrtc.
> 
> The MediaStreams integration looks reasonable at first glance. The main
> issue is having separate audio and video streams. That won't give us A/V
> sync.
> 
> ::: content/media/MediaEngineWebrtc.cpp
> @@ +532,5 @@
> > +  mSource = aStream;
> > +
> > +  AudioSegment* segment = new AudioSegment();
> > +  segment->Init(CHANNELS);
> > +  segment->InsertNullDataAtStart(1);
> 
> Why are you inserting silence here?
>>> It was for debugging .. I should have removed it ..
> 
> @@ +631,5 @@
> > +
> > +  // allocate shared buffer of lenght bytes
> > +  int buff_size = AUDIO_SAMPLE_LENGTH * sizeof(short);
> > +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(buff_size);
> > +  buffer->AddRef();
> 
> What's this extra AddRef for?
> 
> You can avoid copying here by creating the buffer first and then calling
> ExternalPlayoutGetData to write directly into the buffer.
>>> True .. Will update it as such

Comment 25

5 years ago
Building on Randell's contrived example
 stream = navigator.getUserMedia(...);
 newstreamArray[] = {};
 //setup 20 new streams
 for(i till 20)
  newstreamArray[i] = new MediaStream(stream.audioTracks, stream.videotracks);
 //play each stream exponentially spaced in time
 for(i till 20)
  {
   video[i].src = newStreamArray[i];
   setTimeout(i * 2^i, playVideo(i));
 }
 
function playVideo(i) {
  video[i].play();
}

In these scenarios .. will we have buffering issues ? should we buffer at all ? what is the scenario with audio buffers ? how is audio and video sync across different video elements handled ??

Just curious to see what should the browser handle these kind of scenarios
(Reporter)

Comment 26

5 years ago
There are two basic variations:

Realtime flows (or any stream that includes one track that derives from a realtime source):  You never want to do anything except display the most recent data.  If paused, it may stop rendering (freeze), but on thawing (or play()ing) it would jump to current.  If we do this, then there may not be a need for a new API.  We could have an attribute on tracks and on mediastreams about whether they're realtime flows (stream.realtime == or(all tracks.realtime)) if that helps.

Non-realtime flows (recorded flows, streamed recordings, etc) - even if the mediastream used is some type of intermediate/clone/processed stream - In those, we *could* ripple blocking states back, in theory.  So if you paused (or delayed playing) you could ask the source to pause/block.  If it does, fine, if it doesn't/can't ok.  The video element would drop frames, and skip to current on play/resume in either case, but the source would have the option of blocking or not.  But, this is not how mediastreams are designed in Mozilla today, though the getusermedia w3 draft doesn't speak to this at all.   (http://dev.w3.org/2011/webrtc/editor/getusermedia.html)

(If we were to do this, there might be cause to have two variations: video.pause(), which would ask the source to block, and video.freeze, which freeze playback but not ask the source to pause.  But I'm not sure that there's much actual use for freeze(), so probably this isn't worth it.)

Comment 27

5 years ago
Should we have the notifications from the media-stream on data-levels be handled in this code or should it be responsibility of clients using the media-engine ,say, MediaManager (as it is done today) ..
or putting it other way .. where should the MediaStream listener logic lie ??
(In reply to Randell Jesup [:jesup] from comment #23)
> stream = navigator.getUserMedia(...);
> newstream = new MediaStream(stream.audioTracks, stream.videotracks);
> video1.src = stream;
> video2.src = newstream;
> video1.play();
> alert("foo");
> video2.play();
> 
> We need to throw away frames in newstream or video2 until play() is called,
> even if the stream isn't a live stream (sourced from another video element,
> etc).

In this example, SourceMediaStream::HaveEnoughBuffered would consistently return true for newstream (after an initial buffering phase). So if newstream happened to be a media element, we would in fact throttle decoding. If it's a live stream, you can stop sending frames. Of course for live streams, we also need to extinguish that initial buffer.

So we need to expose on SourceMediaStream and maybe elsewhere a flag that you can use to say if you're a live source or not. When a live SourceMediaStream blocks, MediaStreamGraph will throw away its buffer.

Updated

5 years ago
blocking-kilimanjaro: --- → ?
Depends on: 752353
blocking-kilimanjaro: ? → +

Comment 29

5 years ago
Created attachment 634673 [details] [diff] [review]
Webrtc Backend With Basic Snapshot Support and Cleanup

Updated

5 years ago
Attachment #634673 - Flags: review?(rjesup)
Attachment #634673 - Flags: review?(anant)

Updated

5 years ago
Attachment #634673 - Flags: feedback?(roc)

Updated

5 years ago
Attachment #628543 - Attachment is obsolete: true
Attachment #628543 - Attachment is patch: false
Attachment #628543 - Flags: feedback?(roc)
Attachment #628543 - Flags: feedback?(rjesup)
Attachment #628543 - Flags: feedback?(anant)
Comment on attachment 634673 [details] [diff] [review]
Webrtc Backend With Basic Snapshot Support and Cleanup

Review of attachment 634673 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still unsure how you plan to do A/V sync. It really would be better to have a single MediaStream with two tracks to ensure that A/V sync works.

::: content/media/MediaEngineWebrtc.cpp
@@ +66,5 @@
> +  VideoRenderToStreamEvent(MediaEngineWebrtcVideoSource* aOwner,
> +						   unsigned char* aBuffer, 
> +						   int aSize, 
> +						   uint32_t aTimestamp,
> +						   int64_t aRenderTime)

Fix indent :-)

@@ +79,5 @@
> +  NS_IMETHOD Run()
> +  {
> +  	 ReentrantMonitorAutoEnter enter(mOwner->mMonitor);
> + 	 // create a VideoFrame and push it to the input stream;
> +  	layers::Image::Format format = layers::Image::PLANAR_YCBCR;

Fix indent.

Why are you doing this in a runnable? You can append video frames from any thread, so you can just append directly from DeliverFrame I think.

@@ +91,5 @@
> +  	layers::PlanarYCbCrImage::Data data;
> +  	data.mYChannel = frame;
> +  	data.mYSize = gfxIntSize(mOwner->mWidth, mOwner->mHeight);
> +  	data.mYStride = mOwner->mWidth * lumaBpp/ 8.0;
> +  	data.mCbCrStride = mOwner->mWidth * chromaBpp / 8.0;

I think you can use integer division here.

@@ +102,5 @@
> +  	data.mStereoMode = layers::STEREO_MODE_MONO;
> +
> +  	videoImage->SetData(data);
> +
> +  	mOwner->mVideoSegment.AppendFrame(videoImage,1, gfxIntSize(mOwner->mWidth, mOwner->mHeight));

Just append image.forget() and don't do the extra AddRef above.

@@ +112,5 @@
> +  MediaEngineWebrtcVideoSource* mOwner;
> +  unsigned char* mBuffer;
> +  int mSize;
> +  uint32_t mTimestamp;
> +  int64_t mRenderTime;

mSize, mTimestamp and mRenderTime are unused?

@@ +347,5 @@
> +  mViERender->StopRender(mCapIndex);
> +  mViERender->RemoveRenderer(mCapIndex);
> +  
> +  if(mVideoRenderingThread)
> +  {

Space before (

Generally we put { on the same line as the if.

@@ +379,5 @@
> +  mInSnapshotMode = true;
> +
> +  // Have to get rid of this sleep. 
> +  // To discusss and figure a way out
> +  sleep(2);

What's the problem here?

@@ +700,5 @@
> +
> +  // allocate shared buffer of lenght bytes
> +  int buff_size = PLAYOUT_SAMPLE_LENGTH * sizeof(short);
> +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(buff_size);
> +  //buffer->AddRef();

delete comment

@@ +705,5 @@
> +  int16_t* dest = static_cast<int16_t*>(buffer->Data());
> +  for(int i=0; i < sample_length; i++)
> +  {
> +	dest[i] = audio10ms[i];
> +  }

Why aren't you allocating the SharedBuffer first and then reading directly into it in ExternalPlayoutGetData?

@@ +707,5 @@
> +  {
> +	dest[i] = audio10ms[i];
> +  }
> +
> +  mAudioSegment.AppendFrames(buffer.forget(), sample_length, 0, sample_length, nsAudioStream::FORMAT_S16_LE);

Is ExternalPlayoutGetData guaranteed to output little-endian? I suspect not. In that case, you'd need to check for big-endian and either do some byte-swapping or fail with a reasonable NS_ERROR or something like that if you don't want to support big-endian.

Comment 31

5 years ago
Comment on attachment 634673 [details] [diff] [review]
Webrtc Backend With Basic Snapshot Support and Cleanup

Review of attachment 634673 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaEngineWebrtc.cpp
@@ +66,5 @@
> +  VideoRenderToStreamEvent(MediaEngineWebrtcVideoSource* aOwner,
> +						   unsigned char* aBuffer, 
> +						   int aSize, 
> +						   uint32_t aTimestamp,
> +						   int64_t aRenderTime)

Will Do :)

@@ +79,5 @@
> +  NS_IMETHOD Run()
> +  {
> +  	 ReentrantMonitorAutoEnter enter(mOwner->mMonitor);
> + 	 // create a VideoFrame and push it to the input stream;
> +  	layers::Image::Format format = layers::Image::PLANAR_YCBCR;

Runnable was used to keep DeliverFrame() short and quick to be available for WebRTC VideoEngine as soon as possible.  I am open with doing the Frame appending in the same function if it doesn't slow down the process. I am not sure how to verify this though :(

@@ +91,5 @@
> +  	layers::PlanarYCbCrImage::Data data;
> +  	data.mYChannel = frame;
> +  	data.mYSize = gfxIntSize(mOwner->mWidth, mOwner->mHeight);
> +  	data.mYStride = mOwner->mWidth * lumaBpp/ 8.0;
> +  	data.mCbCrStride = mOwner->mWidth * chromaBpp / 8.0;

Will Do ...

@@ +102,5 @@
> +  	data.mStereoMode = layers::STEREO_MODE_MONO;
> +
> +  	videoImage->SetData(data);
> +
> +  	mOwner->mVideoSegment.AppendFrame(videoImage,1, gfxIntSize(mOwner->mWidth, mOwner->mHeight));

true .. will do

@@ +112,5 @@
> +  MediaEngineWebrtcVideoSource* mOwner;
> +  unsigned char* mBuffer;
> +  int mSize;
> +  uint32_t mTimestamp;
> +  int64_t mRenderTime;

They are unused at this point in time. Since we are not sure how do we want to use those values as returned from the video-engine yet.

@@ +347,5 @@
> +  mViERender->StopRender(mCapIndex);
> +  mViERender->RemoveRenderer(mCapIndex);
> +  
> +  if(mVideoRenderingThread)
> +  {

Shall update the conditionals with this style

@@ +379,5 @@
> +  mInSnapshotMode = true;
> +
> +  // Have to get rid of this sleep. 
> +  // To discusss and figure a way out
> +  sleep(2);

Starting the Capture on the VideoEngine takes atleast a second for it to kick-start the capture process on the camera and start seeing the first Captured Frame. If i remove this sleep, the following function GetCaptureDeviceSnapshot() fails since the capture device is not yet seen any frame.  

We have to get rid of sleep anyhow .. I am not very sure what is the best way we can trigger to Dom Thread (MediaManager) when we see the frame asynchronously rather than sleeping in here ...

@@ +700,5 @@
> +
> +  // allocate shared buffer of lenght bytes
> +  int buff_size = PLAYOUT_SAMPLE_LENGTH * sizeof(short);
> +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(buff_size);
> +  //buffer->AddRef();

will do

@@ +705,5 @@
> +  int16_t* dest = static_cast<int16_t*>(buffer->Data());
> +  for(int i=0; i < sample_length; i++)
> +  {
> +	dest[i] = audio10ms[i];
> +  }

Ah! missed that one .. Shall implement that way

@@ +707,5 @@
> +  {
> +	dest[i] = audio10ms[i];
> +  }
> +
> +  mAudioSegment.AppendFrames(buffer.forget(), sample_length, 0, sample_length, nsAudioStream::FORMAT_S16_LE);

Not too sure .. Will check WebRTC spec on this .. Also is there ready-to-use MOZ Macro that i can use to do the endian-testing and swapping ...
(Assignee)

Comment 32

5 years ago
Comment on attachment 634673 [details] [diff] [review]
Webrtc Backend With Basic Snapshot Support and Cleanup

Review of attachment 634673 [details] [diff] [review]:
-----------------------------------------------------------------

For A/V sync, we'll need a method to merge two media streams into one. I believe roc is working on that in bug 758391. Let's not do the merging in the MediaEngine, that's the MediaManager's job, since it knows what was asked by DOM.

::: content/media/MediaEngineWebrtc.cpp
@@ +57,5 @@
> +};
> +
> +/**
> + * Runnable to write the video frames to MediaStream 
> + */

roc already covered this I think, we don't need a runnable.

@@ +155,5 @@
> +void
> +MediaEngineWebrtcVideoSource::Init()
> +{
> +
> +  if(NULL == mVideoEngine)

nit: use { } even if your if statements only contain a single statement.

@@ +379,5 @@
> +  mInSnapshotMode = true;
> +
> +  // Have to get rid of this sleep. 
> +  // To discusss and figure a way out
> +  sleep(2);

Let's not use sleep at all. This is how this should be done:
- Mark a class variable to "SNAPSHOT" or an enum when Snapshot() is called.
- Call start on the video source, create a new thread and wait on it.
- In DeliverFrame, check for the class variable, since it is a snapshot; grab exactly one frame and stop the source.
- Join the thread you created earlier, convert the frame into an image and return from Snapshot.

@@ +652,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +MediaEngineWebrtcAudioSource::Snapshot(PRUint32 aDuration, nsIDOMFile** aFile)

Same comments as for video will apply when this is implemented.

@@ +707,5 @@
> +  {
> +	dest[i] = audio10ms[i];
> +  }
> +
> +  mAudioSegment.AppendFrames(buffer.forget(), sample_length, 0, sample_length, nsAudioStream::FORMAT_S16_LE);

For swapping, you can use PR_htons/PR_htonl to convert a short or long from "host" order to "network" (big-endian) order.
Attachment #634673 - Flags: review?(anant)
(In reply to Anant Narayanan [:anant] from comment #32)
> For A/V sync, we'll need a method to merge two media streams into one. I
> believe roc is working on that in bug 758391. Let's not do the merging in
> the MediaEngine, that's the MediaManager's job, since it knows what was
> asked by DOM.

That can synchronize two streams based on their timestamps, but something needs to make sure the timestamps are compatible in the first place, right? _Something_ has to handle clock-drift from two independent, real-time hardware media sources, and do it without introducing buffering.

Once things get timestamped, it's too late. As I last remember roc's MediaStream Processing API, you could either have blocking inputs (which would cause buffering and introduce latency if a stream gets ahead) or non-blocking inputs (which would cause silence to get interjected into your audio when it falls behind, causing stuttering). Probably what you really want is to adjust the timestamps on the video frames to match the actual rate at which the audio is coming in. I don't know if you'd also need to do something to the audio (adjust the sample rate or actually resample) to get it to match the rate at which the media stream graph is being advanced.

roc, am I off-base here?
(In reply to Suhas from comment #31)
> @@ +112,5 @@
> > +  MediaEngineWebrtcVideoSource* mOwner;
> > +  unsigned char* mBuffer;
> > +  int mSize;
> > +  uint32_t mTimestamp;
> > +  int64_t mRenderTime;
> 
> They are unused at this point in time. Since we are not sure how do we want
> to use those values as returned from the video-engine yet.

Don't add them until you need them.

> @@ +707,5 @@
> > +  {
> > +	dest[i] = audio10ms[i];
> > +  }
> > +
> > +  mAudioSegment.AppendFrames(buffer.forget(), sample_length, 0, sample_length, nsAudioStream::FORMAT_S16_LE);
> 
> Not too sure .. Will check WebRTC spec on this .. Also is there ready-to-use
> MOZ Macro that i can use to do the endian-testing and swapping ...

Not for this, that I know of. We have an IS_LITTLE_ENDIAN macro you can use to detect endianness. So define int16_t NativeEndianToLittleEndian(int16_t) that either does nothing or swaps bytes.
(In reply to Anant Narayanan [:anant] from comment #32)
> For swapping, you can use PR_htons/PR_htonl to convert a short or long from
> "host" order to "network" (big-endian) order.

Here we need to convert from "host" to little-endian order, though.
(In reply to Timothy B. Terriberry (:derf) from comment #33)
> roc, am I off-base here?

Actually, now that I've thought about it some more, we can merge streams in the MediaStreamGraph and reconcile their timelines by saying that the current play point in each stream must be aligned. That makes sense and will work here *if* the two streams are merged before either of them is able to advance (e.g. before any data is placed in either stream).

For A/V sync, it's still the stream producer's responsibility to ensure that the first data put into each stream is in sync, and that having specified a given track rate, the data it puts into that track has that exact rate, without drifting. That's where you need to handle clock drift.

It would be a bit clearer if we just produced a single stream with multiple tracks from the beginning, but if that's hard, I think we'll be OK with the current scheme.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> For A/V sync, it's still the stream producer's responsibility to ensure that
> the first data put into each stream is in sync, and that having specified a
> given track rate, the data it puts into that track has that exact rate,
> without drifting. That's where you need to handle clock drift.

I'm guessing "exactly that rate" means w.r.t. the system clock (I'm presuming that's what the media stream graph is using to advance?). That pretty much means you'll have to resample the audio.
(In reply to Timothy B. Terriberry (:derf) from comment #37)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> > For A/V sync, it's still the stream producer's responsibility to ensure that
> > the first data put into each stream is in sync, and that having specified a
> > given track rate, the data it puts into that track has that exact rate,
> > without drifting. That's where you need to handle clock drift.
> 
> I'm guessing "exactly that rate" means w.r.t. the system clock (I'm
> presuming that's what the media stream graph is using to advance?). That
> pretty much means you'll have to resample the audio.

Right now it's the system clock. I can change that to use the hardware audio-output clock, but that doesn't help, right? Someone has to resample somewhere to deal with clock skew.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Right now it's the system clock. I can change that to use the hardware
> audio-output clock, but that doesn't help, right? Someone has to resample
> somewhere to deal with clock skew.

The hardware output clock doesn't help with getUserMedia-sourced streams, no, though it might make things easier for the echo canceler (I'd have to ask JM). Whatever we do with getUserMedia, we will have to adjust streams coming in off the network, because the sender will have yet another independent clock, but this is usually handled by the jitter buffer (NetEQ, webrtc's jitter buffer, uses pitch period insertion and deletion instead of resampling).

Comment 40

5 years ago
(In reply to Timothy B. Terriberry (:derf) from comment #39)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> > Right now it's the system clock. I can change that to use the hardware
> > audio-output clock, but that doesn't help, right? Someone has to resample
> > somewhere to deal with clock skew.
> 
> The hardware output clock doesn't help with getUserMedia-sourced streams,
> no, though it might make things easier for the echo canceler (I'd have to
> ask JM). Whatever we do with getUserMedia, we will have to adjust streams
> coming in off the network, because the sender will have yet another
> independent clock, but this is usually handled by the jitter buffer (NetEQ,
> webrtc's jitter buffer, uses pitch period insertion and deletion instead of
> resampling).



I think we need something very similar on the peer connection side as well .. How do we merge , sync audio and video streams coming in from the peer ?
(Reporter)

Comment 41

5 years ago
Right - jitter buffers move a lot more than any reasonable hardware clock, so they need to do more than resample.  The Time Scale Modification (TSM) stuff in NetEQ is a common and quite effective trick.  (For anyone who didn't know.)

I certainly would be tempted to use the hardware capture clock for incoming mic data, and the playout clock (not guaranteed to the be same as the capture clock!) for data from the PeerConnection, and if the two are connected directly (audio.src = getUserMedia_stream) then force the resample; but I realize that's not how MediaStreams work.

An example for a videophone (DSP details omitted).  Buffer nodes rebuffer into 10ms frames.
Recording taps, VAD, control, etc all are omitted.  A lot of this is inside the webrtc code, but it helps to understand the complexity of stream management.

The wonder of Emacs artist-mode!
                                   
-------------------------------------------------------------------------
            RTP                         
             |                                     |
       +-------------+                             |    Send audio thread
       | Jitter Buff |     RTP reception thread    | 
       |   Insert    |                             |         RTP
       +------+------+                             |          |
              |                                    |  +-----------------+ 
 -------------+------------------------------------+  |  Speech Encode  |
       +------+------+     Receive audio           |  |                 | 
       | Jitter Buff |         thread              |  +--------+--------+
       |   Dequeue   |                             |  +--------+--------+
       +------+------+                             |  |   Mixer         +- Other
       +------+------+                             |  +--------+--------+ sources
       |  Decode     |                             |   +-------+--------+
       |             |                             |   |  Buffer        |
       +------+------+                             |   +-------+--------+
       +------+------+                             |           |
       |Time Scale   |                             |    +------+----------+
       |Modification |                             |    |   Conference    |
       +------+------+                             | /--+Bridge (optional)|
       +------+------+       +-------------+   /---+-   +-------+---------+
       |   Buffer    +-------+   audio     +---    |            |
       |             |       | conf. bridge|       |     +------+-------+
       +------+------+       +-------------+       |     |AGC and output|
       +------+------+                             |     | level adjust |
       |   Mixer     +-- other sources             |     |& DTMF insert |
       |             |                             |     +------+-------+
       +------+------+                             |            |
       +------+------+                             |     +------+-------+
       |AGC, DC Filt |            +---------------+|     |    Main      |
       | & volume    +------------+ fsb queue     ++-----+    AEC       |
       +------+------+            |               ||     +-------+------+
       +------+------+            +---------------+|      +------+------+
       |Playout audio|                             |      |Capture audio|
       |    driver   |                             |      |    driver   |
       +------+------+                             |      +------+------+
--------------+------------------------------------+-------------+---------
       +------+------+    Linux Kernel                           |
       |  Buffers    |                                    +------+-------+
       +-------------+                                    |  Buffers     |
       |             |                                    +--------------+
       +-------------+                                    |              |
       |             |                                    +--------------+
       +----------+--+                                    |              |
                  |                                       +-------+------+
               +--+-----------------------------------------------+-------+
               |                                                          |
               |           Hardware Audio Codec                           |
               | Note: Use HW Biquad filters to flatten response if avail |
               +--+-----------------------------------------+-------------+
                  |                                         |
                ..|.                                        |
               ..  ..                                     --+--
               ..  ..                                     --+--
              ..    ..                                   (     )
             ..........                                   -----


This let the output (mic -> RTP) thread run with microphone timestamps, and the input thread (after the TSM and associated buffer node) run with audio playout timestamps.  RTP input to TSM runs with RTP source timestamps; the TSM takes the place of a resampler.  The only complexity is at the cross input/output thread links, which would need resampling if the mic and playout clocks aren't synced (mine were), and mixing of other sources (ditto).

---

It's probably helpful to label your clock sources; merging two streams with the same clock source doesn't need a resample; merging them with different sources does.  The complication here is that MediaStreams run on internal (CPU/NTP) clock time; the only sources that do so are probably pre-recorded sources.

So, for all sources that aren't internal (sourced at a video or audio decoder element), the inputs will need associated resamplers or timestamp transcoders.  For audio, the sources will need to monitor input samples versus nominal frequency (at CPU/NTP clock rate), perhaps with a PLL, and use that to drive the settings for the resampler.  For video, I would simply use the PLL to convert input timestamps to internal CPU timestamps.  This might need some tweakery, as many RTP sinks assume video sources are at consistent cadences and timestamp deltas in order to provide smooth playback, generally some integer frame rate, or some integer number of frames each on a 30 (or 25!) fps boundary (i.e. timestamp deltas of 3000 at 90KHz timestamp rate.)

Note that many audio sources may require a resampler anyways to feed into the core, so this would just affect settings for it.  The PLL aspect of it does mean it may take a short time to converge after starting.

We probably need this to be a core part of MediaStream data inputs, with an option to disable it if we know the source is synced to the CPU.

Sorry for the long message...
(Reporter)

Comment 42

5 years ago
(In reply to Suhas from comment #40) 
> I think we need something very similar on the peer connection side as well
> .. How do we merge , sync audio and video streams coming in from the peer ?

That's all handled by the core webrtc code.  The only open question is are they synced/resampled to internal clockrate or not (probably they are). If they are *and* the frequency for audio is correct, we don't need any resampling at the PeerConnection->MediaStream interface.
(Assignee)

Updated

5 years ago
Attachment #605310 - Attachment is obsolete: true
(Assignee)

Comment 43

5 years ago
Comment on attachment 634673 [details] [diff] [review]
Webrtc Backend With Basic Snapshot Support and Cleanup

Clearing flags, I have an updated set of patches in the works.
Attachment #634673 - Attachment is obsolete: true
Attachment #634673 - Flags: review?(rjesup)
Attachment #634673 - Flags: feedback?(roc)
(Reporter)

Updated

5 years ago
Depends on: 771135
(Reporter)

Updated

5 years ago
Depends on: 771248
(Assignee)

Comment 44

5 years ago
Created attachment 639501 [details] [diff] [review]
Part 1: Move MediaEngine files and tweak interface

This is part 1 of 3 patches. All the media engine files have been moved to the content/media/webrtc directory, and a small tweak was made to the MediaEngine interface. Allocate() now returns an nsresult instead of a SourceMediaStream* to allow the MediaManager to call it from a new thread instead of the main thread. 

An allocate operation can take a while depending on device initialization and should not be on the main thread, but we have to create a MediaStream on the main thread. The MediaManager should now do the MediaStream creation and pass it to Start() instead of retrieving one from Allocate().
Assignee: snandaku → anant
Attachment #639501 - Flags: review?(roc)
Attachment #639501 - Flags: review?(rjesup)
(Assignee)

Comment 45

5 years ago
Created attachment 639502 [details] [diff] [review]
Part 2: Implement WebRTC MediaEngine

This part implements the WebRTC backend for MediaEngine. The video and audio backends live in their own files. We don't use timers or runnables anymore, either in the audio or video engine.

There's some trickery in Snapshot() - it is a blocking call expected to be called on a separate thread (all calls into the MediaEngine now are) - but the best way to get a snapshot from WebRTC is asynchronous. I used a condition variable in combination with PR_WaitCondVar to handle this.

There's one unresolved issue: NS_GetSpecialDirectory is not thread-safe and must be called from the main thread. I suspect I'll need a runnable for this and use another condition variable to synchronize with Snapshot(), just like DeliverFrame(). Any other ideas?
Attachment #639502 - Flags: review?(roc)
Attachment #639502 - Flags: review?(rjesup)
(Assignee)

Comment 46

5 years ago
Created attachment 639504 [details] [diff] [review]
Part 3: DOM glue for getUserMedia on Desktop

The main changes to the DOM code are:
- All calls into the MediaEngine (Allocate, Start, Stop, Deallocate) are now off the main thread (as they should have been from the start).
- Creation of a SourceMediaStream and listener management are both on the main thread. Since the latter is now on the main thread we can manipulate the hash table safely without a lock, so I removed it.
- Disabled popup blocking on Desktop until we determine what the UI for {picture:true} is going to be. AFAIK, if we end up with a doorhanger style UI (like Geolocation), we may not need to do this check.

jst understands this code best (since he reviewed the original version), asking him to review these changes too!
Attachment #639504 - Flags: review?(jst)
(Assignee)

Comment 47

5 years ago
We're not going to handle A/V sync in this bug, that's something we can address via bug 771135 and by creating a new bug for the corresponding DOM/MediaEngine changes. In this version, calling getUserMedia({audio:true,video:true}, ...) will return NS_ERROR_NOT_IMPLEMENTED.
Blocks: 768924
(Assignee)

Comment 48

5 years ago
Created attachment 639535 [details] [diff] [review]
Part 2: Implement WebRTC MediaEngine

Fixed the NS_GetSpecialDirectory issue by making MediaEngineWebRTCVideo into a runnable and creating a temporary file on the main thread, and dispatching it synchronously from Snapshot().
Attachment #639502 - Attachment is obsolete: true
Attachment #639502 - Flags: review?(roc)
Attachment #639502 - Flags: review?(rjesup)
Attachment #639535 - Flags: review?(roc)
Attachment #639535 - Flags: review?(rjesup)
Comment on attachment 639501 [details] [diff] [review]
Part 1: Move MediaEngine files and tweak interface

Review of attachment 639501 [details] [diff] [review]:
-----------------------------------------------------------------

Please use "hg move" to preserve history and make this diff sane
Comment on attachment 639535 [details] [diff] [review]
Part 2: Implement WebRTC MediaEngine

Review of attachment 639535 [details] [diff] [review]:
-----------------------------------------------------------------

Is it really the case that WebRTC guarantees exactly mFPS frames per second at regular intervals? Due to frame drops etc I would have expected to get a series of frames with unpredictable timestamps. But other than that, you seem to be using the MediaStreams APIs correctly, so r+ on that.
Attachment #639535 - Flags: review?(roc) → review+
(Assignee)

Comment 51

5 years ago
Created attachment 639560 [details] [diff] [review]
Part 1: Move MediaEngine files and tweak interface

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Comment on attachment 639501 [details] [diff] [review]
> Part 1: Move MediaEngine files and tweak interface
> Please use "hg move" to preserve history and make this diff sane

Nice, thanks for the tip!
Attachment #639501 - Attachment is obsolete: true
Attachment #639501 - Flags: review?(roc)
Attachment #639501 - Flags: review?(rjesup)
Attachment #639560 - Flags: review?(roc)
(Assignee)

Comment 52

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> Is it really the case that WebRTC guarantees exactly mFPS frames per second
> at regular intervals? Due to frame drops etc I would have expected to get a
> series of frames with unpredictable timestamps. But other than that, you
> seem to be using the MediaStreams APIs correctly, so r+ on that.

Yes, DeliverFrame is called when the next frame should be rendered, so the engine takes care of duplicating/skipping frames as required before calling it.

There is one behaviour I observe though: if I pause a stream and resume (say, via <video> controls), the stream tries to "catch up" by playing faster than normal rate; both for audio and video. Is this expected behaviour? If yes, how can we simply skip to the current frame instead?
(Reporter)

Comment 53

5 years ago
Comment on attachment 639535 [details] [diff] [review]
Part 2: Implement WebRTC MediaEngine

Review of attachment 639535 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits addressed.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +61,5 @@
> +    mVoiceEngine = webrtc::VoiceEngine::Create();
> +    if (!mVoiceEngine) {
> +      return;
> +    }
> +    mVoiceEngine->SetTraceFile("GIPSVoETrace.txt");

Not sure we should be enabling debug file generation (in the current directory!), especially in opt/release builds.

Suggestion: enable them in debug builds, or if a specific logging level/target is set. Ditto for video debugs.

See also bug 743703

@@ +88,5 @@
> +    char deviceName[128];
> +    memset(deviceName, 0, 128);
> +
> +    char uniqueID[128];
> +    memset(uniqueID, 0, 128);

really minor nit, but "memset(uniqueID, 0, sizeof(uniqueID));".  Ditto deviceName.  Good habit to avoid repeating constants (when you're forced to use them).

@@ +90,5 @@
> +
> +    char uniqueID[128];
> +    memset(uniqueID, 0, 128);
> +
> +    ptrVoEHw->GetRecordingDeviceName(i, deviceName, uniqueID);

Note that GetRecordingDeviceName() takes char [128]'s for both, so we need to use constants here.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +93,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  // This runnable is for creating a temporary file on the main thread.
> +  NS_IMETHODIMP
> +  Run()

Not sure I like a generically-named runnable that's not in a simple wrapper-class (most Run() methods I saw were in classes that exist largely to wrap Run() runnables).  CreateTempFile()?  CreateTempFileRunnable()?

@@ +96,5 @@
> +  NS_IMETHODIMP
> +  Run()
> +  {
> +    nsCOMPtr<nsIFile> tmp;
> +    nsresult rv = NS_GetSpecialDirectory("TmpD", getter_AddRefs(tmp));

NS_OS_TEMP_DIR

::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +80,5 @@
> +  codec.channels = CHANNELS;
> +  codec.rate = SAMPLE_RATE;
> +  codec.plfreq = SAMPLE_FREQUENCY;
> +  codec.pacsize = 160; // Default packet size
> +  codec.pltype = 0;    // Payload type

prefer pacsize in terms of ms, not a bare constant: #define SAMPLE_LENGTH (SAMPLE_FREQUENCY*10/1000)  // in samples, not bytes

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +12,5 @@
> +NS_IMPL_THREADSAFE_ISUPPORTS1(MediaEngineWebRTCVideoSource, nsIRunnable)
> +
> +// Static variables to hold device names and UUIDs.
> +const unsigned int MediaEngineWebRTCVideoSource::KMaxDeviceNameLength = 128;
> +const unsigned int MediaEngineWebRTCVideoSource::KMaxUniqueIdLength = 256;

These are probably reasonable, but do these values come from somewhere?

@@ +159,5 @@
> +  MediaEngineVideoOptions aOpts;
> +  aOpts.mWidth = mWidth;
> +  aOpts.mHeight = mHeight;
> +  aOpts.mMaxFPS = mFps;
> +  aOpts.codecType = kVideoCodecI420;

As best I can tell, nothing actually looks at mMaxFPS or codecType, and there's no definition of the semantics for the fields in MediaEngineVideoOptions (except indirectly the codecType).  So there's probably nothing to do here.
Attachment #639535 - Flags: review?(rjesup) → review+
Attachment #639560 - Flags: review?(roc) → review+
(Assignee)

Comment 54

5 years ago
(In reply to Randell Jesup [:jesup] from comment #53)
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +93,5 @@
> > +  NS_DECL_ISUPPORTS
> > +
> > +  // This runnable is for creating a temporary file on the main thread.
> > +  NS_IMETHODIMP
> > +  Run()
> 
> Not sure I like a generically-named runnable that's not in a simple
> wrapper-class (most Run() methods I saw were in classes that exist largely
> to wrap Run() runnables).  CreateTempFile()?  CreateTempFileRunnable()?

The first time I wrote it I did it that way, but the main issue then becomes communicating the path between that runnable and Snapshot(). I'd have to make mSnapshotPath a public property (or create a setter), which is alright, but I felt that munging class variables in between threads within the same class was just a tiny bit better than doing it between two different classes :)

> @@ +159,5 @@
> > +  MediaEngineVideoOptions aOpts;
> > +  aOpts.mWidth = mWidth;
> > +  aOpts.mHeight = mHeight;
> > +  aOpts.mMaxFPS = mFps;
> > +  aOpts.codecType = kVideoCodecI420;
> 
> As best I can tell, nothing actually looks at mMaxFPS or codecType, and
> there's no definition of the semantics for the fields in
> MediaEngineVideoOptions (except indirectly the codecType).  So there's
> probably nothing to do here.

It may be of some use in the future, but we can always add it back when needed, so I'll remove this for now.
(Reporter)

Comment 55

5 years ago
(In reply to Anant Narayanan [:anant] from comment #54)
> (In reply to Randell Jesup [:jesup] from comment #53)
> > ::: content/media/webrtc/MediaEngineWebRTC.h
> > @@ +93,5 @@
> > > +  NS_DECL_ISUPPORTS
> > > +
> > > +  // This runnable is for creating a temporary file on the main thread.
> > > +  NS_IMETHODIMP
> > > +  Run()
> > 
> > Not sure I like a generically-named runnable that's not in a simple
> > wrapper-class (most Run() methods I saw were in classes that exist largely
> > to wrap Run() runnables).  CreateTempFile()?  CreateTempFileRunnable()?
> 
> The first time I wrote it I did it that way, but the main issue then becomes
> communicating the path between that runnable and Snapshot(). I'd have to
> make mSnapshotPath a public property (or create a setter), which is alright,
> but I felt that munging class variables in between threads within the same
> class was just a tiny bit better than doing it between two different classes

I think I wasn't clear - I meant just rename it something other than "Run()"; not add a wrapper class.

> :)
> 
> > @@ +159,5 @@
> > > +  MediaEngineVideoOptions aOpts;
> > > +  aOpts.mWidth = mWidth;
> > > +  aOpts.mHeight = mHeight;
> > > +  aOpts.mMaxFPS = mFps;
> > > +  aOpts.codecType = kVideoCodecI420;
> > 
> > As best I can tell, nothing actually looks at mMaxFPS or codecType, and
> > there's no definition of the semantics for the fields in
> > MediaEngineVideoOptions (except indirectly the codecType).  So there's
> > probably nothing to do here.
> 
> It may be of some use in the future, but we can always add it back when
> needed, so I'll remove this for now.

I did NOT check for mWidth/Height.  roc was happy with it for now, probably leave it (maybe flag it to roc to define these and use them).
mWidth and mHeight are used --- they're passed as the intrinsic frame size when we append a video frame to the VideoSegment; that will be used to set the default width and height for <video> elements consuming this stream. So definitely leave them in!
When I set the stream as the source of a video element, and then try to draw a frame of this video to a canvas, it does not work.

The drawImage methods calls nsCanvasRenderingContext2D::DrawImage, that calls nsLayoutUtils::SurfaceFromElement, that bails if the element does not have a current principal, hence the failure. When I comment the relevant lines, everything works fine, though.

I've no idea of the security implications, I just though it would be fun to have real time preview, and to be able to take still picture from my webcam to a canvas and to be able to process the image afterwards.
(Assignee)

Updated

5 years ago
Blocks: 771833
(Assignee)

Updated

5 years ago
No longer depends on: 771135
(Assignee)

Updated

5 years ago
Blocks: 771834
Comment on attachment 639504 [details] [diff] [review]
Part 3: DOM glue for getUserMedia on Desktop

- In ProcessGetUserMediaSnapshot():

+#if !defined(XP_WIN) && !defined(XP_UNIX)
+    if (mWindow && (mWindow->GetPopupControlState() <= openControlled)) {
+      return;
+    }
+    nsCOMPtr<nsIPopupWindowManager> pm =
+      do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID);
+    if (!pm) {
+      return;
+    }

As the comment above this code says, this does *not* run on the main thread, so you'll need to not touch mWindow, the popup window manager, the document, URIs etc here. Use a runnable for this

-  nsCOMPtr<nsPIDOMWindow> mWindow;
+  nsPIDOMWindow* mWindow;

You really should leave this as an nsCOMPtr, just proxy the release of this in the dtor.

Otherwise this looks good. r- until that's fixed.
Attachment #639504 - Flags: review?(jst) → review-
(Assignee)

Comment 59

5 years ago
Created attachment 640900 [details] [diff] [review]
Part 3: DOM glue for getUserMedia on Desktop

Moved the popup blocking to MediaManager::GetUserMedia which is on the main thread and fail early. A nice side-effect of this is that GetUserMediaRunnable no longer needs a reference to the window, just the windowID so we don't have to own a reference to it.

This patch also enables MOZ_MEDIA_NAVIGATOR on Desktop builds. This allows developers to start playing with the feature by flipping the "media.navigator.enabled" pref. The pref is in place (defaults to false) to prevent usage of the API in the wild since we don't have a permission model or UI for getUserMedia on Desktop.
Attachment #639504 - Attachment is obsolete: true
Attachment #640900 - Flags: review?(jst)
Comment on attachment 640900 [details] [diff] [review]
Part 3: DOM glue for getUserMedia on Desktop

   SuccessCallbackRunnable(nsIDOMGetUserMediaSuccessCallback* aSuccess,
-    nsIDOMMediaStream* aStream, PRUint64 aWindowID)
+    already_AddRefed<nsDOMMediaStream> aStream, PRUint64 aWindowID)

Any real reason to use already_AddRefed<> here? Seems a raw pointer does exactly the same thing with a smaller brain print (same compiled code, ideally).

r=jst
Attachment #640900 - Flags: review?(jst) → review+
(Assignee)

Comment 61

5 years ago
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #60)
>    SuccessCallbackRunnable(nsIDOMGetUserMediaSuccessCallback* aSuccess,
> -    nsIDOMMediaStream* aStream, PRUint64 aWindowID)
> +    already_AddRefed<nsDOMMediaStream> aStream, PRUint64 aWindowID)
> 
> Any real reason to use already_AddRefed<> here? Seems a raw pointer does
> exactly the same thing with a smaller brain print (same compiled code,
> ideally).

No particular reason - I was just trying avoid using raw pointer at all at the time - I'll  switch it back.

Thanks for the review, much appreciated!
(Assignee)

Comment 62

5 years ago
(In reply to Anant Narayanan [:anant] from comment #61)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #60)
> >    SuccessCallbackRunnable(nsIDOMGetUserMediaSuccessCallback* aSuccess,
> > -    nsIDOMMediaStream* aStream, PRUint64 aWindowID)
> > +    already_AddRefed<nsDOMMediaStream> aStream, PRUint64 aWindowID)
> > 
> > Any real reason to use already_AddRefed<> here? Seems a raw pointer does
> > exactly the same thing with a smaller brain print (same compiled code,
> > ideally).
> 
> No particular reason - I was just trying avoid using raw pointer at all at
> the time - I'll  switch it back.
> 
> Thanks for the review, much appreciated!

Oh whoops! Since the introduction of GetUserMediaStreamRunnable (which is responsible for calling the success callback), we don't even need this anymore! I've removed the alternate constructor, SuccessCallbackRunnable is now only used for returning DOMFiles as a result of a call with {picture:true}.
(Assignee)

Comment 63

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/431ab4d097c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/038e89521330
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53e8a614a4c
(Assignee)

Comment 64

5 years ago
Backed out, B2G is busted. Investigating.
(Assignee)

Comment 65

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbd6655fb05
https://hg.mozilla.org/integration/mozilla-inbound/rev/3059151ff349
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3ef213c75b
https://hg.mozilla.org/mozilla-central/rev/3bbd6655fb05
https://hg.mozilla.org/mozilla-central/rev/3059151ff349
https://hg.mozilla.org/mozilla-central/rev/1d3ef213c75b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Can this be verified without the front-end implemented? If so, here's some questions:

- Can provide an example of a test case or app that uses getUserMedia? A real application would be better, but a test page works too. Looking to see an example so that our desktop team understand what we are working with.
- Are there any automated tests for this patch? If not, can we get a bug on file for this?
Whiteboard: [qa?]
(Assignee)

Comment 68

5 years ago
Visit https://people.mozilla.com/~anarayanan/gum_test.html and try out the three differnet options. We don't have tests enabled yet, I believe we already have a bug for that, I'll try to find it.

Updated

5 years ago
Whiteboard: [qa?] → [qa+]
(Reporter)

Updated

5 years ago
Blocks: 773644
(Reporter)

Updated

5 years ago
Blocks: 773646
(Reporter)

Updated

5 years ago
Blocks: 773649

Updated

5 years ago
QA Contact: jsmith
I can confirm that this code has landed on Nightly and can be tested using Anant's page. I'll followup ramping up, getting a test plan together, and jumping in the WebRTC meetings to start testing this feature.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
New raw uses of PRLock should not have been approved to land in Gecko, especially new wrong raw uses of PRLock.  I filed bug 848401 on this.
Depends on: 848401
Blocks: 928096
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.