Last Comment Bug 752351 - Implement fallback MediaEngine backend
: Implement fallback MediaEngine backend
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Anant Narayanan [:anant]
:
Mentors:
Depends on: 750943 768924
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-06 11:56 PDT by Anant Narayanan [:anant]
Modified: 2012-07-27 10:27 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
-


Attachments
Fallback Media Engine - v1 (10.34 KB, patch)
2012-05-13 20:50 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Fallback Media Engine - v2 (9.77 KB, patch)
2012-05-18 18:22 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Fallback Media Engine - v3 (10.83 KB, patch)
2012-05-22 12:24 PDT, Anant Narayanan [:anant]
fabrice: feedback+
Details | Diff | Splinter Review
Fallback Media Engine - v3.1 (10.71 KB, patch)
2012-05-24 14:21 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Fallback Media Engine - v3.2 (10.65 KB, patch)
2012-05-24 15:13 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Fallback Media Engine - v3.3 (10.71 KB, patch)
2012-05-24 15:48 PDT, Anant Narayanan [:anant]
roc: review+
Details | Diff | Splinter Review
Fallback Media Engine - v3.3.1 (11.11 KB, patch)
2012-05-31 20:46 PDT, Anant Narayanan [:anant]
no flags Details | Diff | Splinter Review
Fallback Media Engine - v3.3.2 (11.10 KB, text/plain)
2012-06-01 01:12 PDT, Anant Narayanan [:anant]
rjesup: review+
Details

Description Anant Narayanan [:anant] 2012-05-06 11:56:41 PDT
Bug 750943 defines the MediaEngine interface. This bug tracks the implementation of a "fallback" MediaEngine interface that will work on all platforms without or without media hardware present. This will help in testing the DOM injection for getUserMedia on multiple platforms from the start.
Comment 1 Anant Narayanan [:anant] 2012-05-13 20:50:59 PDT
Created attachment 623572 [details] [diff] [review]
Fallback Media Engine - v1

If the interface in bug 750943 is acceptable, this is the fallback engine that returns green video and silent audio. It should work on all platforms and can be the starting point for the DOM implementation until we have backends for Android, B2G and Desktop.
Comment 2 [:fabrice] Fabrice Desré 2012-05-14 15:01:30 PDT
Comment on attachment 623572 [details] [diff] [review]
Fallback Media Engine - v1

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

postponing feedback until bug 750943 is stable
Comment 3 Anant Narayanan [:anant] 2012-05-18 18:22:28 PDT
Created attachment 625327 [details] [diff] [review]
Fallback Media Engine - v2

Updated "default" implementation of a MediaEngine. The engine returns green frames for <video> (0 in YCbCr) and silence for <audio> and lists one device for each.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-20 15:34:40 PDT
Comment on attachment 625327 [details] [diff] [review]
Fallback Media Engine - v2

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

You're not calling AdvanceKnownTracksTime anywhere. You need to.

Also, you're not calling EndTrack() or Finish(). There needs to be some shutdown code.

::: content/media/MediaEngineDefault.cpp
@@ +28,5 @@
> +
> +void
> +MediaEngineDefaultVideoSource::GetUUID(nsAString& aUUID)
> +{
> +  aUUID.Assign(NS_LITERAL_STRING("1041FCBD-3F12-4F7B-9E9B-1EC556DD5676"));

Hmm. Is aUUID always going to be a real UUID? If it is, we should probably use nsID instead of a string.

@@ +40,5 @@
> +    return NULL;
> +  }
> +
> +  mState = kAllocated;
> +  return nsDOMMediaStream::CreateInputStream();

CreateInputStream should be called CreateSourceStream I guess. I'll fix it...

@@ +85,5 @@
> +    
> +  nsRefPtr<layers::Image> image = mImageContainer->CreateImage(&format, 1);
> +  
> +  mImage = static_cast<layers::PlanarYCbCrImage*>(image.get());
> +  PRUint8* frame = (PRUint8*) PR_Calloc((WIDTH * HEIGHT) / 2, 3);

