+++ This bug was initially created as a clone of Bug #1070722 +++ URL: http://imgur.com/a/U8pSW Quick compare against chrome, we seem to do better during progressive decode. But once chrome has the images decoded their scrolling is smooth. Our is 30 FPs and stuttery. STR: 1) Open http://imgur.com/a/U8pSW. 2) Scroll top to bottom several time non stop until the image are all decoded. 3) Continue to scroll top to bottom non stop. Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=0bd840a020e2b2ecb071c68fc5a5b316aa492682#report=0bd840a020e2b2ecb071c68fc5a5b316aa492682

Fixed: http://people.mozilla.org/~bgirard/cleopatra/#report=0bd840a020e2b2ecb071c68fc5a5b316aa492682

Looks like most of the time is either optimizing into a SourceSurfaceCG, or reading back into a SourceSurfaceCairo when we decide to start a high-quality downscale. Both of those will actually be fixed by the second part of bug 1070722.

We probably have the same issue on other platforms (especially windows) though, seth should take a look at this.

(In reply to Matt Woodrow (:mattwoodrow) from comment #2) > reading back into a SourceSurfaceCairo when we decide to start a > high-quality downscale. This one really sucks because it gives us no benefit until the next paint (most likely). We should use a main thread runnable to do the readback for the hq downscale so at least it doesn't block the current paint. Ideally we could do the readback on the scaling thread instead of the main thread if it's possible.

Another thing we should probably do is introduce read only raw access ref's to imgFrame's that the hq downscaler can use so that we don't have to blow away the mOptSurface when the hq downscaler starts, only to recreate it with the exact same bits when the hq downscaler finishes. Also, any draws during this time won't get the benefit of the opt surface.

Yeah, a read-only RawAccessRef is a good idea and could certainly work! However, a "big picture" thought here: It'd be good to understand why we aren't converging to a stable state quickly here. I'm guessing here, as I haven't had a chance to debug this, but probably since scrolling triggers the issue, the problem is that HQ scaling is not triggered until each image comes into view. We only trigger HQ scaling on draw, so things outside of the viewport generally don't get HQ scaled. If that's the problem here, downscale-during-decode will fix this, because instead of decoding to the image's intrinsic size and then doing a separate HQ scaling step, we'll learn the size we should use from layout and produce a surface of that size directly. The fact that the images are not in the viewport will be irrelevant. For that reason, I think we are better off holding off on any imagelib-level fixes for now. Downscale-during-decode is probably the fix for this, and even if it isn't and there's another issue, the fix for *that* issue will likely look totally different post-downscale-and-decode. (It's not clear whether we'll even *support* deoptimizing frames at that point.)

I went ahead and made downscale-during-decode a blocker of this bug.

It shouldn't be to hard to cook up a patch that calls OptimalImageSizeForDest in the image visibility pass (the same thing that decides which images we keep around decoded versions of) with the right values so a hq scale can be kicked off before of painting. That would get us most of the way to a downscale-on-decode type solution. On the down side it will make us do more hq downscaling in general, and we'll have the scaled frame and the regular decoded frame hanging around.

Moving the readback onto the scaling thread seems like a reasonable thing to do in the short term.

(In reply to Timothy Nikkel (:tn) from comment #8) > It shouldn't be to hard to cook up a patch that calls > OptimalImageSizeForDest in the image visibility pass (the same thing that > decides which images we keep around decoded versions of) with the right > values so a hq scale can be kicked off before of painting. Yeah, that might be a good stopgap solution, and it should be pretty simple! We should probably go ahead and do that. (In reply to Matt Woodrow (:mattwoodrow) from comment #9) > Moving the readback onto the scaling thread seems like a reasonable thing to > do in the short term. I actually wanted to do this already, but I wasn't clear on whether it's safe, because I generally don't know what's OK to do off-main-thread with Moz2D. Matt, could you take a look at the Moz2D operations in imgFrame::LockImageData and imgFrame::UnlockImageData and let me know if there's anything main-thread-only there? If not, I'd be happy to write a patch to move the readback off-main-thread.

(In reply to Seth Fowler [:seth] from comment #10) > I actually wanted to do this already, but I wasn't clear on whether it's > safe, because I generally don't know what's OK to do off-main-thread with > Moz2D. Matt, could you take a look at the Moz2D operations in > imgFrame::LockImageData and imgFrame::UnlockImageData and let me know if > there's anything main-thread-only there? If not, I'd be happy to write a > patch to move the readback off-main-thread. Moving the ni? to Bas since he probably knows this better. We'd need to at least change SourceSurface to use threadsafe refcounting, but that's a known issue already, and an easy fix. I believe the D2D device and pixman are threadsafe.

As a general rule, Moz2D should be thread safe when objects are not shared between threads. In this case it might be the Snapshot that's not threadsafe... I think we still need to have a discussion about what we want the thread safety properties of Moz2D to be...

(In reply to Matt Woodrow (:mattwoodrow) (Away until 5 Jan) from comment #11) > (In reply to Seth Fowler [:seth] from comment #10) > > > I actually wanted to do this already, but I wasn't clear on whether it's > > safe, because I generally don't know what's OK to do off-main-thread with > > Moz2D. Matt, could you take a look at the Moz2D operations in > > imgFrame::LockImageData and imgFrame::UnlockImageData and let me know if > > there's anything main-thread-only there? If not, I'd be happy to write a > > patch to move the readback off-main-thread. > > Moving the ni? to Bas since he probably knows this better. > > We'd need to at least change SourceSurface to use threadsafe refcounting, > but that's a known issue already, and an easy fix. > > I believe the D2D device and pixman are threadsafe. There should be patches for the important bits in my multithreaded tiling patches.