Closed Bug 414685 Opened 14 years ago Closed 14 years ago

image downscaling slower than fx2

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file scaling benchmark
Spin-off bug from bug 412396.  Attached is a benchmark for image scaling; needs the JPEG subdir as from the other bug.

One problem is that the benchmark doesn't seem to work in Fx2 -- the scrollTo()'s are just ignored, so I can't get useful numbers.

Looking at the profile from Fx3, we're spending all our time deep inside Quartz, which is where things should be; I'm not sure what wins we can do here, but I need to see the Fx2 numbers first.  Working on figuring out how to get that.
Flags: blocking1.9+
Attached patch patch (obsolete) — Splinter Review
welp.  I thought CGBitmapContextCreateImage was fast (just returning a reference to an internal image), guess not!  Let's cache it for wins.
Attachment #300205 - Flags: review?(roc)
Attached patch patch, part 2 (obsolete) — Splinter Review
Part 2 of the patch, to make it play nice with the image loading optimization in the other bug.  Without this, we end up caching an early version of the image data, and bad things happen.

I may have a third patch here that might help with memory usage as well.
Attachment #300206 - Flags: review?(roc)
Comment on attachment 300205 [details] [diff] [review]
patch

Looks good but please document the ownership rules here, namely that contextImage has an associated ref to the image. Also document that it's imperative for performance that we drop all references to the contextImage before we draw to the surface.

Sometime you should explain to me why this is important for scaled image performance but not normal images.
Attachment #300205 - Flags: review?(roc) → review+
Actually, explaining that in some comments in the code would be good in case anyone wonders why you're doing this.
For performance, shouldn't ImageUpdated be ImageWillUpdate so we can wipe out the image before we change the surface?
Comment on attachment 300206 [details] [diff] [review]
patch, part 2

Using ImageUpdated is going to be fragile.  I'm pretty sure there are already places that need this called.  It would be a lot nicer to have a read only surface that was copy on write or something...
Attachment #300206 - Flags: review?(roc) → review-
The Quartz image is copy-on-write (or something like that, says the documentation). The problem is, we don't want the copy to happen because a) that's expensive and b) it would be incorrect for us to use the stale copy. So we have to do some kind of ImageUpdated/ImageWillUpdate anyway.
Attached patch door #2Splinter Review
No review requests yet, need to run benchmarks tomorrow first.
Attachment #300322 - Flags: review?(roc) → review?(pavlov)
Attachment #300322 - Flags: review?(pavlov) → review+
Comment on attachment 300322 [details] [diff] [review]
door #2

Approve this for big wins.
Attachment #300322 - Flags: approval1.9b3?
Comment on attachment 300322 [details] [diff] [review]
door #2

a=beltzner for 1.9, but not for beta, sorry - gotta keep some of our perf wins for later! :)
Attachment #300322 - Flags: approval1.9b3?
Attachment #300322 - Flags: approval1.9b3-
Attachment #300322 - Flags: approval1.9+
Checked in.  Sadly, no massive Tp drop!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Confirming a 20-50x speedup from yesterday's trunk to today on the scale benchmark.
Depends on: 416217
You need to log in before you can comment on or make changes to this bug.