Last Comment Bug 796174 - Don't use high-quality scaler for non-downscales
: Don't use high-quality scaler for non-downscales
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Joe Drew (not getting mail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-01 15:36 PDT by Joe Drew (not getting mail)
Modified: 2012-10-03 18:59 PDT (History)
3 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
ensure only downscales are scaled (1.13 KB, patch)
2012-10-01 15:36 PDT, Joe Drew (not getting mail)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2012-10-01 15:36:55 PDT
Created attachment 666747 [details] [diff] [review]
ensure only downscales are scaled

Right now we do a high-quality scale on unity scales. This is obviously wrong.
Comment 1 Justin Lebar (not reading bugmail) 2012-10-02 12:55:11 PDT
Comment on attachment 666747 [details] [diff] [review]
ensure only downscales are scaled

Heh.

I'm not sure what the extra function is buying us, but either way is fine.  If you keep the function, can you mark it static or put it in an anonymous namespace?
Comment 2 Joe Drew (not getting mail) 2012-10-02 13:31:30 PDT
I couldn't think of a clearer way to express "Only downscales" than having three conditions, and IME having a separate function is helpful when you start adding tons of conditions.
Comment 3 Justin Lebar (not reading bugmail) 2012-10-02 14:28:51 PDT
(In reply to Joe Drew (:JOEDREW! \o/) from comment #2)
> I couldn't think of a clearer way to express "Only downscales" than having
> three conditions, and IME having a separate function is helpful when you
> start adding tons of conditions.

I mean we already have a function saying "are we going to downscale this?", so if you wrote it as

 bool
 RasterImage::CanScale(gfxPattern::GraphicsFilter aFilter,
                       gfxSize aScale)
 {
 // The high-quality scaler requires Skia.
 #ifdef MOZ_ENABLE_SKIA
   if (!gHQDownscaling || mAnim || !mDecoded ||
       aFilter != gfxPattern::FILTER_GOOD ) {
     return false;
   }

   if (scale.width > 1.0 || scale.height > 1.0) {
     return false;
   }

   if (scale.width == 1.0 && scale.height == 1.0) {
     return false;
   }

   gfxFloat factor = gHQDownscalingMinFactor / 1000.0;
   return (aScale.width < factor || aScale.height < factor);
 #endif

That would be plenty clear to me.
Comment 4 Joe Drew (not getting mail) 2012-10-03 10:29:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/629c399c6f8c
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-10-03 18:59:53 PDT
https://hg.mozilla.org/mozilla-central/rev/6925601f4299

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