Can you use something other than green? It might be helpful since then we can tell the difference between the default video source and something that's erroneously using all zeroes.

@@ +231,5 @@
> +MediaEngineDefaultAudioSource::Notify(nsITimer* aTimer)
> +{
> +  AudioSegment segment;
> +  segment.Init(CHANNELS);
> +  segment.InsertNullDataAtStart(1); // One tick?

Yes.

::: content/media/MediaEngineDefault.h
@@ +45,5 @@
> +  void GetName(nsAString&);
> +  void GetUUID(nsAString&);
> +
> +  MediaEngineVideoOptions GetOptions();
> +  already_AddRefed<nsDOMMediaStream> Allocate();

All virtual methods should include "virtual"

@@ +104,5 @@
> +  void EnumerateAudioDevices(nsTArray<nsAutoPtr<MediaEngineAudioSource> >*);
> +
> +private:
> +  nsAutoPtr<MediaEngineVideoSource> mVsource;
> +  nsAutoPtr<MediaEngineAudioSource> mAsource;

mVSource, mASource

You've got a lifetime management problem here. EnumerateVideoDevices/EnumerateAudioDevices is transferring membership to the caller and nulling out mVSource/mASource. Maybe you shouldn't keep references here and just create them in the Enumerate methods?
Comment 5 Anant Narayanan [:anant] 2012-05-22 12:24:12 PDT
Created attachment 626127 [details] [diff] [review]
Fallback Media Engine - v3

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> You're not calling AdvanceKnownTracksTime anywhere. You need to.

I'm not sure I understand AdvanceKnownTracksTime correctly. Does it need to be called with StreamTime(0) before adding the first track on a stream? In the case of this fallback engine, the stream always only has a single track, so should I be calling it when I'm appending a frame? If so, then I'll need to keep track of how many frames have been appended so far in order to calculate the correct StreamTime.

> Also, you're not calling EndTrack() or Finish(). There needs to be some
> shutdown code.

Fixed!

> > +void
> > +MediaEngineDefaultVideoSource::GetUUID(nsAString& aUUID)
> > +{
> > +  aUUID.Assign(NS_LITERAL_STRING("1041FCBD-3F12-4F7B-9E9B-1EC556DD5676"));
> 
> Hmm. Is aUUID always going to be a real UUID? If it is, we should probably
> use nsID instead of a string.

I'm not sure, on Desktop it will always be a real UUID since we're going to be using the GIPS engine (and that's what it returns); but this may not hold true for B2G and Android. Fabrice, any thoughts?

> > +  nsRefPtr<layers::Image> image = mImageContainer->CreateImage(&format, 1);
> > +  
> > +  mImage = static_cast<layers::PlanarYCbCrImage*>(image.get());
> > +  PRUint8* frame = (PRUint8*) PR_Calloc((WIDTH * HEIGHT) / 2, 3);
> 
> Can you use something other than green? It might be helpful since then we
> can tell the difference between the default video source and something
> that's erroneously using all zeroes.

Yes, that's a good idea; I changed it to gray.

> > +private:
> > +  nsAutoPtr<MediaEngineVideoSource> mVsource;
> > +  nsAutoPtr<MediaEngineAudioSource> mAsource;
> 
> mVSource, mASource
> 
> You've got a lifetime management problem here.
> EnumerateVideoDevices/EnumerateAudioDevices is transferring membership to
> the caller and nulling out mVSource/mASource. Maybe you shouldn't keep
> references here and just create them in the Enumerate methods?

These are now nsRefPtrs and the fallback engine uses the thread safe version of nsISupports.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 14:53:29 PDT
(In reply to Anant Narayanan [:anant] from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > You're not calling AdvanceKnownTracksTime anywhere. You need to.
> 
> I'm not sure I understand AdvanceKnownTracksTime correctly. Does it need to
> be called with StreamTime(0) before adding the first track on a stream? In
> the case of this fallback engine, the stream always only has a single track,
> so should I be calling it when I'm appending a frame? If so, then I'll need
> to keep track of how many frames have been appended so far in order to
> calculate the correct StreamTime.

AdvanceKnownTracksTime(T) is basically a promise that you won't add any new tracks that start before time T.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 14:54:43 PDT
Comment on attachment 626127 [details] [diff] [review]
Fallback Media Engine - v3

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

You're not ending the audio track. Also you need to make sure Finish is called after both tracks have ended.

::: content/media/MediaEngineDefault.cpp
@@ +253,5 @@
> +  segment.Init(CHANNELS);
> +  segment.InsertNullDataAtStart(1);
> +
> +  // We never add another track to the stream, so we can always advance?
> +  // mSource->AdvanceKnownTracksTime(ticksSoFar * RATE);

Uncomment this :-)
Comment 8 [:fabrice] Fabrice Desré 2012-05-22 18:20:29 PDT
Comment on attachment 626127 [details] [diff] [review]
Fallback Media Engine - v3

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

::: content/media/MediaEngineDefault.cpp
@@ +87,5 @@
> +  
> +  int len = ((WIDTH * HEIGHT) * 3 / 2);
> +  mImage = static_cast<layers::PlanarYCbCrImage*>(image.get());
> +  PRUint8* frame = (PRUint8*) PR_Malloc(len);
> +  memset(frame, 0x80, len); // Gray

why not change the color so that we get the feeling of something actually animating, not just a static gray image?
Comment 9 Anant Narayanan [:anant] 2012-05-24 14:21:37 PDT
Created attachment 626965 [details] [diff] [review]
Fallback Media Engine - v3.1

This one should be close to the finish line!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> You're not ending the audio track. Also you need to make sure Finish is
> called after both tracks have ended.

Each stream only has one track. The VideoEngine returns a stream with one track, independent of another stream returned by the AudioEngine.

For the DOM binding to be able to use this fallback engine when the caller requests both audio and video, we will need to merge both streams into one. An earlier version of this patch had that (in the form of the nsDOMMediaStreamHelper) but since we got rid of that object, I'll open a new bug to make sure we add that functionality into nsDOMMediaStream itself (a constructor that takes two streams as arguments probably makes most sense?)

(In reply to Fabrice Desré [:fabrice] from comment #8)
> why not change the color so that we get the feeling of something actually
> animating, not just a static gray image?

Yup, I had considered that; but I also want to do white/pink noise for audio, since silence doesn't really tell us things are working correctly. I think we should do both in a followup bug.
Comment 10 Anant Narayanan [:anant] 2012-05-24 14:34:04 PDT
(In reply to Anant Narayanan [:anant] from comment #9)
> For the DOM binding to be able to use this fallback engine when the caller
> requests both audio and video, we will need to merge both streams into one.
> An earlier version of this patch had that (in the form of the
> nsDOMMediaStreamHelper) but since we got rid of that object, I'll open a new
> bug to make sure we add that functionality into nsDOMMediaStream itself (a
> constructor that takes two streams as arguments probably makes most sense?)

Bug 758391.
Comment 11 Suhas 2012-05-24 14:48:13 PDT
Comment on attachment 626965 [details] [diff] [review]
Fallback Media Engine - v3.1

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

::: content/media/MediaEngineDefault.cpp
@@ +154,5 @@
> +  VideoSegment segment;
> +
> +  nsRefPtr<layers::PlanarYCbCrImage> image = mImage;
> +  segment.AppendFrame(image.forget(), USECS_PER_S / FPS, gfxIntSize(WIDTH, HEIGHT));
> +  mSource->AppendToTrack(TRACK_VIDEO, &segment);

TRACK_VIDEO should be mTrackID ?
Comment 12 Anant Narayanan [:anant] 2012-05-24 15:13:59 PDT
Created attachment 626989 [details] [diff] [review]
Fallback Media Engine - v3.2

(In reply to Suhas from comment #11)
> > +  nsRefPtr<layers::PlanarYCbCrImage> image = mImage;
> > +  segment.AppendFrame(image.forget(), USECS_PER_S / FPS, gfxIntSize(WIDTH, HEIGHT));
> > +  mSource->AppendToTrack(TRACK_VIDEO, &segment);
> 
> TRACK_VIDEO should be mTrackID ?

Indeed, nice catch! I should have removed the #DEFINE from the top, which is why the code was still working but was wrong.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-24 15:20:39 PDT
(In reply to Anant Narayanan [:anant] from comment #9)
> For the DOM binding to be able to use this fallback engine when the caller
> requests both audio and video, we will need to merge both streams into one.
> An earlier version of this patch had that (in the form of the
> nsDOMMediaStreamHelper) but since we got rid of that object, I'll open a new
> bug to make sure we add that functionality into nsDOMMediaStream itself (a
> constructor that takes two streams as arguments probably makes most sense?)

I'm working on something just like that :-).
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-24 15:24:41 PDT
Comment on attachment 626989 [details] [diff] [review]
Fallback Media Engine - v3.2

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

::: content/media/MediaEngineDefault.cpp
@@ +107,5 @@
> +  mImage->SetData(data);
> +  PR_Free(frame);
> +
> +  // Set start time to 0
> +  mSource->AdvanceKnownTracksTime(0);

You're still not advancing the known-tracks-time.

You can just call AdvanceKnownTracksTime(STREAM_TIME_MAX) here and that'll take care of it.

@@ +209,5 @@
> +
> +  mSource = aStream;
> +
> +  // Set start time to 0
> +  mSource->AdvanceKnownTracksTime(0);

Same as above.

You're not setting the start time. AdvanceKnownTracksTime(N) means that you guarantee there are no new tracks before time N. So actually you should call AdvanceKnownTracksTime(STREAM_TIME_MAX) *after* you've added your track.

@@ +260,5 @@
> +}
> +
> +void
> +MediaEngineDefault::EnumerateVideoDevices(
> +  nsTArray<nsRefPtr<MediaEngineVideoSource> >* aVSources) {

all on one line

@@ +268,5 @@
> +}
> +
> +void
> +MediaEngineDefault::EnumerateAudioDevices(
> +  nsTArray<nsRefPtr<MediaEngineAudioSource> >* aASources) {

all on one line
Comment 15 Anant Narayanan [:anant] 2012-05-24 15:48:42 PDT
Created attachment 626997 [details] [diff] [review]
Fallback Media Engine - v3.3

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> You're not setting the start time. AdvanceKnownTracksTime(N) means that you
> guarantee there are no new tracks before time N. So actually you should call
> AdvanceKnownTracksTime(STREAM_TIME_MAX) *after* you've added your track.

Ah, thanks; I didn't know you could advance all the way until the end. I think I understand the purpose of the call now.
Comment 16 Anant Narayanan [:anant] 2012-05-30 22:50:33 PDT
When we a picture interface in bug 750943, this fallback engine will need to be updated to return a picture when asked.
Comment 17 Anant Narayanan [:anant] 2012-05-31 20:46:07 PDT
Created attachment 629037 [details] [diff] [review]
Fallback Media Engine - v3.3.1

Minor change to return NS_ERROR_NOT_IMPLEMENTED when asked for Snapshot(), which we added to the interface in bug 750943.
Comment 18 Anant Narayanan [:anant] 2012-06-01 01:12:37 PDT
Created attachment 629101 [details]
Fallback Media Engine - v3.3.2

Use an nsIDOMFile instead of a nsILocalFile.
Comment 19 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-06-02 14:32:59 PDT
Comment on attachment 629101 [details]
Fallback Media Engine - v3.3.2

Reviewed the minor differences from the previous patch: The changes are good; carrying forward roc's r+
Comment 21 Phil Ringnalda (:philor, back in August) 2012-06-03 12:40:34 PDT
https://hg.mozilla.org/mozilla-central/rev/3d74c0a1e172

Note You need to log in before you can comment on or make changes to this bug.