Testing downscale-during-decode on the Flame, where I was using pinch-to-zoom instead of click-to-zoom, made me realize that downscale-during-decode should always prefer to substitute a fully-decoded version of a surface if it has to substitute surfaces. Otherwise, you can end up in a situation where the surface is you're substituting is *also* undecoded or mostly undecoded, and you'll end up looking at a black screen for a second until the decoder catches up. That's not the experience we want; we want to be putting *something* on the screen whenever possible. In this bug I'll make TryToImproveMatch always prefer completely decoded surfaces when one is available.

Here's the patch.

Tweaked the logic.

Comment on attachment 8560948 [details] [diff] [review] Always prefer decoded surfaces when substituting surfaces for downscale-during-decode >+ // Always prefer completely decoded surfaces. >+ bool bestMatchIsDecoded = context->mBestMatch->IsDecoded(); >+ if (bestMatchIsDecoded && !aSurface->IsDecoded()) { >+ return PL_DHASH_NEXT; >+ } >+ if (!bestMatchIsDecoded && aSurface->IsDecoded()) { >+ context->mBestMatch = aSurface; >+ return PL_DHASH_NEXT; >+ } This might be better structured with a "bestMatchIsDecoded != aSurface->IsDecoded()" check, to reduce calls to IsDecoded(), return statements, and (non-comment) lines of code. Something like: if (bestMatchIsDecoded != aSurface->IsDecoded()) { if (!bestMatchIsDecoded) { // aSurface is decoded & best match so far is not. Prefer aSurface. context->mBestMatch = aSurface; } // else, best match is decoded & aSurface isn't. Skip aSurface. return PL_DHASH_NEXT; } r=me with that if you agree. (Probably r=me if you disagree too, but I'd be curious why)

Thanks for the review! (In reply to Daniel Holbert [:dholbert] from comment #3) > This might be better structured with a "bestMatchIsDecoded != > aSurface->IsDecoded()" check, to reduce calls to IsDecoded(), return > statements, and (non-comment) lines of code. I can see why you think so - I considered that possibility too! I must've tried four or five different ways to structure this logic. However, while the approach in comment 3 does have the advantage that we can merge the |return PL_DHASH_NEXT;| statements from the different branches, I ultimately decided that the version in the patch was more readable. It seemed to me to be the clearest expression of what the rules actually were. I recognize that that's somewhat subjective. =)

Fair enough. :) Thanks, r=me

Went ahead and pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9984133216