1130802 - Make downscale-during-decode prefer decoded surfaces when substituting surfaces

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Always prefer decoded surfaces when substituting surfaces for downscale-during-decode

Here's the patch.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Always prefer decoded surfaces when substituting surfaces for downscale-during-decode

Tweaked the logic.

Comment 3

[Daniel Holbert [:dholbert]]

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)

Comment 4

[Seth Fowler [:seth] [:s2h]]

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. =)

Comment 5

[Daniel Holbert [:dholbert]]

Fair enough. :) Thanks, r=me

Comment 6

[Seth Fowler [:seth] [:s2h]]

Went ahead and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9984133216

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e9984133216