962288 - Major performance regression playing <video> on Windows with DXVA

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

There's been a major perf regression in Windows <video> playback.

The first bad Nightly build was 2014-01-19.
This was introduced by this changeset:

8c9e9d8c4c75	Ali Akhtarzada — Bug 959123 - Implement CairoImage::GetAsSourceSurface. r=nica

Steps To Reproduce (Windows only):
1. Update Nightly to at least 2014-01-19.
2. Open Nightly, load http://people.mozilla.org/~cpearce/avatar.mp4 in URL bar.
3. Video will play automatically.
4. Observe that the video is dropping frames, it doesn't look smooth.

You can also pause the video, right click to open the context menu and select "Show Statistics". Note that the "frames painted" field is a lot smaller than the "frames parsed". This means lots of frames are being dropped in rendering. I'm getting about 30% frames dropped, normally it should be <2%.

You can also open Task Manager, observe Firefox with high CPU usage. For me playing that avatar video was around 4%-5% in Release Firefox, but with the regressions it's around 17% CPU usage.

We cannot ship a regression in video playback like this. It needs to be fixed, or bug 959123 must be backed out. If we are to back it out we must do so soon, before more too much stuff lands on top of 8c9e9d8c4c75 that will need to be backed out with it.

It's not obvious to me how 8c9e9d8c4c75	could cause this regression. The DXVA video decoder should produce surfaces in format ImageFormat::D3D9_RGB32_TEXTURE. A new branch causing us to do read back or somesuch must have been added.

Comment 1

[Ali Ak]

(In reply to Chris Pearce (:cpearce) from comment #0)
> It's not obvious to me how 8c9e9d8c4c75	could cause this regression. The
> DXVA video decoder should produce surfaces in format
> ImageFormat::D3D9_RGB32_TEXTURE. A new branch causing us to do read back or
> somesuch must have been added.

Not obvious to me either. 8c9e9d8c4c75 doesn't change anything functionally. It does have an extra call to gfxPlatform::GetSourceSurfaceForSurface (but that should just wrap the existing gfxSurface in a SourceSurface (unless I'm mistaken on how it works?)

Matt has a theory though - https://bugzilla.mozilla.org/show_bug.cgi?id=959123#c8

Comment 2

[Ali Ak]

Ok, so 8c9e9d8c4c75 also made some memebers private which were previously public (so access went through the Get* member func of Image instead of accessing the member variable directly (which causes the readback). I'll push a patch in a few hours when I get to work!

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks!

Comment 4

[Ali Ak]

Attachment: Don't call DeprecatedGetAsSurface unless it's Cairo

Changeset from Bug 959123 caused a performance decrease because
DeprecatedGetAsSurface was being called on an image that may not
be a CAIRO_SURFACE.

Comment 5

[Ali Ak]

Attachment: Don't call DeprecatedGetAsSurface unless it's Cairo. r=mattwoodrow.

Changeset from Bug 959123 caused a performance decrease because
DeprecatedGetAsSurface was being called on an image that may not
be a CAIRO_SURFACE.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/0b3dd8117325 under suspicion of introducing leaks on Windows debug builds like these:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33416718&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33417451&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33416611&tree=Mozilla-Inbound

Comment 8

[Ali Ak]

Ugh, ok so about these smart pointers...

When a function returns the result of nsRefPtr::forget (which is an already_AddRefed object), does that have to be put inside another nsRefPtr object so that it's properly unrefed?

Comment 9

[Ali Ak]

Attachment: Don't call DeprecatedGetAsSurface unless it's Cairo. r=mattwoodrow.

Changeset from Bug 959123 caused a performance decrease because
DeprecatedGetAsSurface was being called on an image that may not
be a CAIRO_SURFACE.

Reverted CairoImage surface members to public access as they were
before the patch from bug 959123.

Comment 10

[Ali Ak]

(In reply to Ali Ak from comment #8)
> Ugh, ok so about these smart pointers...
> 
> When a function returns the result of nsRefPtr::forget (which is an
> already_AddRefed object), does that have to be put inside another nsRefPtr
> object so that it's properly unrefed?

Seems so.
Changed it back to public member access :(

No BloatView leaks detected on my run.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0c73990f05

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a0c73990f05

Comment 13

[Bogdan Maris, Desktop QA]

Verified as fixed on Firefox 29 beta 1 using Windows 7 64bit/32bit, Windows 8.1 64bit (Surface PRO 2), Windows 8 32bit, Windows Vista 64bit.