Closed Bug 1571513 Opened 5 years ago Closed 4 years ago

Audiosinks are not cleaned up on navigation

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
68 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox75 --- verified

People

(Reporter: u644666, Assigned: achronop)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1571513 - Setup PrincipalHandle and Playing listener of a DecodedStream in the start. r?alwu
47 bytes, text/x-phabricator-request
Details | Review
Bug 1571513 - Implement suspend/resume of MediaDecoder to clean up low level resources. r?alwu
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/75.0.3770.142 Chrome/75.0.3770.142 Safari/537.36

Steps to reproduce:

  1. Opened youtube, watched a video
  2. Went back to youtube homepage using a bookmark

Actual results:

  1. Opened youtube, watched a video
  2. Went back to youtube homepage using a bookmark
  3. i noticed that even with 1 tab, there are 2 volume sliders in the system audio section, i repeated this step and each time a new slider was created. None of them disappeared until i closed Firefox

Expected results:

Firefox should not keep creating new audio channels without deleting the previous ones, and even kill thoose that are not playing anything to keep things clean. Or have an option for the entirety of firefox to be controlled with 1 slider.

This happens on Linux Ubuntu 18.04.2 LTS

Component: Untriaged → Audio/Video: Playback
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64

I was unable to confirm this behavior on my Ubuntu 18.04 machine, but someone else may be able to repro.

I can record it for you if it will be needed

Qik, thanks, that would be helpful.

Flags: needinfo?(janchmiel00)

https://www.youtube.com/watch?v=YD2Mtnsxp-U&feature=youtu.be
sorry for bad quality and recording artifacts. They are not related, i had to upload with this quality because of my weak internet connection and the artifacts are caused by badly made as always nvidia properitary drivers

Flags: needinfo?(janchmiel00)

I can reproduce.

I don't see the output streams in Ubuntu chrome like you, but I use pavuctl to monitor them, and they build up until I close the tab.

For me it was enough to open a youtube video with audio, play it so the output stream gets opened, then reload and do it again. Seems silly to have the old output streams stay alive like that.

I'll try to capture it in a pernosco recording so we can debug efficiently.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Firefox endlessly creating audio channels → Audiosinks are not cleaned up on navigation

I got an rr recording with the STR:

  • load a youtube video
  • play (plays an ad)
  • wait until actual video plays
  • reload
  • play
  • reload
  • play (plays another ad)
  • wait until actual video plays
  • there are now 2 audio streams (expected 3, one went away)
  • open new, empty tab
  • close youtube tab
  • wait until audio streams are gone
  • close firefox

Uploading to pernosco now.

The pernosco recording is at https://pernos.co/debug/sj056teOMSKZHm7eBBxb0A/index.html

So normally we'd stop the audio stream when playback ends, but these youtube videos don't end for some reason.

One can see this by filtering in pernosco on the executions of HTMLMediaElement::PlaybackEnded.

See also the notebook for the relevant moments in time creating and tearing down decoders; and when the AudioStreams were paused instead of stopped (the bug, if you will).

These three videos are played using MediaSource Extensions (MSE), where it's up to the application (youtube) to handle buffering of data. If I filter pernosco on executions of MediaSource::EndOfStream, I can see that it's called for the two ads, and for the second video. Once in a ProgressEvent, and once in an "updateend" event.

The first and third videos' MediaSources do not get any calls to endOfStream() at all. For the second one we do so I'd kind of expect us to play it until the end, and then shut down. But that doesn't happen either, because HTMLMediaElement::NotifyOwnerDocumentActivityChanged() pauses and suspends it.

I wonder how hard it would be to kill the MediaSink when we get suspended.

Adding some people to CC in case anyone is interested in picking this up.

