966247 - Setting volume on a media element does not work when the media element is going to the MediaStreamGraph

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Paul Adenot (:padenot)]

The playbackRate property of the HTMLMediaElement has the signal processing bit implemented at the AudioStream level, to minimize latency between the setting of the property and the actual effect on the sound.

This was okay before, but because we can now capture a stream (mozCaptureStream{,UntilEnded}, and MediaElementAudioSourceNode) without directly touching the AudioStream, the playbackRate is ignored in those cases.

It would be nice to be able to have the playbackRate property working regardless of the mean we use to output the sound.

Comment 1

[Sergio]

The audio element's volume is also ignored in this situation.
Tested on the audio element controls with Fx 28 (currently aurora).

Comment 2

[Paul Adenot (:padenot)]

Indeed, and for the exact same reasons, thanks. This bug is now about both the volume and the playbackRate.

Comment 3

[François Freitag]

Hi Paul,

We've met on Tuesday.
I would like to contribute to the Mozilla codebase.
Is it ok for me to take this bug ?

Comment 4

[François Freitag]

Attachment: bug966247.patch (volume)

I've written a small patch to correct the behaviour of the volume for the media.

Would you please review it?

Comment 5

[Paul Adenot (:padenot)]

(In reply to François Freitag from comment #4)
> Created attachment 8378621 [details] [diff] [review]
> bug966247.patch (volume)
> 
> I've written a small patch to correct the behaviour of the volume for the
> media.
> 
> Would you please review it?

When you want someone to review something, you need to set the review flag:
- When attaching your patch, click the "Review" dropdown, set it to "?", and put the name of the person in the box (here, :padenot, if you want to request the review from me).
- If your patch is already attached, click "Details" on the attachment, and do the same steps.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8378621 [details] [diff] [review]
bug966247.patch (volume)

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

You have a bunch of stuff in the patch that has apparently been added by mistake. This patch should only consist in the changes to MediaDecoderStateMachine.cpp. Can you modify the patch to only include the relevant part, and attach it again, a request a review from me?

Otherwise, looks good! This needs a test, and then we can proceed to the playbackRate part. iirc, you already have a mochitest for this. You can attach it as a second patch, or put it in the same patch, as you please.

Could you also make the commit message a bit clearer? Something along the lines of:

> "Bug 966247 - Make MediaElementAudioSourceNodes take the HTMLMediaElement's volume into account. r=padenot".

Comment 7

[François Freitag]

Attachment: bug966247.patch (volume) v2

Here is the cleaned version of the patch with the test.

Thank you for the review!

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8379148 [details] [diff] [review]
bug966247.patch (volume) v2

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

Looks good, can you please address the comment, and I'll push this patch to the try servers to make sure we didn't overlook something ?

Then I can commit this part to mozilla-central (as it is independent) and you can start working on the playbackRate part.

::: content/media/webaudio/test/test_bug966247.html
@@ +23,5 @@
> +  var measn = ac.createMediaElementSource(a);
> +  var sp = ac.createScriptProcessor();
> +  ps.onaudioprocess = function(e) {
> +    var inputBuffer = e.inputBuffer.getChannelData(0);
> +    ok(isSilent(inputBuffer), "The volume is supposed set to 0, so all the elements of the buffer are supposed to be equal to 0.0");

s/is supposed set/is set to 0/

@@ +28,5 @@
> +  }
> +  // Connect the MediaElementAudioSourceNode to the ScriptProcessorNode to check
> +  // the audio volume.
> +  measn.connect(sp);
> +  a.play();

I expect this test to work most of the time, but it should be converted to an asynchronous test.

Nothing big, just add SimpleTest.waitForExplicitFinish() at the start, like so [1], and after you call ok(), explicitly finish the test [2].

This will ensure that the callback with the ok() runs all the time: because we do the check in a callback, and because Mochitests finish when the script finishes (so before the callback has been ran), we should be extra careful here.

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/test/test_oscillatorNode.html?force=1#13
[2]: http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/test/test_oscillatorNode.html?force=1#52

Comment 9

[François Freitag]

Attachment: bug966247.patch (volume) v3

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8379782 [details] [diff] [review]
bug966247.patch (volume) v3

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

I've made the modification I suggested, and pushed to our try servers (so the patch can be tested on all platforms).

https://tbpl.mozilla.org/?tree=Try&rev=463885aca2be

r+ with the modification.

::: content/media/webaudio/test/test_bug966247.html
@@ +31,5 @@
> +  // the audio volume.
> +  measn.connect(sp);
> +  a.play();
> +
> +  SimpleTest.finish();  

This needs to be next to the ok() call, otherwise it's useless :-)

Comment 11

[Paul Adenot (:padenot)]

Hrm, for some reason I completely missed the fact that the new test was not put in mochitest.ini, hence not running.

Pushed again to try: https://tbpl.mozilla.org/?tree=Try&rev=3a72f7063efa

Comment 12

[Paul Adenot (:padenot)]

François do you have time to finish this ? If not, I have a couple students from ENSIMAG that can take this to the finish line, just let me know.

Comment 13

[François Freitag]

Paul,
I think it's better if the other students could finish this bug as I can't take any time to work on it now.

Thanks again for your support.

Comment 14

[Paul Adenot (:padenot)]

Clément is going to take this to the finish line.

Comment 15

[Clément Geiger [:cgg]]

Attachment: Make MediaElementAudioSourceNode take the HTMLMediaElement volume into account.

Voilà, I finished your patch about the volume; let's get on with the playback rate now!

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8444068 [details] [diff] [review]
Make MediaElementAudioSourceNode take the HTMLMediaElement volume into account.

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

Looks good, thanks !

Comment 17

[Sergio]

Hi! I see version 35 didn't get this fix, any idea when the fix should be released?

Thanks!

Comment 18

[Tiago Katcipis]

I am having some real problems because of that, I am unable to use WebAudio API to apply gain to the audio and also have playback rate working together (and I really need the two of them together :-().

Is there anything I can do to help this move forward ? I am willing to fix the playback rate, but it seems that even the fix for volume has not been applied yet (and it seems to have been accepted already, some time ago).

Comment 19

[Paul Adenot (:padenot)]

I'll apply the fix for volume now, and I'm happy to provide guidance to fix the playbackRate. Or maybe Clément want another try?

Comment 20

[Tiago Katcipis]

Paul, if you can guide me I will be happy to fix it.

Comment 21

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59240f2df2c2

Comment 22

[Tiago Katcipis]

I will take a look on the MediaDecoderStateMachine then.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59240f2df2c2

Comment 24

[Paul Adenot (:padenot)]

(In reply to Tiago Katcipis from comment #22)
> I will take a look on the MediaDecoderStateMachine then.

Yes. You can take a look at AudioStream.cpp, AudioStream::DataCallback to see how we implement the playbackRate change.

Comment 25

[Tiago Katcipis]

I have finally made some time to work a little on this problem. Since my main problem is using playback rate together with the gain filter I'm trying to reproduce the problem on a very simple isolated application.

Sadly, even the most basic example of the gain filter does not seem to be working on Firefox. Here is the gist:

https://gist.github.com/katcipis/c4610223c369508dbe1b

It loads the page but nothing happens when I try to play. If I comment out the gain filter code it works. 

I thought that it was something wrong with the code, but there is nothing on the console and it works fine on chrome. Anyone can help me on what I'm doing wrong ?

I based myself on this code:

https://github.com/mdn/media-source-buffer

It is from mdn, but sadly it works only on chrome too.

I'm using Linux Mint with Firefox 34

Comment 27

[Ludwig Fiolka]

Still reproduced on Firefox 47.0 (see http://stackoverflow.com/questions/37830502/video-playbackrate-not-working-on-firefox-when-using-htmlmediaelementsourcenod/37918759#37918759)

Anything new regarding this issue ?

Comment 29

[richardzhe]

Hi all,

Have we got any update on this? I still have such problem (FF 52.0) where setting playbackRate does not work when Web Audio API is used (AudiContext.createMediaElementSource(audio)). Just wondering if this is ever going to be fixed or not.

Thanks

Comment 30

[Tim Nguyen :ntim]

Paul, can you provide more details? I'd love to give a stab at this as this is preventing my project (https://nt1m.github.io/media-player/)  from working properly in Firefox.

Comment 31

[Paul Adenot (:padenot)]

JW, what would be the best way to do this at the moment ? We need to be able to apply the playbackRate when an HTMLMediaElement is being piped to a MediaStreamGraph.

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/media/mediasink/DecodedStream.h#33

We will need to overhaul DecodedStream which is responsible for sending audio/video frames to a media stream.

To support playbackRate rate changes in video is simple. We just need to modify the duration of each frame (duration /= playbackRate) before sending it to the media stream.

We have more work to do when it comes to audio. We will need to employ SoundTouch (which is also used by AudioStream) to stretch audio frames in response to playbackRate changes.

The biggest issue is the latency in playbackRate changes introduced by the push-model of DecodedStream. DecodedStream pushes audio/video frames to the media stream immediately after the frames are received by MDSM. So if the amount of decode-ahead is 10s, the change in playbackRate will take effect 10s later.

One solution is to change the push-model to pull-model to minimize the latency (which we have done for AudioStream!). However it requires rewriting most of the code of DecodedStream which is certainly not trivial.

Comment 33

[krazhtest]

«We need to be able to apply the playbackRate when an HTMLMediaElement is being piped to a MediaStreamGraph.»

Took me a while to find this issue.
We are looking forward for a workaround.
Thank you guys.

Comment 34

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 35

[esterTion]

So... This has surely been here for a while. 
I just implemented a custom volume control for video, to allow audio amplification. And then I found out playbackRate no longer work in firefox. 
Safari is pretty buggy with playbackRate, too. Seems only chrome can handle it properly.

Comment 36

[Paul Adenot (:padenot)]

PlaybackRate should _not_ work per spec, per [0]. I'm closing this.

[0]: https://w3c.github.io/mediacapture-main/#mediastreams-in-media-elements

Comment 37

[Tim Nguyen :ntim]

(In reply to Paul Adenot (:padenot) from comment #36)
> PlaybackRate should _not_ work per spec, per [0]. I'm closing this.
> 
> [0]: https://w3c.github.io/mediacapture-main/#mediastreams-in-media-elements

Does that mean Chrome has the incorrect behaviour ?

Comment 38

[Paul Adenot (:padenot)]

Yes.

Comment 39

[Kaiido]

I am sorry I didn't read the whole document again, but I fail to see how is the part you linked to relevant to the current bug.

Isn't this bug about the fact that we can't set the playbackRate of the *source* MediaElement of a MediaStream?
The paragraph from the specs you linked to only talks about MediaElements that do *consume* this MediaStream, i.e, the one playing a MediaStream (wherever this stream comes from).

If it is in some other place of this document, then could you point exactly to it please?

And if I misunderstood this bug report, then should I open a new one? Because the behavior in the provided fiddle[1] still sounds like a bug to me.

[1](https://jsfiddle.net/o59r1ks4/)

Comment 40

[Paul Adenot (:padenot)]

You're right I confused the two. This is specced by [0] (at the end of the description of the method).

[0]: https://w3c.github.io/mediacapture-fromelement/#dom-htmlmediaelement-capturestream

Comment 41

[Caleb Sacks]

Paul, if playbackRate should not work, then how else do you modify the playback rate for a MediaElementAudioSourceNode? Unlike AudioBufferSourceNode [0], MediaElementAudioSourceNode does not implement playbackRate.

[0]: https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/playbackRate

Comment 42

[Tim Nguyen :ntim]

Going to split playbackRate out to its own bug, since it's quite confusing to have one part (volume) land in Firefox 36, and the other land ages later.

Comment 43

[Julian P.]

I'm facing the same problem. Changing the playbackRate to 2.0 works with Chrome and Opera, but not with Firefox. To find a solution, I created a post on stackoverflow:
https://stackoverflow.com/questions/57391854/how-to-set-playbackrate-of-htmlaudioelement-in-combination-with-a-sourcenode-and

I don't understand why the playbackRate can't be changed when I just changed the volume with the Web audio API. Also I tried to find the proof that changing playbackRate is not supported in the specs.

Comment 44

[Tim Nguyen :ntim]

(In reply to Julian P. from comment #43)

I'm facing the same problem. Changing the playbackRate to 2.0 works with Chrome and Opera, but not with Firefox. To find a solution, I created a post on stackoverflow:
https://stackoverflow.com/questions/57391854/how-to-set-playbackrate-of-htmlaudioelement-in-combination-with-a-sourcenode-and

I don't understand why the playbackRate can't be changed when I just changed the volume with the Web audio API. Also I tried to find the proof that changing playbackRate is not supported in the specs.

This bug is closed and is about making the volume work. Please comment on bug 1517199, which is more relevant.