Closed
Bug 1426719
Opened 7 years ago
Closed 7 years ago
Latest insider build of Windows 10 (17063) breaks sound playback completely
Categories
(Core :: Audio/Video, defect, P1)
Core
Audio/Video
Tracking
()
People
(Reporter: zbraniecki, Assigned: achronop)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
jesup
:
review+
|
Details |
(deleted),
patch
|
jesup
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jesup
:
review+
jcristau
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jesup
:
review+
jcristau
:
approval-mozilla-esr52-
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•7 years ago
|
||
Microsoft engineer suggest a patch on reddit (sic!): https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drkfxmk/
Comment 2•7 years ago
|
||
[Tracking Requested - why for this release]:
Serious issue that may also affect the january win10 update flights.
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
tracking-firefox58:
--- → ?
Summary: Latest insider build of Windows 10 (17063) It breaks sound playback on Firefox Quantum → Latest insider build of Windows 10 (17063) breaks sound playback completely
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
The uploaded patch is an implementation of the workaround suggested by the person on reddit. I have no access to a Windows machine (I'm at the airport atm) and cannot verify if it fixes the issue.
Also, if this is the patch we may have to try to upstream it up to stable as it affects users in production right now.
Comment 5•7 years ago
|
||
The MS response on their feedback hub is as follows:
Windows has several audio volume APIs - IAudioEndpointVolume, IChannelAudioVolume, ISimpleAudioVolume, and IAudioEndpointVolume. These APIs can be used to change the volume and/or mute state of the stream, app, or audio device.
In build 17063 a change was made to these APIs to have them return S_FALSE (1) if the requested change was a no-op.
This broke apps (like Firefox) which request changes (that may be no-ops) and then explicitly check the return value against S_OK (0).
Comment 6•7 years ago
|
||
Someone should probably audit the other uses of bare S_OK and S_FALSE in mozilla-central and determine whether they should use the SUCCEEDED or FAILED macros instead. And perhaps add a mach lint check.
Blocks: 1197049
status-firefox-esr52:
--- → affected
Reporter | ||
Comment 7•7 years ago
|
||
In that case, there are a couple more places where we do this: https://searchfox.org/mozilla-central/search?q=%3D%3D+S_OK&case=false®exp=false&path=
Updated•7 years ago
|
Attachment #8938450 -
Flags: review?(padenot)
Attachment #8938450 -
Flags: review?(achronop)
Comment 8•7 years ago
|
||
(Asking for review from whoever's still around - don't actually need more than 1 review. I tested this patch on my windows insider box, and it fixes the issue for me.)
Assignee | ||
Comment 9•7 years ago
|
||
Thanks for the report. That's true, I also see it in the documentation here: https://msdn.microsoft.com/en-us/library/windows/desktop/ff485842(v=vs.85).aspx
"All of the constants with the prefix "E_" are error codes. The constants S_OK and S_FALSE are both success codes. Probably 99% of COM methods return S_OK when they succeed; but do not let this fact mislead you. A method might return other success codes, so always test for errors by using the SUCCEEDED or FAILED macro."
I will patch it first on cubeb repo and when is landed I will import it in Gecko.
Comment 10•7 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #6)
> Someone should probably audit the other uses of bare S_OK and S_FALSE in
> mozilla-central and determine whether they should use the SUCCEEDED or
> FAILED macros instead. And perhaps add a mach lint check.
Filed bug 1426722 for this.
Updated•7 years ago
|
Assignee: nobody → achronop
Rank: 10
Priority: -- → P1
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8938450 [details]
Bug 1426719 - Workaround a bug in Windows Insider build 17063 which breaks sound in Firefox.
https://reviewboard.mozilla.org/r/209154/#review214970
r+ though I'm not officially a peer. Clear, obvious fix supplied by MS.
::: media/libcubeb/src/cubeb_wasapi.cpp:1495
(Diff revision 1)
> XASSERT(mix_format->wFormatTag == WAVE_FORMAT_EXTENSIBLE);
> *reinterpret_cast<WAVEFORMATEXTENSIBLE *>(mix_format.get()) = hw_mix_format;
> } else if (hr == S_OK) {
> LOG("Requested format accepted by WASAPI.");
> } else {
This needs to change too, as achronop indicated
Attachment #8938450 -
Flags: review+
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8938468 [details]
Bug 1426719 - Update cubeb from upstream to e1e8337.
https://reviewboard.mozilla.org/r/209176/#review214978
Attachment #8938468 -
Flags: review?(rjesup) → review+
Comment 14•7 years ago
|
||
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6217d397dc08
Update cubeb from upstream to e1e8337. r=jesup
Assignee | ||
Updated•7 years ago
|
Attachment #8938450 -
Flags: review?(padenot)
Attachment #8938450 -
Flags: review?(achronop)
Attachment #8938450 -
Flags: review+
Comment 15•7 years ago
|
||
Nils -- Like we talked about on our call earlier today, can you verify that this patch solves the problem?
Flags: needinfo?(drno)
Comment 16•7 years ago
|
||
If we can verify this patch solves the problem, we should get it into 58 and esr.
tracking-firefox59:
--- → ?
tracking-firefox-esr52:
--- → ?
Assignee | ||
Comment 20•7 years ago
|
||
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1197049
[User impact if declined]: No audio output on Windows
[Is this code covered by automated tests?]: No (we don't have such a sound card in automation)
[Has the fix been verified in Nightly?]: Just landed
[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?]: Not.
[Why is the change risky/not risky?]: It's very simple and it has been verified in Nightly
[String changes made/needed]:
Attachment #8938509 -
Flags: review?(rjesup)
Attachment #8938509 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Attachment #8938509 -
Flags: review?(rjesup) → review+
Comment 21•7 years ago
|
||
I verified that the 17063 build broke audio playback for me and that the patch attached to this bug fixes the playback issue.
Unfortunately it does not fix the mic issue which we have since the earlier insider builds.
Note: there is one more comparison to S_OK in the cubeb wasapi code left. But since it only affect logging it's probably not worth fixing and uplifting.
Flags: needinfo?(drno)
Comment 22•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Reporter | ||
Comment 23•7 years ago
|
||
Thanks all - verified as fixed as per:
- https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drm8287/
- https://www.reddit.com/r/Windows10/comments/7kzt7s/psa_if_you_use_firefox_dont_upgrade_to_17063_it/drm7bg2/
Status: RESOLVED → VERIFIED
Tracking for now since this is a new Windows release potentially coming out in January.
We can look at the uplifts tomorrow.
Comment 25•7 years ago
|
||
Alex, could you prepare a patch for esr and release (just in case)? I guess it will need some rebasing for esr?
Thanks
(this can wait for January 3rd)
Flags: needinfo?(achronop)
Comment 27•7 years ago
|
||
Comment on attachment 8938509 [details] [diff] [review]
Bug 1426719 - Pick cubeb e1e8337 to beta.
cubeb error handling update to fix sound on windows insider build, beta58+
Attachment #8938509 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 28•7 years ago
|
||
uplift |
Flags: needinfo?(jcristau)
Assignee | ||
Comment 30•7 years ago
|
||
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1197049
[User impact if declined]: Playback and webrtc does not work
[Is this code covered by automated tests?]: No (we don't have such a sound card in automation)
[Has the fix been verified in Nightly?]: Just landed
[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?]: Not.
[Why is the change risky/not risky?]: It's very simple and it has been verified in Nightly
[String changes made/needed]:N/A
Attachment #8939517 -
Flags: review?(rjesup)
Attachment #8939517 -
Flags: approval-mozilla-release?
Assignee | ||
Comment 31•7 years ago
|
||
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Basic functionality does not work (all playbacks like youtube etc).
User impact if declined: Playback and webrtc does not work.
Fix Landed on Version: Nightly, beta (requested for release)
Risk to taking this patch (and alternatives if risky): No risk, it's very simple fix and it has been verified in Nightly
String or UUID changes made by this patch: N/A
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(achronop)
Attachment #8939522 -
Flags: review?(rjesup)
Attachment #8939522 -
Flags: approval-mozilla-esr52?
Comment 32•7 years ago
|
||
Do we know when this new version of Windows is going to be released?
Updated•7 years ago
|
Comment 33•7 years ago
|
||
Microsoft is targeting March 2018 for the new version of Windows 10.
Windows 10 current release strategy is:
"Twice-per-year feature update release targeting around March and September"
https://technet.microsoft.com/en-us/windows/release-info.aspx?f=255&MSPPError=-2147217396
Insiders have access to the builds now, in the public realm, to test with. Fast and Slow rings.
Fast Ring includes many changes, including this API change, in Build 17063. It's unclear whether the change was intentional or a mistake. I tried asking in the Feedback Hub item (as Jacob K. in a comment), but have not received a response. And I doubt I'll get one.
https://aka.ms/G2cgew
Slow Ring is more stable, and if the API change landed there, I'd assume it was intentional at that point.
I'm happy that Mozilla is responsive in getting this change implemented, and if it doesn't break any existing audio, I'd personally like it to land in release Firefox 57.
Thanks,
Jacob
Comment 34•7 years ago
|
||
Thanks Jacob for the information.
As we are going to ship Firefox 58 in a bit more than 2 weeks (2018-01-23), maybe we could just wait...
Comment 35•7 years ago
|
||
(In reply to Jacob W. Klein from comment #33)
> Fast Ring includes many changes, including this API change, in Build 17063.
> It's unclear whether the change was intentional or a mistake. I tried asking
> in the Feedback Hub item (as Jacob K. in a comment), but have not received a
> response. And I doubt I'll get one.
> https://aka.ms/G2cgew
>
> Slow Ring is more stable, and if the API change landed there, I'd assume it
> was intentional at that point.
The engineer at Microsoft who proposed this fix also said on Reddit he had fixed it internally and added tests to prevent the bug from happening again, so even if nothing changed in Firefox, final users shouldn't experience this bug when the next version of Windows is released.
Comment 36•7 years ago
|
||
ok, so, we would not even need to take the patch in esr52 then...
Updated•7 years ago
|
Attachment #8939517 -
Flags: review?(rjesup) → review+
Updated•7 years ago
|
Attachment #8939522 -
Flags: review?(rjesup) → review+
Comment 37•7 years ago
|
||
Heads-up, Windows fix has reached mainline as of build 17073
Comment 38•7 years ago
|
||
OK, so, let's wontfix that for previous versions.
Comment 39•7 years ago
|
||
Comment on attachment 8939517 [details] [diff] [review]
update-cubeb-to-e1e8337-release.patch
per comment 38
Flags: needinfo?(lhenry)
Attachment #8939517 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•7 years ago
|
Attachment #8939522 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Comment 40•7 years ago
|
||
I believe Windows 10 Insider Build 17074, fixes the issue, by possibly reverting the Sound API change that broke 17063.
See here:
https://blogs.windows.com/windowsexperience/2018/01/11/announcing-windows-10-insider-preview-build-17074-pc
- We fixed an issue resulting in certain apps, like Firefox, might not have audio after upgrading to the previous flight. This issue also impacted the ability to record audio in Microsoft Edge.
You need to log in
before you can comment on or make changes to this bug.
Description
•