(In reply to Andreas Pehrson [:pehrsons] from comment #9)

Adding some people to CC in case anyone is interested in picking this up.

Will it be fixed in the next update ? I really like Firefox but cant use it because of this

(In reply to Qik from comment #10)

Will it be fixed in the next update ? I really like Firefox but cant use it because of this

That depends. If we get a patch soon enough it could get into Firefox 70. If anyone wants to contribute to this, that's also welcome.

Blocks: 1584177
See Also: → 1588395

Just tested this on my brothers computer running Mint 19 XFCE
Happens EXACTLY line on my computer.

Tested on firefox 70.0.1

See Also: → 1602345

I gave a try to this one. My finding is that after a refresh the old dom tree, which includes the media element, is not deleted. I guess this is due to some kind of caching (bfcache maybe?). So after the refresh the old stream remains and, in parallel, a new media element is created with a new stream attached. When the tab is closing, all dom trees are being cleaned up so the media elements and the streams are cleaned up, eventually.

I also noticed that on refresh, the old media element is paused and suspended. What suggested in comment 8 can be the solution here. We could implement the MediaDecoder::Suspend() method, which is currently blank, to shutdown the MediaSink. That looks relatively easy and will clear the streams on refresh. We must be careful though, to restore everything if the medial element becomes active again.

Assignee: nobody → achronop

Implement the MediaDecoder::Suspend/Resume to shutdown and recreate the MediaSink. This is important to refresh because the existing tree does not shutdown. It stays alive but remains suspended. Thus it is necessary to clean up the low-level media resource on suspend. Otherwise, for example in audio case, the system audio stream does not clean up and it is listed in system settings.

(In reply to Alex Chronopoulos [:achronop] from comment #17)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=169a98e2955e211ab1363cb8233e960fa260767d

Is this being worked on ? If so, THANK YOU !

Priority: P2 → P1
See Also: → 1610193

The situation has been improved after Bug 1610193 has landed. The old tree including the media element and the audio resources are collected eventually at some point bedore tab close. The system audio streams remain open till then.

(In reply to Alex Chronopoulos [:achronop] from comment #19)

The situation has been improved after Bug 1610193 has landed. The old tree including the media element and the audio resources are collected eventually at some point bedore tab close. The system audio streams remain open till then.

For me the audio channels remain until i close the tab. I left my laptop running for 2 houts to test that

Hmm that's strange, I tested and I see the audio streams (or channels) collected before closing the tab. Are you sure you are using the latest Nightly? No need to wait for 2 hours, 30 secs is enough. In any case, this will not change anything about this bug. I'll get it fixed.

(In reply to Alex Chronopoulos [:achronop] from comment #21)

Hmm that's strange, I tested and I see the audio streams (or channels) collected before closing the tab. Are you sure you are using the latest Nightly? No need to wait for 2 hours, 30 secs is enough. In any case, this will not change anything about this bug. I'll get it fixed.

Daily i use the stable version or chromium if audio channels go wild, but i conducted the test on the latest nightly

I am dealing with an intermittent that I have not been able to clean up. The run with MediaDecoder logs:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=288906079&revision=4a8bbccfa1d8983b499e24e0320d63b18ae049cf

Track if MediaSink is suspended and disallow to start it or change the sink id. Also, take care of how the suspended state interacts with a DecodedStream. When media capture is requested allow the MediaSink to change to a DecodedStream but starting the sink is restricted until the resume of the sink.

Attachment #9125096 - Attachment is obsolete: true
Attachment #9128113 - Attachment is obsolete: true

mPrincipalHandle and mPlaying` listeners were being connected in the ctor of a DecodedStream. However, this is not necessary because their attributes will be modified after the sink start. In addition to that, it was causing problems if a sink was replaced before being started or stopped (and shutdown). This is a valid scenario, though, that we need to support.

Implement the MediaDecoder::Suspend/Resume to shutdown and recreate the MediaSink. This is important the page is refreshed because the existing tree does not shutdown immediatelly. It stays alive but remains suspended thus it is necessary to clean up the low-level system resources. In order to support the suspended state with the existing work flow, if MediaSink is suspended it is disallowed to start it or change the sink id. Recreating the MediaSink is possible but starting it is restricted until resume.

Pushed by achronopoulos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aac6ada5bac3
Setup PrincipalHandle and Playing listener of a DecodedStream in the start. r=alwu
https://hg.mozilla.org/integration/autoland/rev/cc1790a6b7e4
Implement suspend/resume of MediaDecoder to clean up low level resources. r=alwu

https://hg.mozilla.org/mozilla-central/rev/aac6ada5bac3
https://hg.mozilla.org/mozilla-central/rev/cc1790a6b7e4

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox75: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Regressions: 1618908
Flags: qe-verify+

Hello,

I can confirm that this issue is verified in Fx 75.0b5 (Build ID: 20200317211402).

Status: RESOLVED → VERIFIED
status-firefox75: fixed → verified
Flags: qe-verify+

(In reply to Daniel Cicas [:dcicas], Release QA from comment #33)

Hello,

I can confirm that this issue is verified in Fx 75.0b5 (Build ID: 20200317211402).

So its not fixed ?

(In reply to Qik from comment #34)

So its not fixed ?

It's fixed, the comment 33 is about verifying the fix.

Blocks: 1628521
Regressions: 1823586
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: