1345049 - Update cubeb from upstream to f07ee6d

Status: VERIFIED FIXED

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

Description

[Alex Chronopoulos [:achronop]]

Commits included:

d5fcfe1 Alex Chronopoulos       audiounit: preserve volume after stream restart (Bug 1343920) (#251)
edac47d Alex Chronopoulos       audiounit: aggregate devices follow up (#247)

Comment 1

[Alex Chronopoulos [:achronop]]

Added one commit, in total:

da03b3c Alex Chronopoulos       audiounit: make static set_volume definition (follow up from last commit)
d5fcfe1 Alex Chronopoulos       audiounit: preserve volume after stream restart (Bug 1343920) (#251)
edac47d Alex Chronopoulos       audiounit: aggregate devices follow up (#247)

Comment 2

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e0fc0cf8456

Comment 3

[Alex Chronopoulos [:achronop]]

There is a question in cubeb repo about the failed build. Copy here for reference:

It does not build for OSX 10.7 (as expected) because AudioHardwareCreateAggregateDevice and AudioHardwareDestroyAggregateDevice do not exist there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cd68a8f11981106a12d0a13659a88694168afe

You mentioned we do not need to support the audiounit backend for versions earlier that 10.9. What do you suggest for those versions?

Comment 4

[Benjamin Smedberg]

[Tracking Requested - why for this release]: nominating because of crasher bug 1343920 which is shipping with 52.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Alex Chronopoulos [:achronop] from comment #3)
> There is a question in cubeb repo about the failed build. Copy here for
> reference:
> 
> It does not build for OSX 10.7 (as expected) because
> AudioHardwareCreateAggregateDevice and AudioHardwareDestroyAggregateDevice
> do not exist there.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=89cd68a8f11981106a12d0a13659a88694168afe
> 
> You mentioned we do not need to support the audiounit backend for versions
> earlier that 10.9. What do you suggest for those versions?

It looks like our builders are still on 10.7 and building against the 10.7 API, despite Firefox supporting 10.9 as the minimum since 48 in August 2016.  I guess we'll have to use the old code paths until either bug 1270217 is fixed  or the builders are migrated to >= 10.9.

Comment 6

[Alex Chronopoulos [:achronop]]

More commits added:

f07ee6d Alex Chronopoulos       audiounit: handle unexpected number of returned frames from user callback (#248)
11dc1e3 Alex Chronopoulos       audiounit: fix typo
427e6d0 Alex Chronopoulos       audiounit: context synchronization in reinit stream
991c98d Alex Chronopoulos       audiounit: revert part of edac47d
40f1cce Alex Chronopoulos       audiounit: add synchronization in cubeb destroy (Bug 1344580) (#249)
f1a6eed Chun-Min Chang  audiounit: Replace auto_channel_layout with a unique_ptr.
988e9e2 Paul Adenot     wasapi: When a device has been invalidated, clear out the device pointer before trying again with the next device. (#252)
c500d18 Paul Adenot     wasapi: Check the emergency bailout boolean before and after the WaitForMultipleObject. (#255)
da03b3c Alex Chronopoulos       audiounit: make static set_volume definition (follow up from last commit)
d5fcfe1 Alex Chronopoulos       audiounit: preserve volume after stream restart (Bug 1343920) (#251)
edac47d Alex Chronopoulos       audiounit: aggregate devices follow up (#247)

Comment 7

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46a4638ca257

Comment 8

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Update cubeb from upstream to f07ee6d.

Review commit: https://reviewboard.mozilla.org/r/118674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118674/

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8845551 [details]
Bug 1345049 - Update cubeb from upstream to f07ee6d.

https://reviewboard.mozilla.org/r/118674/#review120732

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d40fe2f45605
Update cubeb from upstream to f07ee6d. r=kinetik

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/d40fe2f45605

Comment 12

[Gerry Chang [:gchang]]

Track 54+ as the audio issue in bug 1343920 also affected 54.

Hi :achronop,
Since this bug also affects 54, do you think it's worth uplifting to 54 if this patch is not too risky?

Comment 13

[Alex Chronopoulos [:achronop]]

I will create an uplift. It's not risky at all. I will also include fix for bug 1343930 which is an oneliner.

Comment 14

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a8499c2707f

Comment 15

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Uplift to aurora cubeb upstream f07ee6d. r?padenot

Approval Request Comment
[Feature/Bug causing the regression]: 
1342389, 1343920, 1343972, 1344580, 1343930 

[User impact if declined]:
A number of crashes for windows and osx plus volume reset after unglug external usb headset

[Is this code covered by automated tests?]: No

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]:
One of the fixes does:
2.Play any YouTube video, pause after a few seconds
3.Mute the video.
4.Unplug headphones.
5.Plug headphones.
6.Click on the Play button.
7.Observe that the video resumes and the audio is outputted in the headphones even tho the video is muted.

[List of other uplifts needed for the feature/fix]: No

[Is the change risky?]: No

[Why is the change risky/not risky?]:
It's a combination of small fixes already operate in Nightly

[String changes made/needed]:No

Comment 16

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Uplift to aurora cubeb upstream f07ee6d. r?padenot

Approval Request Comment
[Feature/Bug causing the regression]: 
1342389, 1343920, 1343972, 1344580, 1343930 

[User impact if declined]:
A number of crashes for windows and osx plus volume reset after unglug external usb headset

[Is this code covered by automated tests?]: No

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]:
One of the fixes does:
2.Play any YouTube video, pause after a few seconds
3.Mute the video.
4.Unplug headphones.
5.Plug headphones.
6.Click on the Play button.
7.Observe that the video resumes and the audio is outputted in the headphones even tho the video is muted.

[List of other uplifts needed for the feature/fix]: No

[Is the change risky?]: No

[Why is the change risky/not risky?]:
It's a combination of small fixes already operate in Nightly

[String changes made/needed]:No

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8847678 [details] [diff] [review]
Bug 1345049 - Uplift to aurora cubeb upstream f07ee6d. r?padenot

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

r+ but change the strip.

::: media/libcubeb/update.sh
@@ +64,5 @@
>    echo "Remember to update README_MOZILLA with the version details."
>  fi
> +
> +echo "Applying a patch on top of $version"
> +patch -p1 < ./uplift-cubeb-f07ee6d-to-aurora.patch

p3, this patch is from gecko, we need to strip more paths components.

Comment 18

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Uplift to aurora cubeb upstream f07ee6d. r=padenot

Updated with review comments.

Approval Request Comment
[Feature/Bug causing the regression]: 
1342389, 1343920, 1343972, 1344580, 1343930 

[User impact if declined]:
A number of crashes for windows and osx plus volume reset after unglug external usb headset

[Is this code covered by automated tests?]: No

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]:
One of the fixes does:
2.Play any YouTube video, pause after a few seconds
3.Mute the video.
4.Unplug headphones.
5.Plug headphones.
6.Click on the Play button.
7.Observe that the video resumes and the audio is outputted in the headphones even tho the video is muted.

[List of other uplifts needed for the feature/fix]: No

[Is the change risky?]: No

[Why is the change risky/not risky?]:
It's a combination of small fixes already operate in Nightly

[String changes made/needed]:No

Comment 19

[Gerry Chang [:gchang]]

Track 53+ as potential uplift candidate.

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 20

[ovidiu boca[:Ovidiu]]

Tested on Mac OS X 10.12 with  Nightly 55.0a1 (2017-03-16) using the steps from comment 18 and I’m still able to reproduce this. The expected result is that after you resume the video, the audio is outputted in the headphones even tho the video is muted, in my case the audio is not outputted. 
For testing this I used  Microsoft LifeChat LX-3000 headphones.

Alex, do you have any idea why I can still reproduce this?

Comment 21

[Alex Chronopoulos [:achronop]]

It works here. I am in 55.0a1 2017-03-17.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Doesn't apply to aurora:

grafting 385218:d40fe2f45605 "Bug 1345049 - Update cubeb from upstream to f07ee6d. r=kinetik"
merging media/libcubeb/README_MOZILLA
merging media/libcubeb/src/cubeb_wasapi.cpp
warning: conflicts while merging media/libcubeb/README_MOZILLA! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 23

[ovidiu boca[:Ovidiu]]

Attachment: Recording #9.mp4

I still can reproduce this on Mac OS X 10.12 FF Nightly 55.0a1(2017-03-19)and I found a new scenario that doesn't work for me:

STR: 

1. Play any video on youtube
2. Mute the video
3. Press on the next video button (>|).
4. The next video that starts is on mute, unmute the video 

Expected results: The audio is outputted. 

Actual results: The audio is not outputted.  

If I refresh the page the audio is outputted. Please see the attached video, for a better understanding.

Comment 24

[Alex Chronopoulos [:achronop]]

It's not clear what you can still repro. This bug includes a number of fixes. Can you please comment on the corresponding bug. 

Can you please open a new bug with the new scenario.

Comment 25

[ovidiu boca[:Ovidiu]]

I can still reproduce the scenario from comment 18, the one that is posted for manual QA. I tested on Mac OS X 10.12 using FF Nightly 55.0a1(2017-03-20)


For the new scenario from comment 23, I opened the bug 1348831. Thanks for suggesting that Alex.

Comment 26

[Alex Chronopoulos [:achronop]]

Thank you for opening the new bug. About the repro it's strange because I still cannot repro that error here.

Comment 27

[Alex Chronopoulos [:achronop]]

I redirect the discussion with :Ovidiu to bug 1343920.

Comment 28

[Alex Chronopoulos [:achronop]]

We need to uplift this to fix Bug 1349238 in beta. This import contains a number of fixes that we want in beta. If the resolution of Bug 1343920 is not clear yet we can use a different issue. I could also remove the manual test from Approval Request

Comment 29

[Gerry Chang [:gchang]]

Hi :achronop, 
Per comment #25, do we really fix the issue provided in comment #18? It seems to me that QA can still repro the issue.

Comment 30

[Alex Chronopoulos [:achronop]]

The issue is fixed here and my colleague Paul is able to verify the fix. On the other hand QA still repro the error. It's very unclear what is going on. In any case we could exclude Bug 1343920 from the request and uplift the rest of the fixes which are very useful.

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8847684 [details]
Bug 1345049 - Uplift to aurora cubeb upstream f07ee6d. r=padenot

Fix an issue on youtube with volume reset after unplugging external USB headset. Aurora54+.

Comment 32

[ovidiu boca[:Ovidiu]]

I can confirm the fix, I verified on Mac OS X 10.12 with  Nightly 55.0a1 (2017-03-22) using the steps from comment 18 and I can't reproduce this. It was a confusion regarding the expected results from this issue, but now everything is clear. 

Note: The expected result for this issue is: The video continues to play but the audio is muted.

Thanks for help Alex.

Comment 33

[Alex Chronopoulos [:achronop]]

Uplift pushed in Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e41b1af3cd4e88f65cf092891c050a9206c3990

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like the crash mentioned in comment 28 (which is described in bug 1344580) only happens in 54. 
Do you still need to uplift something to beta, here?  Can you describe what you think it will fix in beta?

Comment 35

[Alex Chronopoulos [:achronop]]

Liz, in comment 28 I was meaning Aurora, where the uplift was requested and landed. Sorry for that, I had another beta uplift and got confused.

Comment 36

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Uplift part of cubeb upstream f07ee6d to beta

Approval Request Comment
[Feature/Bug causing the regression]: Bugs 1343920, 1343930
[User impact if declined]: Crash after pause/play when plug/unplug external device.
Volume reset after plug/unplug device
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:
Bug 1343930:
1.Launch Firefox.
2.Access any YouTube video.
3.Press the pause button.
4.Unplug headphones.
5.Plug headphones.
6.Click on the Play button
7.Verify that tab does not crash
Bug 1343920:
1.Launch Firefox.
2.Access any YouTube video.
3.Mute the video.
4.Unplug headphones.
5.Plug headphones.
7.Verify that video remains muted.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Small patch already in Nightly for sometime, verified by QA
[String changes made/needed]: none

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8852443 [details] [diff] [review]
Bug 1345049 - Uplift part of cubeb upstream f07ee6d to beta

Sounds like this may also fix the mac headphone crash in bug 1351728. 
This is late in beta for the uplift but if we can get it into today's build, or Monday's beta 9 build at the latest, we can still have some time to respond if this causes new regressions.

Comment 38

[Alex Chronopoulos [:achronop]]

Beta uplift:
https://hg.mozilla.org/releases/mozilla-beta/rev/ecbac09ad9f8b37691b13f604a554230897886f2

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Are we going to want this last patch on esr52 too? I'm having a hard time keeping track of which cubeb issues are fixed by which uplifts in which bugs at this point...

Comment 40

[Alex Chronopoulos [:achronop]]

I believe it's a safe patch and good to have it in esr52. If you agree I need to create a new patch for that version, this one will not apply cleanly.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Are we going to want this last patch on esr52 too? I'm having a hard time
> keeping track of which cubeb issues are fixed by which uplifts in which bugs
> at this point...

I know it's a mess ... I will create a list with the bugs and the version that they are in.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Yes, we should take this on ESR52 too in that case. Thanks.

Comment 42

[Andrei Vaida [:avaida]]

Let's make sure this works as intended on Beta 53 as well.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

I guess this is sorta not wontfix for esr52, even though it's just the one spot fix we care about and not the entire upstream update.

Comment 44

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1345049 - Uplift part of cubeb upstream f07ee6d to esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fixes one crash and one volume reset issue, both after plug/unplug external headset
User impact if declined: 
One less crash and one volume reaset problem when plug/unplug headset (OSX specific)
Fix Landed on Version:55, 54, 53
Risk to taking this patch (and alternatives if risky): Low risk, small patch already landed in a bunch of branches.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 45

[Julien Cristau [:jcristau]]

Comment on attachment 8853413 [details] [diff] [review]
Bug 1345049 - Uplift part of cubeb upstream f07ee6d to esr52

cubeb fix for 52.1.0esr

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/88466b911357

Comment 47

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce the issue on Firefox 55.0a1 (2017-03-07), under Mac OS X 10.12.1.

I confirm that the issue is no longer reproducible on Firefox 53.0b9, or on Firefox 54.0a2 (2017-04-05).

Tests were performed under Mac OS X 10.12.1.

Comment 48

[Alex Chronopoulos [:achronop]]

Patch landed in m-c on 2017-03-10 so your Nightly build does not include it. Also, it's not clear which issue you managed to reproduce. Please note this patch contains fixes for a number of issues.

Comment 49

[Mihai Boldan, Desktop QA [:mboldan]]

Hi Alex,

I've reproduced and verified the issue described in Comment 15, that was reproducible also on the Nightly build from (2017-03-07).
I noticed that the issue with the crash is covered by Bug 1343930.

Comment 50

[Alex Chronopoulos [:achronop]]

There is a small confusion about the expected result on the Comment 15. Please check comment 32 for the correct one. Just for the history the fix has already been confirmed in comment 32. Nevertheless if you want to retest the fix you should grab a latest Nightly because fix landed on 2017-03-10 and your build is before that date.

Comment 51

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to Alex Chronopoulos [:achronop] from comment #50)
> There is a small confusion about the expected result on the Comment 15.
> Please check comment 32 for the correct one. Just for the history the fix
> has already been confirmed in comment 32. Nevertheless if you want to retest
> the fix you should grab a latest Nightly because fix landed on 2017-03-10
> and your build is before that date.

It's clearly a confusion. I performed a check on an affected build (Firefox 55.0a1 (2017-03-07)) in order to confirm that I'm aware of the issue. 
I performed tests only on a Latest Aurora and on a Latest Beta builds, since Ovidiu confirmed the fix on Latest Nightly build (See Comment 32).

So just to be clear, the Comment 47 stands as a confirmation that the issue with Mute/Un-mute is no longer reproducible on Latest Aurora and Latest Beta builds.

Comment 52

[Andrei Vaida [:avaida]]

Mihai, could you please verify this on 52.1esr as well?

Comment 53

[Mihai Boldan, Desktop QA [:mboldan]]

The issue described in Comment 15 is no longer reproducible on Firefox 52.1.0 ESR build. Tests were performed under Mac OS X 10.11.