958008 - Avoid copying images on Android/Gonk when there's nothing to optimize

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Michael Wu [:mwu]]

Attachment: Avoid unnecessary image copy on Android/Gonk

On Android/Gonk, offscreen surfaces are just gfxImageSurfaces, so there's no point in copying an image to a new gfxImageSurface if there's no format conversion.

Comment 1

[Michael Wu [:mwu]]

Attachment: Avoid unnecessary image copy on Android/Gonk, v2

A cleaner version without ifdefs. I can do the same cleanup for windows.

Comment 2

[Andreas Gal :gal]

Comment on attachment 8359044 [details] [diff] [review]
Avoid unnecessary image copy on Android/Gonk, v2

Why not put that straight into gfxPlatform::OptimizeImage?

Comment 3

[Michael Wu [:mwu]]

That's what the first version of my patch did, but it only did it for Android/Gonk. Seems better to put the platform specific fastpaths into the platform specific implementations.

Other platforms generally seem to have platform specific offscreen surfaces so they probably don't want this.

Comment 4

[Andreas Gal :gal]

Your patch seems to be a general if-already-optimal-don't-optimize, so why not allow that code everywhere?

Comment 5

[Michael Wu [:mwu]]

Even when the format of the image is optimal, how that image is stored might not be optimal. For example, on OSX, we always convert our gfxImageSurface to a gfxQuartzImageSurface. Platforms other than gonk/android seem to always have a more optimal native surface to use rather than gfxImageSurface. I have no idea how much those surfaces improve things.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8359044 [details] [diff] [review]
Avoid unnecessary image copy on Android/Gonk, v2

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

This looks good to me.

Comment 7

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9394e6901453

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9394e6901453

Comment 9

[ying.xu]

Would you merge this into v1.3 branch?

Comment 10

[Andreas Gal :gal]

The request was made to add this to 1.3. We can also consider the specific branches for the uplift.

Comment 11

[Andrew Overholt [:overholt]]

How do people feel about only taking this for tarako vs taking it on all devices for 1.3?  Other than being concerned with risk (but that's not up to us if mwu and jrmuziel are cool with it), people in triage didn't have much of an opinion either way.

Comment 12

[Michael Wu [:mwu]]

AFAIK, the risk is pretty low, and it's in a code path shared with Fennec so we get extra testing here for free.

Comment 13

[Andrew Overholt [:overholt]]

What could possibly go wrong?  :)

Comment 14

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

We've been keen on any kind of memory usage reduction, so this should play nicely.  I imagine this only applies to the peak memory usage, rather than overall, but either way, it should help.

Comment 15

[Andrew Overholt [:overholt]]

(In reply to Andrew Overholt [:overholt] from comment #13)
> What could possibly go wrong?  :)

In case it's not clear, this was meant as a joke and came out of a semi-hilarious-at-the-time discussion we had during triage :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d53dc428fc55

Comment 17

[Jason Smith [:jsmith]]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=960224#c24 - this needs to be backed out. This caused a 50ms startup performance regression in the Music app.

Flagging checkin-needed for a backout on all branches.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/659f28f869bc
https://hg.mozilla.org/releases/mozilla-aurora/rev/62b1dd99bf38
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3cedba4f54d2

Comment 19

[Jason Smith [:jsmith]]

Unblocking this due the performance regression risk.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Smith [:jsmith] from comment #19)
> Unblocking this due the performance regression risk.

This wasn't backed out from mozilla-release or mozilla-beta, so those status flags were correct (albeit pointless).

Comment 21

[Michael Wu [:mwu]]

In theory, we should check this back in because this did not actually cause a 50ms regression in the music app. In practice, bug 980035 makes us to not go through this path anymore, so there's no point in checking this code back in.