Closed Bug 752351 Opened 12 years ago Closed 12 years ago

Implement fallback MediaEngine backend

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
blocking-kilimanjaro ?
blocking-basecamp -

People

(Reporter: anant, Assigned: anant)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 7 obsolete files)

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.
Attached patch Fallback Media Engine - v1 (obsolete) — Splinter Review
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.
Assignee: nobody → anant
Status: NEW → ASSIGNED
Attachment #623572 - Flags: review?(rjesup)
Attachment #623572 - Flags: feedback?(fabrice)
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
Attachment #623572 - Flags: feedback?(fabrice)
Attached patch Fallback Media Engine - v2 (obsolete) — Splinter Review
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.
Attachment #623572 - Attachment is obsolete: true
Attachment #623572 - Flags: review?(rjesup)
Attachment #625327 - Flags: review?(roc)
Attachment #625327 - Flags: feedback?(fabrice)
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?
Attached patch Fallback Media Engine - v3 (obsolete) — Splinter Review
(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.
Attachment #625327 - Attachment is obsolete: true
Attachment #625327 - Flags: review?(roc)
Attachment #625327 - Flags: feedback?(fabrice)
Attachment #626127 - Flags: review?(roc)
Attachment #626127 - Flags: feedback?(fabrice)
(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 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 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?
Attachment #626127 - Flags: feedback?(fabrice) → feedback+
Attached patch Fallback Media Engine - v3.1 (obsolete) — Splinter Review
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.
Attachment #626127 - Attachment is obsolete: true
Attachment #626127 - Flags: review?(roc)
Attachment #626965 - Flags: review?(roc)
(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 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 ?
Attached patch Fallback Media Engine - v3.2 (obsolete) — Splinter Review
(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.
Attachment #626965 - Attachment is obsolete: true
Attachment #626965 - Flags: review?(roc)
Attachment #626989 - Flags: review?(roc)
(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 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
Attached patch Fallback Media Engine - v3.3 (obsolete) — Splinter Review
(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.
Attachment #626989 - Attachment is obsolete: true
Attachment #626989 - Flags: review?(roc)
Attachment #626997 - Flags: review?(roc)
When we a picture interface in bug 750943, this fallback engine will need to be updated to return a picture when asked.
Attached patch Fallback Media Engine - v3.3.1 (obsolete) — Splinter Review
Minor change to return NS_ERROR_NOT_IMPLEMENTED when asked for Snapshot(), which we added to the interface in bug 750943.
Use an nsIDOMFile instead of a nsILocalFile.
Attachment #626997 - Attachment is obsolete: true
Attachment #629037 - Attachment is obsolete: true
Attachment #629101 - Flags: review?(roc)
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
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+
Attachment #629101 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/3d74c0a1e172
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 768924
blocking-basecamp: ? → -
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.