Closed Bug 1403063 Opened 7 years ago Closed 7 years ago

Disable VP8 HW decoder on windows 10

Categories

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

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox55 --- wontfix
firefox56 blocking fixed
firefox57 + fixed
firefox58 + fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files)

Microsoft has confirmed a bug there, which is only fixed in RS3 Windows.

So disable VP8 for now, there's not much gain to have by using VP8 hardware decoder, most of those videos are low res anyway
Tracking release, this is a top crasher.
(In reply to Jean-Yves Avenard [:jya] from comment #0)
> Microsoft has confirmed a bug there, which is only fixed in RS3 Windows.
Do you know is there any bugs reported in Bugzilla may be related to it? 
Is Bug 1374231 related to this?
(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> (In reply to Jean-Yves Avenard [:jya] from comment #0)
> > Microsoft has confirmed a bug there, which is only fixed in RS3 Windows.
> Do you know is there any bugs reported in Bugzilla may be related to it? 
> Is Bug 1374231 related to this?
Ah. Just found this bug has been set to depend on bug 1374231. I should refresh my page often.
MozReview-Commit-ID: KJLfSFYyTWu
Assignee: nobody → jyavenard
Comment on attachment 8912120 [details] [diff] [review]
Disable VP8 HW decoder on Windows. r?gerald

Approval Request Comment
[Feature/Bug causing the regression]: 1376838, which enabled the hw decoder, however this is bypassing a Windows 10 bug 
[User impact if declined]: Crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[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?]: no
[Why is the change risky/not risky?]: This will default to using the vp8 software decoder which is already happening on most of our platforms. That code path has been far more covered than the HW accelerator
[String changes made/needed]: none
Attachment #8912120 - Attachment filename: Bug-1403063---Disable-VP8-HW-decoder-on-Windows-rg.patch → Disable-VP8-HW-decoder-on-Windows-rg-beta.patch
Attachment #8912120 - Flags: review?(gsquelart)
Attachment #8912120 - Flags: approval-mozilla-release?
Comment on attachment 8912107 [details]
Bug 1403063 - Disable VP8 HW decoder on Windows.

Approval Request Comment
[Feature/Bug causing the regression]: 1376838, which enabled the hw decoder, however this is bypassing a Windows 10 bug 
[User impact if declined]: Crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[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?]: no
[Why is the change risky/not risky?]: This will default to using the vp8 software decoder which is already happening on most of our platforms. That code path has been far more covered than the HW accelerator
[String changes made/needed]: none
Attachment #8912107 - Flags: approval-mozilla-beta?
:lizzard, this is likely the real fix for bug 1374231...

The crash wasn't due to any changes we made in 56, but a recent bug in Windows 10
Flags: needinfo?(lhenry)
Comment on attachment 8912107 [details]
Bug 1403063 - Disable VP8 HW decoder on Windows.

https://reviewboard.mozilla.org/r/183482/#review188650
Attachment #8912107 - Flags: review?(gsquelart) → review+
Attachment #8912120 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8ac3ab29d3c
Disable VP8 HW decoder on Windows. r=gerald
Priority: -- → P1
Comment on attachment 8912107 [details]
Bug 1403063 - Disable VP8 HW decoder on Windows.

This is really super late for 56 :/

Taking it on 57. Should be in 57b4
Attachment #8912107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8912120 [details] [diff] [review]
Disable VP8 HW decoder on Windows. r?gerald

I guess we want that in release too. Taking it in case Liz wants that too.
Attachment #8912120 - Flags: approval-mozilla-release? → approval-mozilla-release+
the signature from bug 1374231 is already gone in 56rc2 (so the other workaround taken there is at least working for the moment).
i can't judge if it's worth spinning up another release build just for that now...
I still see crashes in 57 and 58 which has the same code behaviour as 56.
Theres also plenty of crashes showing as the last 56.


Are you sure rc2 made it public?
As if it's 57 it will be in 56 for sure.
according to the status flags in bug 1374231 the workaround there only made it into 56 and not 57/58.

i'm positive that 56.0rc2 was pushed to the beta population yesterday - it shows up as version 56.0b99 with buildid 20170922200134 in crash stats search: https://crash-stats.mozilla.com/search/?build_id=20170922200134&product=Firefox&version=56.0b99#facet-signature
and this is how crash signatures have shifted between rc1 & rc2: https://mozilla.github.io/stab-crashes/compare-betas.html?product=Firefox&beta1=56.0b99%20-%2020170918210324&beta2=56.0b99%20-%2020170922200134
Your only looking at the status that introduced a cut down version of the fix that made 57.
The fact remain that the code behaviour in 56 and 57 is identical 
And the crash is happening in 57 and 58.

Thst you don't see the crash in 56 is irrelevant at this stage, just a matter of luck and time.

https://bugzilla.mozilla.org/show_bug.cgi?id=1352016
OK, sounds like a plan. Ryan, can you land this on m-r for Yet Another Build? Thanks!
Flags: needinfo?(lhenry) → needinfo?(ryanvm)
To be more clear, I will plan for 56 build 6 later today.  (Maybe with one other issue still yet to land).  Then we can test and then proceed with the more-cautious-than-usual rollout to release.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/d8ac3ab29d3c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1403622
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Jean-Yves's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.