Closed Bug 843214 Opened 7 years ago Closed 7 years ago

getUserMedia - Request video for video #1, then request video/audio in video #2 in same/different tab - video #2 shows no camera/mic stream

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox20 + verified
firefox21 + fixed
firefox22 + fixed

People

(Reporter: jsmith, Assigned: jesup)

Details

(Whiteboard: [getUserMedia][blocking-gum+])

Attachments

(3 files)

Attached file Testcase Zip File
Steps:

1. Request access to gUM video for a video (select "Turn on local video 1 with gum video only")
2. Approve perms
3. Request access to gUM video/audio for a video (select "Turn on local video 2 with gum video/audio")
4. Approve perms

Expected:

Both videos should render from the same camera stream. The second video stream should additionally be integrated into your mic that you requested.

Actual:

The second video doesn't render a video stream from your camera and no mic integration occurs. I get a dead white video that does nothing.
This is a bit of an edge case - it only seems to happen in the video --> video+audio case in the same tab. Although the severity here is bad. Don't know if we want to block or not.
Whiteboard: [getUserMedia]
Turns out this isn't as much of an edge case as I initially thought - you can also reproduce this pretty easily by requesting video on tab #1, then audio & video on tab #2. So sounds like we don't handle the incremental case of multiple concurrent use.

Up to blocking for now, but I'll talk tomorrow about this one too.
Summary: getUserMedia - Request video for video #1, then request video/audio in video #2 in same tab - video #2 shows no camera/mic stream → getUserMedia - Request video for video #1, then request video/audio in video #2 in same/different tab - video #2 shows no camera/mic stream
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
###!!! ASSERTION: Bad track ID!: 'Error', file ../../../content/media/MediaStreamGraph.h, line 656
###!!! ASSERTION: Append to non-existent track!: 'Error', file ../../../content/media/MediaStreamGraph.cpp, line 1650
-578836736[42bf330]: Start
Comment on attachment 716957 [details] [diff] [review]
don't call NotifyPull until source has been Start()ed

I realize we're accessing this bool from another thread, though this is "safe" in that we're trying to block access until start(), so if the second thread sees the transition "late" it doesn't hurt anything (just as it wouldn't if the setting thread was delaying in calling listener->VideoStarted()), so locking shouldn't be needed.

Roc: any issues if a NotifyPull slips into the "gap" between Start() and VideoStarted(), and so we call audiosource->NotifyPull(...track 0...) but don't call videosource->NotifyPull(...track 1....)?  If there is, I can find some way to make this threadsafe (preferably without adding another lock access to every NotifyPull, but that's an option).

I'll note that the single NotifyPull that slipped in *before* Start()/AddTrack() and the assertion it caused (which this patch fixes) seemed to mess up video forever, perhaps due to how "delta" for each frame is calculated.
Attachment #716957 - Flags: review?(tterribe)
Attachment #716957 - Flags: feedback?(roc)
Assignee: nobody → rjesup
Priority: -- → P1
Comment on attachment 716957 [details] [diff] [review]
don't call NotifyPull until source has been Start()ed

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

So, I don't think this is really thread safe, as there's nothing establishing a "happens before" relationship between setting the flags and checking them (technically this is okay for audio, since the video source's AddTrack() call will do the trick, but that's pure luck... we could fix the problem by doing audio and video in the other order since audio does not currently use NotifyPull(), but I think I threw up in my mouth a little bit just suggesting that).

Really, this is caused by the fact that SourceMediaStream::AddTrack is designed to be called from somewhere other than the MSG thread, where NotifyPull runs, so the moment we have a source feeding multiple MediaStreamSources, we have this problem. We could check to see that MediaEngineWebRTCVideoSource::mSources.Contains(aSource) in NotifyPull(), but a) that means we're doing a bunch of work that the AppendToTrack already has to do, and b) would require more locking (some of which we already need to add because MediaEngineWebRTCVideoSource::mState is being checked from the MSG thread, but isn't always locked elsewhere), which AppendToTrack is also already doing. I think we should just make AppendToTrack return an error (which we would promptly ignore) instead of invoking NS_ERROR. Maybe we don't even need to return the error code, since AppendToTrack already silently discards the segment on a track that's ended. Either way, that would remove the need to checkMediaEngineWebRTCVideoSource:: mState in NotifyPull(), which would actually allow us to remove locking.
Attachment #716957 - Flags: review?(tterribe) → review-
We'll track for release as long as this is blocking-gum+ (which we should reconsider if left unfixed closer to FF20's release).
Ok, so an update:

Simple case: (all using the gum_test page): tab 1: video. tab 2: ask for video+audio. You get the underrun assertion (bug839650), and no video, and the streams appear perpetually blocked getting +-0 deltas for NotifyPull (sometimes -1, usually 0)

however, if you have tab 1: video, tab 2 audio, and ask for tab 3 audio+video, all is happy. 

