1120128 - Black frames on Youtube 1080@60fps with AMD graphics card

Status: VERIFIED FIXED

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

Description

[Tuomas Heittola]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150110004004

Steps to reproduce:

I am running Windows 8.1. I have AMD HD 5850 graphics card and I am using drivers of version 14.12 AMD Catalyst Omega Software (latest stable).

I am watching videos on Youtube using the HTML5 player and video setting 1080@60fps

Example: https://www.youtube.com/watch?v=7SRTEXSpcyI

Using IE to watch the same video works fine.


Actual results:

Video has a lot of black frames and some stuttering.


Expected results:

No black frames or stuttering

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Tuomas, could you attach the graphics section from about:support?

Comment 2

[Tuomas Heittola]

Attachment: aboutsupport_graphics.png

Comment 3

[Tuomas Heittola]

Attachment: aboutsupport_graphics_2.png

I accidentally attached the info from FF34. This one is from FF36

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

David, let's see if this is the same cause as bug 1112331...

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Tuomas Heittola from comment #0)

Tuomas, if you go to about:config and set "layers.d3d11.disable-warp" to true, does the problem persist?

If it does, could you also try "layers.acceleration.disabled" set to true?

Comment 6

[Tuomas Heittola]

(In reply to David Anderson [:dvander] from comment #5)
> "layers.d3d11.disable-warp" to true
Problem still persists with this setting

> "layers.acceleration.disabled" set to true?
With this setting the video plays fine. No black frames or stuttering.

Comment 7

[Ralph Giles (:rillian)]

Is there a driver upgrade available for this card? If that fixed it we could disable acceleration for this particular version.

Comment 8

[Matt Woodrow (:mattwoodrow)]

This user doesn't have blacklisted drivers, so won't be using WARP.

Unfortunately we don't (currently) have a way to blacklist only DXVA, so we might need to blacklist layers for this driver, especially for beta.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Does setting media.windows-media-foundation.use-dxva=false fix the issue?

Comment 10

[Ralph Giles (:rillian)]

Catalyst Omega 14.12, is still the latest driver release for this card. (published 2014 December 8)

Could try downgrading, I guess? Milan, what do you think about Matt's layers disable suggestion?

Comment 11

[Tuomas Heittola]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Does setting media.windows-media-foundation.use-dxva=false fix the issue?

This makes the video play fine.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

We don't really have the data if it's just this driver or a range or all of them on this card, right? It's a pretty big hammer, especially if this is the "only" (not that it's not a big one) problem.  I can see us doing this as a temporary solution, but what are the chances of adding DXVA blacklisting, and in what kind of timeframe would we be able to get that?

Including Bas, just in case this is fixed with some of the more recent work, or he has an idea as to whether there is a fix we can deploy on our side.  Not to mention the opinion on the blacklisting approach.

Comment 13

[Ralph Giles (:rillian)]

Right. This would be a regression if we ship MSE for YouTube, which we're planning to do in 36, so there's not much time. Of course 37 or 38 is still better than nothing.

