Non GUM MediaStreams added to Peer Connections fails due to missing hints

VERIFIED FIXED in Firefox 28

Status

()

Core
WebRTC: Audio/Video
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Suhas, Assigned: Suhas)

Tracking

24 Branch
mozilla28
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox28 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 909524
Related to the bug for forwarding streams: bug 931903
(Assignee)

Comment 2

5 years ago
Created attachment 8337009 [details] [diff] [review]
Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection
(Assignee)

Updated

5 years ago
Assignee: nobody → snandaku
(Assignee)

Updated

5 years ago
Attachment #8337009 - Attachment description: Bug932845.patch → Bug932845.patch (WIP)
(Assignee)

Updated

5 years ago
Attachment #8337009 - Attachment description: Bug932845.patch (WIP) → Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection
Attachment #8337009 - Flags: review?(rjesup)
(Assignee)

Comment 3

5 years ago
Created attachment 8337212 [details]
pc_audio_mozcapture.html

HTML page to allow passing MediaStream from <audio> to PeerConnection
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)
(Assignee)

Comment 5

5 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]:
-----------------------------------------------------------------

::: 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)
(Assignee)

Comment 6

5 years ago
Created attachment 8343666 [details] [diff] [review]
Support hints for non gUM mediastreams in Debug only mode
Attachment #8337009 - Attachment is obsolete: true
(Assignee)

Comment 7

5 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]:
-----------------------------------------------------------------

Incorporated comments from Randell
Attachment #8343666 - Flags: review?(rjesup)
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

5 years ago
Created attachment 8344260 [details] [diff] [review]
Support hints for non gUM mediastreams in Debug only mode

Taking r+ from Jesup
Attachment #8343666 - Attachment is obsolete: true
Attachment #8344260 - Flags: checkin?(rjesup)
(Assignee)

Updated

5 years ago
Attachment #8344260 - Flags: checkin?(rjesup)
(Assignee)

Comment 11

5 years ago
Created attachment 8344261 [details] [diff] [review]
Support hints for non gUM mediastreams in Debug only mode
Attachment #8344260 - Attachment is obsolete: true
(Assignee)

Comment 12

5 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)
Attachment #8344261 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/26aa24dc9581
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Keywords: verifyme
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

4 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)
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

4 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
Needinfo Petruta to answer comment 18.
Flags: needinfo?(petruta.rasa)
status-firefox28: --- → fixed
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
status-firefox28: fixed → verified
Flags: needinfo?(petruta.rasa)
QA Contact: petruta.rasa
(Assignee)

Comment 21

4 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.