Closed
Bug 932845
Opened 11 years ago
Closed 11 years ago
Non GUM MediaStreams added to Peer Connections fails due to missing hints
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
firefox28 | --- | verified |
People
(Reporter: snandaku, Assigned: snandaku)
References
Details
Attachments
(2 files, 3 obsolete files)
2.88 KB,
text/html
|
Details | |
3.15 KB,
patch
|
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
Peer Connection should accept media streams coming from non GUM sources such as the example below using captureStreamUntilEnded() audio-elem.src = "input.wav" audioStream = audio-elem.mozCaptureStreamUntilEnded() audio-elem.play() peerconnection.addStream(audioStream) peerconnection.createOffer(...) As of today createOffer() fails with error "SDP cannot be created without any streams" due to dependency on the Hints Mechanism for the stream acquired via GUM.
Comment 1•11 years ago
|
||
Related to the bug for forwarding streams: bug 931903
Attachment #8337009 -
Attachment description: Bug932845.patch → Bug932845.patch (WIP)
Attachment #8337009 -
Attachment description: Bug932845.patch (WIP) → Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection
Attachment #8337009 -
Flags: review?(rjesup)
HTML page to allow passing MediaStream from <audio> to PeerConnection
Comment 4•11 years ago
|
||
Comment on attachment 8337009 [details] [diff] [review] Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection Review of attachment 8337009 [details] [diff] [review]: ----------------------------------------------------------------- Almost r+, but I want us to consider alternatives to doing that hinting in DEBUG always. ::: content/html/content/src/HTMLMediaElement.cpp @@ +1748,5 @@ > > already_AddRefed<DOMMediaStream> > HTMLMediaElement::CaptureStreamInternal(bool aFinishWhenEnded) > { > + uint8_t hints = 0; Move hints into the ifdef @@ +1756,5 @@ > } > +#ifdef DEBUG > + // Estimate hints based on the type of media element. > + // TODO: Revisit this once hints mechanism is dealt with > + // holistically. Include bug number (932845) @@ +1768,3 @@ > > OutputMediaStream* out = mOutputStreams.AppendElement(); > + out->mStream = DOMMediaStream::CreateTrackUnionStream(window, hints); Move into the ifdef, and use a #else without the hints, so the opt/release code isn't affected. I'd prefer if we could only do this if a specific pref was on and we'd use that pref in the audio quality tests. Lets see if that's possible before landing a kludge in debug like this. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +743,5 @@ > switch (chunk.mBufferFormat) { > case AUDIO_FORMAT_FLOAT32: > + { > + const float* buf = static_cast<const float *>(chunk.mChannelData[0]); > + ConvertAudioSamplesWithScale(buf, static_cast<int16_t*>(samples), chunk.mDuration, chunk.mVolume); Why is the static_cast needed here? samples should match int16_t* --to force it to use the right template? What error happens if it's left off?
Attachment #8337009 -
Flags: review?(rjesup)
Comment on attachment 8337009 [details] [diff] [review] Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection Review of attachment 8337009 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMediaElement.cpp @@ +1748,5 @@ > > already_AddRefed<DOMMediaStream> > HTMLMediaElement::CaptureStreamInternal(bool aFinishWhenEnded) > { > + uint8_t hints = 0; Sure, will do @@ +1756,5 @@ > } > +#ifdef DEBUG > + // Estimate hints based on the type of media element. > + // TODO: Revisit this once hints mechanism is dealt with > + // holistically. Will do @@ +1768,3 @@ > > OutputMediaStream* out = mOutputStreams.AppendElement(); > + out->mStream = DOMMediaStream::CreateTrackUnionStream(window, hints); Agreed. Will add a preference media.enable.mozCaptureHints for this purpose and move this fix into the preference as suggested ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +743,5 @@ > switch (chunk.mBufferFormat) { > case AUDIO_FORMAT_FLOAT32: > + { > + const float* buf = static_cast<const float *>(chunk.mChannelData[0]); > + ConvertAudioSamplesWithScale(buf, static_cast<int16_t*>(samples), chunk.mDuration, chunk.mVolume); If we don't cast it by hand the compiler bails out with the following error not being able to find the right function to choose. /Users/suhasnandakumar/cisco/webrtc/firefox/mc_3/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:748:11: error: no matching function for call to 'ConvertAudioSamplesWithScale' 0:16.54 ConvertAudioSamplesWithScale(buf, samples, chunk.mDuration, chunk.mVolume); 0:16.54 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0:16.54 /Users/suhasnandakumar/cisco/webrtc/firefox/mc_3/media/webrtc/signaling/../../../content/media/AudioSampleFormat.h:120:1: note: candidate function not viable: no known conversion from 'const float *' to 'const int16_t *' (aka 'const short *') for 1st argument 0:16.54 ConvertAudioSamplesWithScale(const int16_t* aFrom, int16_t* aTo, int aCount, float aScale) 0:16.54 ^ 0:16.54 /Users/suhasnandakumar/cisco/webrtc/firefox/mc_3/media/webrtc/signaling/../../../content/media/AudioSampleFormat.h:109:1: note: candidate template ignored: failed template argument deduction 0:16.54 ConvertAudioSamplesWithScale(const From* aFrom, To* aTo, int aCount, float aScale)
Attachment #8337009 -
Attachment is obsolete: true
Comment on attachment 8343666 [details] [diff] [review] Support hints for non gUM mediastreams in Debug only mode Review of attachment 8343666 [details] [diff] [review]: ----------------------------------------------------------------- Incorporated comments from Randell
Attachment #8343666 -
Flags: review?(rjesup)
Comment 9•11 years ago
|
||
Comment on attachment 8343666 [details] [diff] [review] Support hints for non gUM mediastreams in Debug only mode Review of attachment 8343666 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMediaElement.cpp @@ +1765,5 @@ > OutputMediaStream* out = mOutputStreams.AppendElement(); > +#ifdef DEBUG > + // Estimate hints based on the type of the media element, > + // Bug932845: Revisit this once hints mechanism is dealt with > + // holistically. I would just add one more comment about *why* this is being done -- i.e. what this enables, and why we only need it on a debug build.
Attachment #8343666 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Taking r+ from Jesup
Attachment #8343666 -
Attachment is obsolete: true
Attachment #8344260 -
Flags: checkin?(rjesup)
Attachment #8344260 -
Flags: checkin?(rjesup)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8344260 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8344261 [details] [diff] [review] Support hints for non gUM mediastreams in Debug only mode Taking r+ from Randell
Attachment #8344261 -
Flags: checkin?(rjesup)
Updated•11 years ago
|
Attachment #8344261 -
Flags: checkin?(rjesup) → checkin+
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/26aa24dc9581 Please make sure future patches have your name included in the commit information. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/26aa24dc9581
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 15•10 years ago
|
||
Is there a way QA can verify this is fixed? Using the page from attachment, the following error is displayed "Something Failed [object Object]" on both Nightly 2013-10-30 and Firefox 28 beta 4. Only this warning is shown in the console: "HTTP load failed with status 404. Load of media resource https://bug932845.bugzilla.mozilla.org/test.ogg failed." Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0 (20140218122424)
Flags: needinfo?(snandaku)
Assignee | ||
Comment 16•10 years ago
|
||
Please try loading this web-page: http://suhashere.github.io/try-webrtc/pc_video_mozcapture.html. This page tries to load a video file into Peer-Connection using mozCaptureStream*(). The fix in this patch should make the video work correctly.
Flags: needinfo?(snandaku)
Comment 17•10 years ago
|
||
Suhas, using the link I have the same behavior in Nightly 28 2013-10-30 as in Firefox 28 beta 6: The pop-up "Something Failed [object Object]" is displayed on the screen after selecting Start. The third video is played and there are no errors on the console. After selecting OK, "Failure callback: [object Object]" is displayed below the Stop button. None of the videos is played if pressing their own start button.
Assignee | ||
Comment 18•10 years ago
|
||
Hello Petruta Please try http://suhashere.github.io/try-webrtc/pc_audio_mozcapture.html. There are somethings that are needed to verify this fix 1. You need a DEBUG build of the firefox 2. The preference media.capturestream_hints.enabled must be set to true The reason the video test failed was due to sampling frequency of the audio was 441000 which we don't support in our code. If you run the above audio test with steps 1 and 2 followed, you should see local audio file (input.wav) feeding to Peer Connection remote <audio> element. Please let me know Thanks Suhas
status-firefox28:
--- → fixed
Comment 20•10 years ago
|
||
Thanks for the details, Suhas. I added the preference and the results were the ones described in comment 17 for the Nightly debug build 2013-10-29. Using the beta debug build from 2014-03-01 I've got the following text after pressing Start: pc2 got remote stream from pc1 addstream pc1 got remote stream from pc2 addstream HIP HIP HOORAY The first audio was playing a sound (the time control is placed at the end), nothing happens with the second audio and the third one is played for 10 seconds. I believe that this is the correct behavior, so I'm marking this bug as verified. Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Status: RESOLVED → VERIFIED
Flags: needinfo?(petruta.rasa)
QA Contact: petruta.rasa
Assignee | ||
Comment 21•10 years ago
|
||
Hello Petruta That's correct. The second audio element doesn't play anything to avoid feedback audio. The javascript code has commented out the play() method on the second audio element purposefully. Thanks for verifying.
You need to log in
before you can comment on or make changes to this bug.
Description
•