Last Comment Bug 598736 - Use higher-quality image interpolation on mobile
: Use higher-quality image interpolation on mobile
Status: RESOLVED FIXED
: mobile, perf
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: P1 normal with 3 votes (vote)
: mozilla6
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
: 637955 644204 (view as bug list)
Depends on: 598874 628324 648951 649041 650988 656782 669851 678223 750172
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-22 13:10 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-04-30 01:44 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
6+


Attachments
patch (1.93 KB, patch)
2011-04-01 10:50 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Review
patch v2 (2.04 KB, patch)
2011-04-01 16:07 PDT, Matt Brubeck (:mbrubeck)
jmuizelaar: review+
Details | Diff | Review
mobile-nearest-scaling-for-video.diff (874 bytes, patch)
2011-04-03 13:03 PDT, Siarhei Siamashka
mark.finkle: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Review
patch for checkin (2.05 KB, patch)
2011-04-08 10:16 PDT, Matt Brubeck (:mbrubeck)
mbrubeck: review+
Details | Diff | Review
patch v2 (3.18 KB, patch)
2011-05-24 12:08 PDT, Matt Brubeck (:mbrubeck)
jmuizelaar: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2010-09-22 13:10:05 PDT
When MOZ_GFX_OPTIMIZE_MOBILE=1, we always use nearest-neighbor resampling for images in content.  This makes scaled images look noticeably worse in Fennec than in other mobile or desktop browsers.  This is important for mobile because content is almost always zoomed in or out in Fennec.
Comment 1 Robert Longson 2010-09-22 15:41:10 PDT
removing this should do it... http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxDrawable.cpp#165
Comment 2 Robert Longson 2010-09-22 23:51:41 PDT
there's another one though...
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#2835
Comment 3 Oleg Romashin (:romaxa) 2010-09-22 23:55:45 PDT
(In reply to comment #2)
> there's another one though...
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#2835

This must be enabled for Pixman/X based scaling, otherwise our pinch zoom and content update will be much slower...

we should use GOOD filter only if it is optimized enough... (GL case for example)
Comment 4 Oleg Romashin (:romaxa) 2010-09-22 23:56:59 PDT
Does GOOD filter already works anywhere fast enough?
Comment 5 Siarhei Siamashka 2010-09-24 02:28:51 PDT
(In reply to comment #4)
> Does GOOD filter already works anywhere fast enough?

Currently it's approximately 10-30 times slower than NEAREST scaling for pixman-0.19.4 on ARM Cortex-A8:

Test using 2048x2048 image size (for all scaling operations source size ~= destination size):

== unscaled SRC ==
op=1, src_fmt=20028888, dst_fmt=20028888, speed=83.52 MPix/s
op=1, src_fmt=20028888, dst_fmt=10020565, speed=120.83 MPix/s
op=1, src_fmt=10020565, dst_fmt=10020565, speed=183.47 MPix/s

== nearest SRC ==
op=1, src_fmt=20028888, dst_fmt=20028888, speed=53.44 MPix/s
op=1, src_fmt=20028888, dst_fmt=10020565, speed=42.33 MPix/s
op=1, src_fmt=10020565, dst_fmt=10020565, speed=81.60 MPix/s

== bilinear SRC ==
op=1, src_fmt=20028888, dst_fmt=20028888, speed=4.53 MPix/s
op=1, src_fmt=20028888, dst_fmt=10020565, speed=4.39 MPix/s
op=1, src_fmt=10020565, dst_fmt=10020565, speed=2.25 MPix/s


Even with full NEON optimizations added, I expect that BILINEAR is still going to be about 2-4x slower than NEAREST. But indeed, NEON optimizations for bilinear scaling would be very nice to have.
Comment 6 Siarhei Siamashka 2010-09-24 02:40:27 PDT
That was measured on 720MHz OMAP3 device (IGEPv2 board) with gcc 4.5.1. Other ARM devices may have similar results with some variations due to CPU clock frequency, memory speed and compiler version. MPix/s numbers is easy to convert to FPS depending on target screen resolution.
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2010-09-28 22:25:14 PDT
blocking 2.0+ for figuring out what to do here
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-09-28 22:28:05 PDT
With GL compositing and bug 598874, we'll get this "for free" for content images.  The problem is harder without bug 598874 and with SW compositing.
Comment 9 Siarhei Siamashka 2010-12-01 05:24:30 PST
Would SIMD optimizations (SSE2, ARM NEON) for bilinear scaling in pixman be still somewhat useful after GL compositing solution is rolled out?
Comment 10 Stuart Parmenter 2011-01-10 23:12:16 PST
We won't block on this, but hopefully will get for free
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-01-20 11:07:08 PST
This is one of the worse polish issues we have in Fennec and GL will not make it for Fennec 4. So we'd like a plan to fix this for Fennec 4, even partially (on some devices), without a GL hail mary.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-01-20 11:08:49 PST
Are SIMD optimizations something we can look at for Fennec 4?
Comment 13 Brad Lassey [:blassey] (use needinfo?) 2011-01-20 11:18:43 PST
(In reply to comment #12)
> Are SIMD optimizations something we can look at for Fennec 4?

I'd skip SIMD and look at NEON
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2011-01-23 22:53:06 PST
I believe the issue is downscaling, which has always looked bad due to an unimplemented "good" algorithm.  That is, there's nothing to "make fast", afaik; have you tried not forcing nearest-neighbour and seeing if it actually gives you the quality you want?

Also, for this to really be performant, the downscaled images should be cached... and I just had a somewhat evil thought: in bug 622184, I already added a mechanism by which a decode can be forced if a frame is requested with different flags than the previous frame.  This mechanism could be extended to include a scale factor, triggering a re-decode whenever the image is zoomed, followed by a single high quality scale.  At draw time, the current frame's scale should be set on the gfxPattern, leading to the scale on the context cancelling it out and turning this into a fast 1:1 blit to the page..
Comment 15 Siarhei Siamashka 2011-01-24 03:48:39 PST
(In reply to comment #14)
> I believe the issue is downscaling, which has always looked bad due to an
> unimplemented "good" algorithm.

Downscaling is a different issue. I think this bug is about having nearest scaling enforced for mobile builds, which looks worse than bilinear. For upscaling too.

> That is, there's nothing to "make fast", afaik; have you tried not forcing nearest-neighbour and seeing if it actually gives you the quality you want?

Yes, bilinear scaling looks a lot better than nearest for the images and it's really hard not to notice.

> Also, for this to really be performant, the downscaled images should be
> cached...

Caching is not a magic pill. And it can easily make performance *worse*. Because between the moment when you enter URL, and the moment when you actually see the result on the screen, scaling operation has to be done anyway when there is a scaled image to display. Of course, caching of a downscaled image is reasonable if we have it ready as a by-product of rendering and there is enough RAM. Caching upscaled images is a bit questionable as they need more RAM and memory bandwidth.

Another case for fast scaling done in realtime is pinch zoom, but this probably has to keep using nearest scaling for software rendering.

I don't quite understand the rationale of your comment. Are you against getting SIMD optimizations for bilinear scaling? And what happens if the caching strategy fails? The previous bet was on GL compositing.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-01-24 10:55:04 PST
I think fast bilinear scaling on the CPU is going to be part of any plan here.  Filed bug 628324 for adding NEON a fast-path.
Comment 17 Vladimir Vukicevic [:vlad] [:vladv] 2011-01-24 22:01:43 PST
(In reply to comment #15)
> (In reply to comment #14)
> > I believe the issue is downscaling, which has always looked bad due to an
> > unimplemented "good" algorithm.
> 
> Downscaling is a different issue. I think this bug is about having nearest
> scaling enforced for mobile builds, which looks worse than bilinear. For
> upscaling too.

Yeah, but see below about common case..

> Caching is not a magic pill. And it can easily make performance *worse*.
> Because between the moment when you enter URL, and the moment when you actually
> see the result on the screen, scaling operation has to be done anyway when
> there is a scaled image to display. Of course, caching of a downscaled image is
> reasonable if we have it ready as a by-product of rendering and there is enough
> RAM.

Right, that was my thinking.  You can also free up memory by throwing away the 1:1 image.

> Caching upscaled images is a bit questionable as they need more RAM and
> memory bandwidth.

Yep, agree again; I was only thinking of downscaled here, since I assumed that was the fennec common case, no?  If the user zooms in past 1:1 then yeah, current bilinear upscaling will help, but I think zoomed in beyond 1:1 is the rare case..


> Another case for fast scaling done in realtime is pinch zoom, but this probably
> has to keep using nearest scaling for software rendering.
> 
> I don't quite understand the rationale of your comment. Are you against getting
> SIMD optimizations for bilinear scaling? And what happens if the caching
> strategy fails? The previous bet was on GL compositing.

Not at all; was just saying that having a faster version of what we already have won't necessarily improve the quality; e.g. I don't think it'll improve downscaling quality at all.  I think the right thing is to have fast scaling (up/down) *and* caching *and* higher quality downscaling :-)
Comment 18 Matt Brubeck (:mbrubeck) 2011-01-25 06:40:23 PST
(In reply to comment #17)
> Yep, agree again; I was only thinking of downscaled here, since I assumed that
> was the fennec common case, no?

Mobile-optimized sites (like m.engadget.com) are typically upscaled to 150% on the high-resolution devices that we are targeting, so that is a very common case too.  (And even on sites designed for large screens, Fennec users typically need to zoom in before actually reading or interacting with the page.)
Comment 19 Siarhei Siamashka 2011-01-25 06:48:39 PST
Yep, even though the resolution is lower on mobile devices, still DPI is typically higher there compared to desktop systems. So zoom in feature is used quite often.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2011-01-25 12:57:10 PST
Can someone pick this bug up and own it?
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-08 11:04:44 PST
Anyone got a patch for this bug in the works?
Comment 22 Stuart Parmenter 2011-02-09 10:26:45 PST
Not blocking on this -- nice to have, would take, but we need to fix our other blockers first
Comment 23 Siarhei Siamashka 2011-02-10 03:37:59 PST
My understanding is that solving this bug just requires changing the way how the use of nearest scaling instead of bilinear is configured. If I understand this correctly, right now it is decided at compile time. But shouldn't it actually be a runtime configuration option? So that the users can select whether they prefer better performance or better image quality.
Comment 24 Thomas Arend [:tarend] 2011-03-01 16:53:16 PST
I wonder if there is any connection at all to Bug 620808
Comment 25 Benjamin Stover (:stechz) 2011-03-01 17:05:33 PST
I think a fix for this could be really simple.

We could use bilinear filtering only in the content process, and nearest neighbor in the parent process. This gives us responsive scrolling and high-quality images, probably at a checkerboarding penalty. This might be good enough though.
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-01 17:57:32 PST
*** Bug 637955 has been marked as a duplicate of this bug. ***
Comment 27 Thomas Arend [:tarend] 2011-03-03 11:39:42 PST
Re: Comment 22 and Comment 25: do we have an estimate what "really simple" means? Is this a quick fix? We absolutely have to fix other blockers first. However, comparing us vs. stock makes us look really bad on most web pages that use images or logos.
Comment 28 Brad Lassey [:blassey] (use needinfo?) 2011-03-03 14:55:14 PST
Thomas, why did you put this in the nom queue? It is not blocking our release. If a patch comes along and is safe we can talk about taking it then.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-03 14:57:16 PST
The updated pixman might have NEON fast-paths for the operators we care about.  We should check after the new-cairo dep on bug 562746 is fixed; we might get this for free too.
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-03 14:58:24 PST
Should note that the version of pixman I'm looking at importing *doesn't* have Siarhei's patch listed in bug 628324 comment 10, we may not get what we need.
Comment 31 Siarhei Siamashka 2011-03-03 15:27:21 PST
(In reply to comment #30)
> Should note that the version of pixman I'm looking at importing *doesn't* have
> Siarhei's patch listed in bug 628324 comment 10, we may not get what we need.

Right. But also even that patch only adds a common optimized bilinear scaling framework and implements just one basic scaling operation (16bpp support is still missing). I have more patches available here which should significantly improve bilinear scaling support and cover more practical cases such as html5 games. I will send this patchset after a bit more testing and cleanup. Too bad it may be a bit late for the upcoming release of fennec.

Nevertheless almost *any* update of pixman should improve performance on ARM because there are some NEON optimizations added earlier.
Comment 32 Tony Chung [:tchung] 2011-03-17 17:25:15 PDT
adding some examples of what i see.
- Android Stock browser - http://www.flickr.com/photos/32283582@N00/5535596671/in/photostream/ (likely webkit-css; webkit-border-image)

- fennec nightly - http://www.flickr.com/photos/32283582@N00/5535596613/in/photostream/
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-23 10:14:53 PDT
*** Bug 644204 has been marked as a duplicate of this bug. ***
Comment 34 Siarhei Siamashka 2011-04-01 08:27:18 PDT
Would just a simple search&replace NEAREST to BILINEAR under MOZ_GFX_OPTIMIZE_MOBILE ifdefs be sufficient to resolve this issue? Like suggested in comment 1 and comment 2?
Comment 35 Matt Brubeck (:mbrubeck) 2011-04-01 10:50:37 PDT
Created attachment 523633 [details] [diff] [review]
patch

This works for me, and scrolling is still fast on my T-Mobile G2, even on complex sites with lots of images (like planet.mozilla.org).
Comment 36 Robert Longson 2011-04-01 11:48:28 PDT
I suspect you just want to remove the code in gfxDrawable.cpp.
Comment 37 Stuart Parmenter 2011-04-01 12:25:02 PDT
I think we only want to do this for images, not for all scaling?
Comment 38 Stuart Parmenter 2011-04-01 12:25:36 PDT
It would also be nice to be able to pull images out in to their own layers and use the GPU for scaling
Comment 39 Matt Brubeck (:mbrubeck) 2011-04-01 12:34:22 PDT
Comment on attachment 523633 [details] [diff] [review]
patch

We should test on non-NEON devices (e.g. Tegra) before making this change.

(In reply to comment #38)
> It would also be nice to be able to pull images out in to their own layers and
> use the GPU for scaling

This is bug 598874.
Comment 40 Matt Brubeck (:mbrubeck) 2011-04-01 16:07:54 PDT
Created attachment 523716 [details] [diff] [review]
patch v2

There does not seem to be any noticeable checkerboarding regression from this patch, even on an Atrix (Tegra processor, no NEON support).
Comment 41 Siarhei Siamashka 2011-04-01 17:25:27 PDT
(In reply to comment #40)
> There does not seem to be any noticeable checkerboarding regression from this
> patch, even on an Atrix (Tegra processor, no NEON support).

The most visible impact of slow (bilinear) scaling can be observed after pinch zoom. By checking how long it takes to re-render content with a new scale factor and change the picture on screen from jagged to smooth. A convenient web page for testing is http://images.google.com (just search for something first and then try zooming it).

Tegra2 is a relatively fast dualcore processor. I'm glad that it can still cope even without NEON and Mozilla does not have a strong need for better armv6 optimizations yet. And maybe they placed their bet on providing good GPU drivers so that nobody would have any need to resort to software rendering :)
Comment 42 Siarhei Siamashka 2011-04-02 19:43:37 PDT
(In reply to comment #40)
> Created attachment 523716 [details] [diff] [review]
> patch v2

Patch works fine for me with ordinary web pages full of images and fennec looks *much* better. But there is a some slowdown for html5 video playback. Unfortunately seems like all the fancy scaling operations mentioned in bug 641196 now need bilinear counterparts. The solution probably has to be one of the following:
1. fix bug 641196 and make video pipeline behave in a sane way
2. bite the bullet and provide additional NEON fast paths in pixman for these operations too, some of these can be also useful for html5 games which use bilinear scaling regardless of MOZ_GFX_OPTIMIZE_MOBILE define
3. somehow enforce nearest scaling for video, but use bilinear scaling for images
4. fix bug 634557 and do scaling early after decoding combined with YUV->RGB conversion, but without fixing video pipeline it will also cause some additional overhead in the video upscaling case because more data will need to be passed from 'plugin-container' to 'fennec' process.
Comment 43 Robert Longson 2011-04-03 00:26:44 PDT
3. Would be straightforward. You's just give the browser css something like

video { 
    image-rendering: -moz-crisp-edges;
}
Comment 44 Siarhei Siamashka 2011-04-03 03:38:58 PDT
Thanks. Which of the css files would be best to add this change for Fennec?
Comment 45 Robert Longson 2011-04-03 04:21:05 PDT
On firefox I think it would be browser.css. Does Fennec have that?
Comment 46 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-03 06:52:11 PDT
(In reply to comment #44)
> Thanks. Which of the css files would be best to add this change for Fennec?

in Fennec you probably want content.css - which is the CSS used for web content.
Comment 47 Siarhei Siamashka 2011-04-03 08:04:43 PDT
(In reply to comment #46)
> in Fennec you probably want content.css - which is the CSS used for web
> content.

Thanks, adding the chunk suggested by Robert to 'mobile/chrome/content/content.css' really helps. Should it be a part of this fix?

That said, I'm getting a bit spoiled and now would like to eventually also get bilinear scaling for video and pinch zoom in addition to static images. Hopefully with some more performance tuning it may become possible. But it's probably better to track these as a separate bug.
Comment 48 Matt Brubeck (:mbrubeck) 2011-04-03 08:26:42 PDT
(In reply to comment #47)
> That said, I'm getting a bit spoiled and now would like to eventually also get
> bilinear scaling for video and pinch zoom in addition to static images.
> Hopefully with some more performance tuning it may become possible. But it's
> probably better to track these as a separate bug.

We should get these when we enable OpenGL on mobile (bug 607684).
Comment 49 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-03 09:23:35 PDT
(In reply to comment #47)
> (In reply to comment #46)
> > in Fennec you probably want content.css - which is the CSS used for web
> > content.
> 
> Thanks, adding the chunk suggested by Robert to
> 'mobile/chrome/content/content.css' really helps. Should it be a part of this
> fix?

You can add a patch for mobile-broswer to this bug and we can land them together.
Comment 50 Siarhei Siamashka 2011-04-03 13:03:13 PDT
Created attachment 523911 [details] [diff] [review]
mobile-nearest-scaling-for-video.diff
Comment 51 Siarhei Siamashka 2011-04-03 13:16:48 PDT
(In reply to comment #48)
> (In reply to comment #47)
> > That said, I'm getting a bit spoiled and now would like to eventually also get
> > bilinear scaling for video and pinch zoom in addition to static images.
> > Hopefully with some more performance tuning it may become possible. But it's
> > probably better to track these as a separate bug.
> 
> We should get these when we enable OpenGL on mobile (bug 607684).

Is OpenGL going to be enabled soon? And will it work properly on all the supported devices?

I think an update for pixman NEON optimizations to address some of the newly discovered performance hotspots is quite doable in a timeframe of one or two weeks. Also I would prefer to ensure at least high quality playback of 360p webm videos from youtube without dropping frames on Nokia N900 (600MHz ARM Cortex-A8) because this annoyed me the most.
Comment 52 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-03 21:35:45 PDT
(In reply to comment #51)

> Is OpenGL going to be enabled soon? And will it work properly on all the
> supported devices?

NEON optimizations are welcome. Not every device will likely have OpenGL supported and we don't have a firm timeline for OpenGL yet, iirc.
Comment 53 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-08 09:35:05 PDT
Comment on attachment 523911 [details] [diff] [review]
mobile-nearest-scaling-for-video.diff

Looks OK
Comment 54 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-08 09:36:29 PDT
Jeff - Do you have time for this review? I'd like to get it on the next aurora push.
Comment 55 Jeff Muizelaar [:jrmuizel] 2011-04-08 09:39:14 PDT
Has anyone done tests to make sure the performance impact of this is acceptable?
Comment 56 Brad Lassey [:blassey] (use needinfo?) 2011-04-08 09:47:40 PDT
performance is acceptable on an atrix IMO, which is the only device without neon support
Comment 57 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-08 09:48:56 PDT
and if we get more feedback about poor performance from aurora users, we can easily back it out.
Comment 58 Matt Brubeck (:mbrubeck) 2011-04-08 09:51:33 PDT
(In reply to comment #55)
> Has anyone done tests to make sure the performance impact of this is
> acceptable?

We've done only eyeball testing so far.  We should also do Tp testing with Zipity, or on a project branch with mobile Talos coverage.  (I don't think we have mobile Talos on Try Server.)

There will definitely be some performance regression, so we need to define what is an "acceptable" trade-off of image quality for speed.  (Or as I like to say, how much are we willing to make the web look crappy in order to run faster?)
Comment 60 Matt Brubeck (:mbrubeck) 2011-04-08 10:16:40 PDT
Created attachment 524657 [details] [diff] [review]
patch for checkin
Comment 61 Brad Lassey [:blassey] (use needinfo?) 2011-04-08 10:16:55 PDT
(In reply to comment #59)
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e95166704b63&tochange=657c2a92ee2b

sorry, wrong bug
Comment 63 Doug Turner (:dougt) 2011-04-11 08:57:13 PDT
this patch causes a pretty huge regression in terms of repainting after the fuzzy zoom.  During a zoom, after you lift your fingers, there is around a 4 second delay before the repaint on the Tab.  For the Nexus S, it is a bit over 1s.  When I revert these changes, I see about 1-2 seconds for the Tab, and something under a second for the S.

reopening.  i think we should back this out.
Comment 64 Siarhei Siamashka 2011-04-11 09:10:19 PDT
First, can the use of bilinear scaling actually become a runtime configurable option (basically getting back to comment 23)?

Second question. Which web page are you using for testing? The delay you are observing there seems to be higher than normal.
Comment 65 Siarhei Siamashka 2011-04-11 09:13:39 PDT
(In reply to comment #63)
> When I revert these changes, I see about 1-2 seconds for the Tab, and
> something under a second for the S.

I mean, isn't 1-2 seconds time already very bad even without higher quality scaling? Something must be wrong.
Comment 66 Robert Longson 2011-04-11 09:16:46 PDT
(In reply to comment #64)
> First, can the use of bilinear scaling actually become a runtime configurable
> option (basically getting back to comment 23)?
> 

You can put disable it for images in content.css just like you did with video. Or as a user you can edit userContent.css to enable/disable it for whatever tags you want.
Comment 67 Siarhei Siamashka 2011-04-11 09:25:30 PDT
(In reply to comment #66)
> You can put disable it for images in content.css just like you did with video.
> Or as a user you can edit userContent.css to enable/disable it for whatever
> tags you want.

I seriously don't want to do that as a *user* of Fennec :) I would prefer a simple checkbox.
Comment 68 Doug Turner (:dougt) 2011-04-11 09:32:01 PDT
backed out until we can figure this out:

http://hg.mozilla.org/mozilla-central/rev/9a7c40dbd288
Comment 69 Doug Turner (:dougt) 2011-04-11 09:34:39 PDT
*** Bug 648951 has been marked as a duplicate of this bug. ***
Comment 70 Siarhei Siamashka 2011-04-13 13:07:45 PDT
Would it make some sense to treat this bug as a meta-bug and add a set of practical problems to solve as dependencies here (such as having bug 648951 as a dependency but not a duplicate)?
Comment 71 Siarhei Siamashka 2011-05-20 05:00:36 PDT
I just wonder if this bug may need to be somehow split to track GPU scaling and NEON scaling separately? Either of these can be probably sufficient.

But in order for NEON scaling to fly, some easy way for the users to enable higher quality scaling is needed. So that they can discover and report the use cases where performance is still not sufficient. Otherwise there will be no way to have good quality images when browsing (pixelated scaled images are very ugly, image galleries are quite painful to use right now), no bugreports in bugzilla and hence no progress.

Even now pixman 0.22.0 has more NEON optimizations for bilinear scaling than the built-in mozilla's copy. But they are likely not providing a complete coverage yet. Which optimizations exactly are still missing? This is yet to be figured out. But I guess http://ie.microsoft.com/testdrive/performance/fishietank/ would be a good start (there needs to be a bug for tracking improving its performance in Fennec).
Comment 72 Jeff Muizelaar [:jrmuizel] 2011-05-20 05:03:12 PDT
I actually merged upstream pixman into mozilla-central yesterday, so it might be worth trying this out again.
Comment 73 Siarhei Siamashka 2011-05-20 06:28:02 PDT
(In reply to comment #72)
> I actually merged upstream pixman into mozilla-central yesterday, so it
> might be worth trying this out again.

OK, thanks. That's great.
Comment 74 Matt Brubeck (:mbrubeck) 2011-05-24 11:22:50 PDT
Most of the regression caused by this patch is now fixed by the latest pixman changes (see bug 648951 comment 12).

We can't aim for zero regression:  Nearest-neighbor scaling will always be somewhat faster (and uglier) than bilinear.  But at the current level of performance I think the trade-off is well worth it.  Can we re-land this patch now?  If not, what criteria would people like to see for landing?

Are there specific metrics we need as additional input here, and if so, how will those metrics be used to make a decision?  What are the key test cases, and how slow would they need to be for us to stick with low-quality scaling?
Comment 75 Siarhei Siamashka 2011-05-24 11:31:12 PDT
The patches probably need to be modified to have bilinear scaling enabled only if NEON is available. Otherwise the performance will be really bad on Tegra2 devices, as indirectly tested by bug 648951
Comment 76 Doug Turner (:dougt) 2011-05-24 11:35:56 PDT
matt, you going to modify the patch like Siarhei mentioned.  Also, this hurt perf last time you landed.  (specifically on the Galaxy Tab).  Do you have new data that suggests that this isn't a probably any longer?
Comment 77 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-24 11:39:36 PDT
(In reply to comment #76)
> matt, you going to modify the patch like Siarhei mentioned.  Also, this hurt
> perf last time you landed.  (specifically on the Galaxy Tab).  Do you have
> new data that suggests that this isn't a probably any longer?

matt, stopwatch times on the galaxy tab _with_ the patches would be enough for me. we can make a decision based on those numbers.
Comment 78 Siarhei Siamashka 2011-05-24 11:54:32 PDT
> stopwatch times on the galaxy tab _with_ the patches would be enough for me. we can make a decision based on those numbers.

stopwatch times are available in bug 648951 comment 12

oprofile data is available in bug 648951 comment 6
Comment 79 Matt Brubeck (:mbrubeck) 2011-05-24 12:08:37 PDT
Created attachment 534857 [details] [diff] [review]
patch v2

Updated to use bilinear/good scaling on devices that support NEON, while sticking with nearest/fast for all other devices (when MOZ_GFX_OPTIMIZE_MOBILE is enabled).

In addition to the tests on Galaxy tab referenced above, tested on Motorola Xoom (Tegra 2, no NEON support).  With the old patch from this bug, the Xoom used bilinear scaling and the test case from bug 648951 takes >1s.  With this new patch, the Xoom continues to use nearest-neighbor scaling and zooming latency is <0.5s (same as trunk without any patches).
Comment 80 Jeff Muizelaar [:jrmuizel] 2011-05-24 12:10:55 PDT
Comment on attachment 534857 [details] [diff] [review]
patch v2

Does mozilla::supports_neon() work on non-arm platforms?
Comment 81 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-24 12:13:51 PDT
> > stopwatch times on the galaxy tab _with_ the patches would be enough for me. we can make a decision based on those numbers.
> 
> stopwatch times are available in bug 648951 comment 12
> 
> oprofile data is available in bug 648951 comment 6

based on those numbers, a=mfinkle to land for additional testing, once you get r+ on the new patch.
Comment 82 Siarhei Siamashka 2011-05-24 12:17:35 PDT
(In reply to comment #80)
> Does mozilla::supports_neon() work on non-arm platforms?

yes, that was discussed in bug 656797 comment 4
Comment 83 Matt Brubeck (:mbrubeck) 2011-05-24 12:21:25 PDT
(In reply to comment #80)
> Does mozilla::supports_neon() work on non-arm platforms?

Yes.  On platforms without MOZILLA_MAY_SUPPORT_NEON, it is defined as:

  inline bool supports_neon() { return false; }
Comment 84 Matt Brubeck (:mbrubeck) 2011-05-24 12:57:43 PDT
http://hg.mozilla.org/projects/cedar/rev/a45a4abce7ce
Comment 85 Matt Brubeck (:mbrubeck) 2011-05-24 15:04:23 PDT
http://hg.mozilla.org/mozilla-central/rev/a45a4abce7ce
Comment 86 Matt Brubeck (:mbrubeck) 2011-05-24 15:20:30 PDT
Note that although this bug is resolved, the current solution (software scaling with NEON optimizations) works only on devices with NEON support.

Devices without NEON (any device with a Tegra processor, including the Atrix, Xoom, Iconia A500, Vega, Optimus 2X, Optimus Pad, G Tablet, etc.) are still using nearest neighbor scaling.  Bug 650988 looks like the most likely plan to fix this.

(I'm curious what WebKit and Opera do on these devices to get fast, high-quality scaling without NEON.  Are they using the GPU, or just better software scaling?)
Comment 87 Siarhei Siamashka 2011-05-24 16:53:45 PDT
(In reply to comment #86)
> I'm curious what WebKit and Opera do on these devices to get fast,
> high-quality scaling without NEON.

Tegra2 still supports ARMv6 style SIMD instructions, which can also provide some measurable speedup for bilinear scaling (not implemented in pixman, and I myself have no plans for doing such optimizations at the moment).

Another thing is that bilinear scaling r5g6b5->r5g6b5 is slower than a8r8g8b8->a8r8g8b8 or a8r8g8b8->r5g6b5. That's because interpolation is done with a8r8g8b8, and some extra color format conversions may be involved. So keeping images in a8r8g8b8 format may be faster if they are used as the source images for bilinear scaling (with its own disadvantages: bigger RAM footprint and slower non-scaled blits).
Comment 88 Matt Brubeck (:mbrubeck) 2011-06-02 10:05:34 PDT
Comment on attachment 523911 [details] [diff] [review]
mobile-nearest-scaling-for-video.diff

Missed this patch earlier; pushed to mozilla-central now for Firefox 7:
http://hg.mozilla.org/mozilla-central/rev/4ed9e3297485

Requesting mozilla-approval-aurora for both patches in this bug.  This is a top user complaint, and it is a competiveness/parity issue (makes Firefox visibly worse than all other Android browsers).  Both patches affect mobile only.

Note You need to log in before you can comment on or make changes to this bug.