Closed Bug 966247 Opened 11 years ago Closed 6 years ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- wontfix
firefox53 --- affected
firefox54 --- affected
firefox55 --- affected

People

(Reporter: padenot, Assigned: cgg, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 2 obsolete files)

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.
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
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
Attached patch bug966247.patch (volume) (obsolete) (deleted) — 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.
Attachment #8378621 - Flags: review?(paul)
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+
Attached patch bug966247.patch (volume) v2 (obsolete) (deleted) — 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)
Attached patch bug966247.patch (volume) v3 (deleted) — Splinter Review
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)
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
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+
Hi! I see version 35 didn't get this fix, any idea when the fix should be released? Thanks!
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)
Paul, if you can guide me I will be happy to fix it.
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.
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
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
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
Assignee: clement.geiger → nobody
Flags: needinfo?(clement.geiger)
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)
«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.
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
Closed: 7 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
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
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
(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
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 → ---
Rank: 15
Priority: -- → P2
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
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
Closed: 6 years ago6 years 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
Blocks: 1517199

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.

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: