1331869 - Update cubeb from upstream to d96e35f02d

Status: RESOLVED FIXED

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

Description

[Alex Chronopoulos [:achronop]]

Update cubeb from upstream to 00ee120370

Comment 1

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1331869 - Update cubeb from upstream to d96e35f02d.

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

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1331869 - Update moz.build to build cubeb_mixer.cpp.

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

Comment 3

[Ryan VanderMeulen [:RyanVM]]

We want this on 52 too.

Comment 4

[Paul Adenot (:padenot)]

Yeah we'll cherry pick the relevant stuff.

Comment 5

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a374e9b6b47

Comment 6

[Alex Chronopoulos [:achronop]]

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

Comment 7

[Alex Chronopoulos [:achronop]]

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

Comment 8

[Alex Chronopoulos [:achronop]]

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

Comment 9

[Alex Chronopoulos [:achronop]]

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

Comment 10

[Alex Chronopoulos [:achronop]]

Comment on attachment 8827895 [details]
Bug 1331869 - Update moz.build to build cubeb_mixer.cpp.

https://reviewboard.mozilla.org/r/105502/#review106750

I cancel this one to create a new.

Comment 11

[Alex Chronopoulos [:achronop]]

Comment on attachment 8827855 [details]
Bug 1331869 - Update cubeb from upstream to d96e35f02d.

https://reviewboard.mozilla.org/r/105476/#review106752

I cancel this one also.

Comment 12

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1331869 - Initialize new field of cubeb_stream_params.

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

Comment 13

[Alex Chronopoulos [:achronop]]

Comment on attachment 8827855 [details]
Bug 1331869 - Update cubeb from upstream to d96e35f02d.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105476/diff/1-2/

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8827855 [details]
Bug 1331869 - Update cubeb from upstream to d96e35f02d.

https://reviewboard.mozilla.org/r/105476/#review106884

Comment 15

[Matthew Gregan [:kinetik]]

Comment on attachment 8828402 [details]
Bug 1331869 - Initialize new field of cubeb_stream_params.

https://reviewboard.mozilla.org/r/105832/#review106886

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8828402 [details]
Bug 1331869 - Initialize new field of cubeb_stream_params.

https://reviewboard.mozilla.org/r/105832/#review107044

::: dom/media/GraphDriver.cpp:666
(Diff revision 1)
>    }
>  
>  
>    input = output;
>    input.channels = mInputChannels; // change to support optional stereo capture
> +  input.layout = (mInputChannels == 1) ? CUBEB_LAYOUT_MONO : CUBEB_LAYOUT_STEREO;

Just hardcode this to mono, some other code is not ready for stereo input, iirc.

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8827855 [details]
Bug 1331869 - Update cubeb from upstream to d96e35f02d.

https://reviewboard.mozilla.org/r/105476/#review107048

Comment 18

[Alex Chronopoulos [:achronop]]

Comment on attachment 8828402 [details]
Bug 1331869 - Initialize new field of cubeb_stream_params.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105832/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/056197b55d49
Update cubeb from upstream to d96e35f02d. r=kinetik,padenot
https://hg.mozilla.org/integration/autoland/rev/2d93fb71eb88
Initialize new field of cubeb_stream_params. r=kinetik,padenot

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/056197b55d49
https://hg.mozilla.org/mozilla-central/rev/2d93fb71eb88

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Alex, according to comment #3, we want this in 52. could you fill the uplift request? Thanks

Comment 22

[Paul Adenot (:padenot)]

I've rebased the mac-only part of this, and I've pushed to try, it's currently retriggering mda a bunch to make sure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eec8fd545d3fc8429812b9cf7910aa50a591908

Comment 23

[Alex Chronopoulos [:achronop]]

Clearing NI based on previous comment

Comment 24

[Paul Adenot (:padenot)]

Attachment: Uplift the OSX portion of bug 1331869 to beta

Alex, can you double check my rebase ?

This is very green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eec8fd545d3fc8429812b9cf7910aa50a591908&selectedJob=71880326

Comment 25

[Paul Adenot (:padenot)]

This patch is quite big because:
- It's moving two very big function around ;
- It's renaming some stuff ;
- It includes a copy of the patch in the patch (see update.sh changes, when uplifting cubeb revision, we prefer to do it this way so that we know where all the modifications come from).

The actual changes are not too big. I feel confident about uplifting.

Comment 26

[Alex Chronopoulos [:achronop]]

Comment on attachment 8830393 [details] [diff] [review]
Uplift the OSX portion of bug 1331869 to beta

Looks good, thanks

Comment 27

[Paul Adenot (:padenot)]

Comment on attachment 8830393 [details] [diff] [review]
Uplift the OSX portion of bug 1331869 to beta

Approval Request Comment
[Feature/Bug causing the regression]: Rather old deadlock on OSX (multiple years old at least)
[User impact if declined]: Frequent oranges on automation, deadlocks in the wild, when playing back audio on OSX
[Is this code covered by automated tests?]: No, but sherrifs observed that the oranges are gone. We haven't been able to repro the deadlock reliably to write a unit test.
[Has the fix been verified in Nightly?]: Yes, it has proven to be effective
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None, this is a rollup patch that includes everything
[Is the change risky?]: I don't think it is. Also this can be backed out cleanly.
[Why is the change risky/not risky?]: No other problems have been reported in aurora or nightly with this patch, and it has fixed a number of oranges. 
[String changes made/needed]: None

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 8830393 [details] [diff] [review]
Uplift the OSX portion of bug 1331869 to beta

cubeb update to fix osx deadlock, beta52+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc169ee114d4

Comment 30

[Paul Adenot (:padenot)]

Attachment: Uplift a double-free fix (cubeb revision f82f15)

Comment 31

[Paul Adenot (:padenot)]

Comment on attachment 8831621 [details] [diff] [review]
Uplift a double-free fix (cubeb revision f82f15)

This is fixed on central and aurora and fixes a low volume crash.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1302231
[User impact if declined]: low volume crash
[Is this code covered by automated tests?]: no, we're not even able to reproduce
[Has the fix been verified in Nightly?]: Yes, and also on aurora (approximately a week of baking each)
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Can be backed out cleanly. Crash stats clearly shows this is effective.
[Why is the change risky/not risky?]: Not risky, this simply adds a check fore deleting some data
[String changes made/needed]: None

Comment 32

[Julien Cristau [:jcristau]]

Comment on attachment 8831621 [details] [diff] [review]
Uplift a double-free fix (cubeb revision f82f15)

one more cubeb fix for beta52

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a5045a32c413