But as you say, we have no data about how common this issue is.

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Milan Sreckovic [:milan] from comment #12)
> We don't really have the data if it's just this driver or a range or all of
> them on this card, right? It's a pretty big hammer, especially if this is
> the "only" (not that it's not a big one) problem.  I can see us doing this
> as a temporary solution, but what are the chances of adding DXVA
> blacklisting, and in what kind of timeframe would we be able to get that?
> 
> Including Bas, just in case this is fixed with some of the more recent work,
> or he has an idea as to whether there is a fix we can deploy on our side. 
> Not to mention the opinion on the blacklisting approach.

I totally agree, especially since this is the latest stable driver.

Can we get QA to test a bunch of different driver version on this (and similar) hardware, to try narrow down a more precise range?

Adding code to blacklist DXVA in time to uplift to beta sounds scary, though it's probably something we should start working on to ride the trains.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Who knows the blacklist code the best? I really have no idea how the downloadable part works, and I think we'll want that in case new issues appear on release.

I'm happy to do the work here if someone can point me to what needs to be done.

Comment 16

[Bas Schouten (:bas.schouten)]

I feel like this bug is evidence that we'll sadly need to add DXVA blacklisting.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Add blacklist for DXVA

It appears that we can't actually use the downloadable blacklist for new features because of a bug in the original implementation.

We'll have to hardcode the drivers/devices to blacklist instead.

This patch blacklists DXVA for the given device, regardless of driver/OS version. If we can get more precise data on which drivers are affected we might be able to relax this.

Link to build for testing: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6b378741c668/try-win32/

Comment 18

[Tuomas Heittola]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Link to build for testing:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwoodrow@mozilla.
> com-6b378741c668/try-win32/
On this build the video works fine for me.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8554359 [details] [diff] [review]
Add blacklist for DXVA

This is a "large-ish" patch, but it I'd be comfortable recommending for uplift, as I don't see much of a risk in breaking existing functionality.  Once reviewed, let's ask for uplifts...

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 8554359 [details] [diff] [review]
Add blacklist for DXVA

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

I don't know the blacklisted code that well. But this seems like it should be ok.

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 8554359 [details] [diff] [review]
Add blacklist for DXVA

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

I don't know the blacklisted code that well. But this seems like it should be ok.

::: gfx/thebes/gfxPlatform.cpp
@@ +2163,5 @@
>   * not have any effect until we restart.
>   */
>  static bool sLayersSupportsD3D9 = false;
>  static bool sLayersSupportsD3D11 = false;
> +static bool sLayersSupportsDXVA = false;

Can you make these 3 into members of the Singleton for the purposes of consistency?

Comment 22

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #21)

> 
> Can you make these 3 into members of the Singleton for the purposes of
> consistency?

Consistency with what? gfxPlatform doesn't have any other static data members, and we don't have access to the global gfxPlatform object at the time we initialize these values.

Comment 23

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/774ff21aecb3

Comment 24

[Matt Woodrow (:mattwoodrow)]

(In reply to Tuomas Heittola from comment #18)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> > Link to build for testing:
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwoodrow@mozilla.
> > com-6b378741c668/try-win32/
> On this build the video works fine for me.

Thanks for testing that!

Comment 25

[Matt Woodrow (:mattwoodrow)]

[Tracking Requested - why for this release]:

Comment 26

[Ralph Giles (:rillian)]

Looks like this was backed out without updating the bug. Build bustage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef01cebd1e07

Matt, it looks like you accidentally pushed a bunch of ResourceQueue changes when landing this patch.

Comment 27

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e097183cab30

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Ralph Giles (:rillian)]

Comment on attachment 8554359 [details] [diff] [review]
Add blacklist for DXVA

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Users with particular hardware will be unable to play youtube videos.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Risk is lowish. The graphics changes are low risk, just adding another checkbox to the block list. Windows mp4 playback on matching systems will be forced to use software decoding, MSE or not, but that's confirmed to work.
[String/UUID change made/needed]: None.

Comment 30

[avada]

(In reply to Bas Schouten (:bas.schouten) from comment #16)
> I feel like this bug is evidence that we'll sadly need to add DXVA
> blacklisting.

IE uses DXVA too right? Then why doesn't it also have this issue on the same system?

Comment 31

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/02ea301be041

Comment 32

[Ralph Giles (:rillian)]

Attachment: Backport to 36 beta.

Backport patch to beta branch, which doesn't have the WARP checking.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #29)
> [String/UUID change made/needed]: None.

Umm, widget/nsIGfxInfo.idl says otherwise. And we changed an interface without revving the UUID?!

Comment 34

[Ralph Giles (:rillian)]

Attachment: Update nsIGfxInfo uuid

I missed that this patch changed an idl file without changing the uuid, so we need to update that as well. Proposing for all branches, but Ryan tells me we need special ba+ because of the uuid change.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: As above, but abi compatibility issues.
[Describe test coverage new/current, TreeHerder]: Works locally. Fixed by clobber on m-i?
[Risks and why]: Low, I think. Add-ons could need updating?
[String/UUID change made/needed]: UUID changed on nsIGfxInfo.

Comment 35

[Matt Woodrow (:mattwoodrow)]

(In reply to Ralph Giles (:rillian) from comment #34)

> Approval Request Comment
> [Feature/regressing bug #]: MSE
> [User impact if declined]: As above, but abi compatibility issues.
> [Describe test coverage new/current, TreeHerder]: Works locally. Fixed by
> clobber on m-i?
> [Risks and why]: Low, I think. Add-ons could need updating?
> [String/UUID change made/needed]: UUID changed on nsIGfxInfo.

It shouldn't cause any compatibility issues since it's just adding a new constant. It's an entirely backwards compatible change.

Comment 36

[Ralph Giles (:rillian)]

Pushed uuid fix to central and aurora per sheriff request on irc.

https://hg.mozilla.org/mozilla-central/rev/49a3acc91bd3
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ea8231d8308

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8556704 [details] [diff] [review]
Update nsIGfxInfo uuid

You shouldn't bump the IID for constant additions, they have no ABI impact. Certainly not on beta.

If for some reason the build system doesn't like it you should use CLOBBER.

Comment 38

[Ralph Giles (:rillian)]

I think that happened when we landed on m-c, but I can't push to beta, because there's a checkin hook. Should I put ba=gavin on the original patch, without changing the uuid?

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Assuming "original patch" means attachment 8556643 [details] [diff] [review], yes, you can land that on beta with ba=me.

(I can't read bugmail for the next few hours, best to email directly if something comes up.)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/914ded138557

Comment 41

[Ralph Giles (:rillian)]

Thanks Gavin, Ryan.

Comment 42

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using a AMD Radeon HD6450 card on a old Nightly build before the fix but I still see the bad behaviour (black screen frames) on latest Nightly, Aurora, 36 beta 7.

Setting media.windows-media-foundation.use-dxva=false fixes the issue.

Graphics
Adapter Description	AMD Radeon HD 6450
Adapter Drivers	aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM	1024
Device ID	0x6779
Direct2D Enabled	true
DirectWrite Enabled	true (6.3.9600.17111)
Driver Date	11-20-2014
Driver Version	14.501.1003.0
GPU #2 Active	false
GPU Accelerated Windows	1/1 Direct3D 11 (OMTC)
Subsys ID	21251462
Vendor ID	0x1002
WebGL Renderer	Google Inc. -- ANGLE (AMD Radeon HD 6450 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote	true
AzureCanvasBackend	direct2d 1.1
AzureContentBackend	direct2d 1.1
AzureFallbackCanvasBackend	cairo
AzureSkiaAccelerated	0

Catalyst Version: 14.12 AMD Catalyst Omega Software

Does this fix only apply to AMD Radeon HD5800? Should I log a new bug about this?

Comment 43

[Matt Woodrow (:mattwoodrow)]

Yes, please file a new bug for that. Sounds like we'll need to blacklist multiple AMD cards, we need to work out which ones are affected.

Comment 44

[Bogdan Maris, Desktop QA]

Thanks Matt, logged bug 1131638.

Tuomas, can you please download the following builds and confirm that the issue does not reproduce anymore?

Firefox 36 beta 8:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/36.0b8-candidates/build1/win32/en-US/firefox-36.0b8.zip

Firefox Aurora/Developer Edition:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/firefox-37.0a2.en-US.win32.zip

Firefox Nightly:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-38.0a1.en-US.win32.zip

Comment 45

[Tuomas Heittola]

The video plays fine on all of them.

Except on the build 36 beta 8 media.mediasource.enabled was false by default. I had to change it to true before I was able to choose 1080@60.

Comment 46

[Bogdan Maris, Desktop QA]

(In reply to Tuomas Heittola from comment #45)
> The video plays fine on all of them.
> 
> Except on the build 36 beta 8 media.mediasource.enabled was false by
> default. I had to change it to true before I was able to choose 1080@60.

Thanks Tuomas! I will go ahead and close this bug as verified fixed based on the comment from above.

Comment 48

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Do we still need to blacklist WARP with the new failure detection?

Comment 49

[Matt Woodrow (:mattwoodrow)]

Yes. The failure detection catches (at least one type of) invalid textures. The WARP issue is that the compositor fails to draw with an otherwise perfectly valid texture.