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

RESOLVED FIXED

Status

()

defect
P2
normal
Rank:
15
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: padenot, Assigned: cgg, Mentored)

Tracking

(Blocks 1 bug)

unspecified
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 affected, firefox54 affected, firefox55 affected)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments, 2 obsolete attachments)

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

5 years ago
The audio element's volume is also ignored in this situation.
Tested on the audio element controls with Fx 28 (currently aurora).
Indeed, and for the exact same reasons, thanks. This bug is now about both the volume and the playbackRate.
Summary: Setting playbackRate on a media element does not work when the media element is piped to Web Audio → Setting playbackRate (or volume) on a media element does not work when the media element is piped to Web Audio

Comment 3

5 years ago
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 ?
Assignee: nobody → bugzilla

Comment 4

5 years ago
Posted patch bug966247.patch (volume) (obsolete) — Splinter Review
I've written a small patch to correct the behaviour of the volume for the media.

Would you please review it?
(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 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".
Attachment #8378621 - Flags: review?(paul) → feedback+

Comment 7

5 years ago
Posted patch bug966247.patch (volume) v2 (obsolete) — Splinter Review
Here is the cleaned version of the patch with the test.

Thank you for the review!
Attachment #8378621 - Attachment is obsolete: true
Attachment #8379148 - Flags: review?(paul)
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
Attachment #8379148 - Flags: review?(paul)

Comment 9

5 years ago
Attachment #8379148 - Attachment is obsolete: true
Attachment #8379782 - Flags: review?(paul)
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 :-)
Attachment #8379782 - Flags: review?(paul) → review+
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
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.
Flags: needinfo?(bugzilla)
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.
Flags: needinfo?(bugzilla)

Updated

5 years ago
Assignee: bugzilla → nobody
Mentor: paul
Whiteboard: [mentor=padenot] [lang=c++] → [lang=c++]
Clément is going to take this to the finish line.
Assignee: nobody → clement.geiger
Assignee

Comment 15

5 years ago
Voilà, I finished your patch about the volume; let's get on with the playback rate now!
Attachment #8444068 - Flags: review?(paul)
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 !
Attachment #8444068 - Flags: review?(paul) → review+

Comment 17

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

Thanks!

Comment 18

5 years ago
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).
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?
Flags: needinfo?(clement.geiger)

Comment 20

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

Comment 22

5 years ago
I will take a look on the MediaDecoderStateMachine then.
(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

4 years ago
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
Component: Audio/Video → Audio/Video: Playback
Duplicate of this bug: 1251640
Summary: Setting playbackRate (or volume) on a media element does not work when the media element is piped to Web Audio → Setting playbackRate (or volume) on a media element does not work when the media element is going to the MediaStreamGraph
Duplicate of this bug: 1323012

Comment 29

2 years ago
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

Updated

2 years ago
Assignee: clement.geiger → nobody
Flags: needinfo?(clement.geiger)

Comment 30

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(padenot) → needinfo?(jwwang)
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.
Flags: needinfo?(jwwang)

Comment 33

2 years ago
«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

a year ago
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.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INACTIVE

Updated

a year ago
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

Comment 35

a year ago
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.
PlaybackRate should _not_ work per spec, per [0]. I'm closing this.

[0]: https://w3c.github.io/mediacapture-main/#mediastreams-in-media-elements
Status: REOPENED → RESOLVED
Last Resolved: a year ago11 months ago
Resolution: --- → FIXED

Comment 37

11 months ago
(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 ?
Flags: needinfo?(padenot)
Yes.
Flags: needinfo?(padenot)
Assignee: nobody → bugzilla

Comment 39

11 months ago
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/)
Flags: needinfo?(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
Status: RESOLVED → REOPENED
Flags: needinfo?(padenot)
Resolution: FIXED → ---

Updated

11 months ago
Rank: 15
Priority: -- → P2

Comment 41

5 months ago
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

5 months ago
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.
Assignee: bugzilla → clement.geiger
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago5 months ago
Resolution: --- → FIXED
Summary: Setting playbackRate (or volume) on a media element does not work when the media element is going to the MediaStreamGraph → Setting volume on a media element does not work when the media element is going to the MediaStreamGraph

Updated

5 months ago
Blocks: 1517199
You need to log in before you can comment on or make changes to this bug.