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

Status: VERIFIED FIXED

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

Description

[Suhas]

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

[Randell Jesup [:jesup] (needinfo me)]

Related to the bug for forwarding streams: bug 931903

Comment 2

[Suhas]

Attachment: Bug932845.patch - Add hints to support non GUM MediaStreams in Peer Connection

Comment 3

[Suhas]

Attachment: pc_audio_mozcapture.html

HTML page to allow passing MediaStream from <audio> to PeerConnection

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 5

[Suhas]

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)

Comment 6

[Suhas]

Attachment: Support hints for non gUM mediastreams in Debug only mode

Comment 7

[Suhas]

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

Comment 8

[Suhas]

https://tbpl.mozilla.org/?tree=Try&rev=6f4943293973

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 10

[Suhas]

Attachment: Support hints for non gUM mediastreams in Debug only mode

Taking r+ from Jesup

Comment 11

[Suhas]

Attachment: Support hints for non gUM mediastreams in Debug only mode

Comment 12

[Suhas]

Comment on attachment 8344261 [details] [diff] [review]
Support hints for non gUM mediastreams in Debug only mode

Taking r+ from Randell

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26aa24dc9581

Comment 15

[Petruta Horea [:phorea], Desktop QA]

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)

Comment 16

[Suhas]

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.

Comment 17

[Petruta Horea [:phorea], Desktop QA]

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.

Comment 18

[Suhas]

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

Comment 19

[u279076]

Needinfo Petruta to answer comment 18.

Comment 20

[Petruta Horea [:phorea], Desktop QA]

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

Comment 21

[Suhas]

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.