Major performance regression playing <video> on Windows with DXVA

VERIFIED FIXED in mozilla29

Status

()

Core
Graphics: Layers
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: cpearce, Assigned: Ali Ak)

Tracking

({perf, regression})

29 Branch
mozilla29
All
Windows 7
perf, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
(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
(Assignee)

Comment 2

5 years ago
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!
(Reporter)

Comment 3

5 years ago
Thanks!
(Assignee)

Comment 4

5 years ago
Created attachment 8363676 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #8363676 - Flags: review?(matt.woodrow)
Attachment #8363676 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 8363835 [details] [diff] [review]
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.
Attachment #8363676 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Keywords: perf, regression
(Assignee)

Comment 8

5 years ago
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?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 9

5 years ago
Created attachment 8364321 [details] [diff] [review]
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.
Attachment #8363835 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #8364321 - Flags: review?(matt.woodrow)
Attachment #8364321 - Flags: review?(matt.woodrow) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0c73990f05
Flags: needinfo?(matt.woodrow)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2a0c73990f05
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.