The key is that a "freewheeling" stream causes an audio+video stream to block, unless there's another stream with audio (even a blocked one: tab 1: video, tab 2 video+audio (frozen/blocked), then tab 3: audio+video will work
Sorry, the assertion that fires is the one discussed above by derf (and which my original patch on this bug avoids) for a bad track ID (caused by a NotifyPull on the stream between Audio AddTrack time and video AddTrack time, and the gUM code wasn't noticing that *this* stream hadn't AddTrack'd yet)
I got around those track assertions by making AppendToTrack etc noops when the track doesn't exist yet.

What I'm seeing now is a series of NotifyPulls on the audio+video stream that seem to be doing approximately the right thing, e.g.:
14512[a574680]: Calling NotifyPull aStream=47f5280 t=0.245325 current end=0.239991
14512[a574680]: SourceMediaStream 47f5280 track 2, advancing end from 2296800 to 2296960
But we hardly ever get video appended in response to those NotifyPulls. I'll look into why.
Lack of video being appended by gum is almost always (in fact really the only reason) due to negative or zero "deltas" - aDesired time is earlier than a previous aDesired time, so we've already given a frame (or believe we have; it could well be a null frame) that covers the requested time period.  Basically, if you call NotifyPull, I'll append a frame (even if it's a dup) if delta is > 0.  The problem was presenting itself as zero/negative deltas *if* there's an existing stream with no audio track before I created an a+v stream

mediamanager:6 logging will show the NotifyPulls and deltas (plus other stuff)

I had a patch to disable the asserts also (in addition to the patch derf r-'d that avoided hitting the assertion); they didn't seem to help.
GetUserMediaCallbackMediaStreamListener::NotifyPull tries to track how much video has been appended in mLastEndTimeVideo. This gets compared to aDesiredTime (after converting from StreamTime to TrackTicks). The problem is, GetUserMediaCallbackMediaStreamListener::NotifyPull has done one or more AppendFrames that didn't actually append anything since the track didn't exist yet, but those AppendFrames were accounted for in mLastEndTimeVideo. So mLastEndTimeVideo is out of sync with the where the video track actually ends and things are broken.
(In reply to Timothy B. Terriberry (:derf) from comment #6)
> Really, this is caused by the fact that SourceMediaStream::AddTrack is
> designed to be called from somewhere other than the MSG thread, where
> NotifyPull runs, so the moment we have a source feeding multiple
> MediaStreamSources, we have this problem.

It would actually be OK to call AddTrack during NotifyPull --- if you make sure to delay calling AdvanceKnownTracksTime until then, too.
Attached patch one possible fixSplinter Review
This patch works. The key bit is that AppendToTrack returns whether or not data was actually appended, so MediaEngineWebRTCVideoSource::NotifyPull can track the end of the track correctly.

I think it would be better to avoid calling AppendToTrack when the track doesn't exist. If having the gUM code add the video track during NotifyPull helps, we should do that.
Comment on attachment 717773 [details] [diff] [review]
one possible fix

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

Also, if AppendToTrack is now "safe", we can remove "if (mState != kStart) return;"'s from NotifyPull() in video and Process() in Audio, as if they slip through (at the very start or end) and call AppendToTrack it will fail.

Calling AddTrack in NotifyPull requires some extra (locked) state on a per-source basis, and right now the code shares these objects among N source, so I'd have to store it in with mSources (or a parallel array), and access it on every NotifyPull just to check.  This patch is MUCH better from that aspect.  If we want to ensure AddTrack happens first, we should fix the threading issues Tim identified in my original patch.

::: content/media/MediaStreamGraph.cpp
@@ +1678,5 @@
>  {
>    MutexAutoLock lock(mMutex);
>    TrackData* data = FindDataForTrack(aID);
>    if (!data) {
> +    aSignalThread->Dispatch(aSignalRunnable, 0);

Aha, this was something I'd missed in my version of this patch (along with a few other bits), which I'd put aside temporarily in order to get a better understanding of what was happening in MSG first.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +123,5 @@
>    // Doing so means a negative delta and thus messes up handling of the graph
>    if (delta > 0) {
>      // NULL images are allowed
>      segment.AppendFrame(image ? image.forget() : nullptr, delta, gfxIntSize(mWidth, mHeight));
> +    if (aSource->AppendToTrack(aID, &(segment))) {

Add this:
    // This can fail is either a) we haven't added the track yet, or b)
    // we've removed or finished the track.
Attachment #717773 - Flags: review+
Attachment #716957 - Flags: feedback?(roc)
We'll want to request uplift as soon as this is in m-c and verified
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum+][webrtc-uplift]
Relanded the one roc landed for me (with a tab->space correction).  Note the cause of the win32 build failure was another patch in the group.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78cc3311ad3
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/e78cc3311ad3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
lgtm on Nightly 2/28 build
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment on attachment 717773 [details] [diff] [review]
one possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Camera sharing in December

User impact if declined: Failure to share camera between two tabs if video alone is requested first.  This could impact some applications that want to do "background" video capture, for example gesture/face recognition, or a filmstrip or gum applications.  If you try to make or accept a video call without stopping the video-only app, video capture will fail for the call.

Testing completed (on m-c, etc.): On m-c and tested for a while.


Risk to taking this patch (and alternatives if risky): Low.  Alternative is to Relnote that this may happen and that the user must end any video captures before attempting to use audio+video capture.


String or UUID changes made by this patch: none.
Attachment #717773 - Flags: approval-mozilla-beta?
Attachment #717773 - Flags: approval-mozilla-aurora?
Comment on attachment 717773 [details] [diff] [review]
one possible fix

Verified and early enough in beta cycle that we can take this low risk uplift. Please land today so it can ship in FF20 beta 3.
Attachment #717773 - Flags: approval-mozilla-beta?
Attachment #717773 - Flags: approval-mozilla-beta+
Attachment #717773 - Flags: approval-mozilla-aurora?
Attachment #717773 - Flags: approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0

Verified the fix on latest beta (Firefox 20 beta 3) builds.
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift] → [getUserMedia][blocking-gum+]
You need to log in before you can comment on or make changes to this bug.