Last Comment Bug 795376 - Investigate using the high-quality scaler for upscaling too
: Investigate using the high-quality scaler for upscaling too
Status: RESOLVED FIXED
[lang=c++][mentor=joe@drew.ca]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal with 5 votes (vote)
: mozilla22
Assigned To: Luis de Bethencourt [:luisbg]
:
Mentors:
Depends on: 486918
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-28 10:14 PDT by Joe Drew (not getting mail)
Modified: 2013-10-10 08:54 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to use high-quality scaler both for downscaling and upscaling. (1.52 KB, patch)
2013-03-23 16:26 PDT, Luis de Bethencourt [:luisbg]
no flags Details | Diff | Splinter Review
The difference in quality of the scaled image is noticeable (328.78 KB, image/png)
2013-03-23 16:33 PDT, Luis de Bethencourt [:luisbg]
no flags Details
memory usage comparison between scalers (1.28 KB, text/plain)
2013-03-23 16:37 PDT, Luis de Bethencourt [:luisbg]
no flags Details
quality scale up as well unless target size is bigger than 10mb (2.34 KB, patch)
2013-03-23 20:33 PDT, Luis de Bethencourt [:luisbg]
joe: review-
Details | Diff | Splinter Review
quality scale up as well unless target size is bigger than 20mb (3.21 KB, patch)
2013-03-23 21:44 PDT, Luis de Bethencourt [:luisbg]
joe: review+
Details | Diff | Splinter Review
new version of the patch with review fixes (3.97 KB, patch)
2013-03-25 10:43 PDT, Luis de Bethencourt [:luisbg]
no flags Details | Diff | Splinter Review
fixed comments to specify size in megapixels and resolution (4.04 KB, patch)
2013-03-25 11:00 PDT, Luis de Bethencourt [:luisbg]
joe: review+
Details | Diff | Splinter Review
s/amount of pixels/number of pixels (4.04 KB, patch)
2013-03-25 11:17 PDT, Luis de Bethencourt [:luisbg]
no flags Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2012-09-28 10:14:37 PDT
The high quality scaler imported as part of bug 486918 is capable of upscaling, but we only use it for downscaling. To see what results are like, you can modify RasterImage::CanScale in image/src/RasterImage.cpp to ignore aScale; you'll want to investigate both quality and memory usage, because we will hold on to the larger upscaled image.
Comment 1 Dae S. Kim 2012-10-17 15:28:41 PDT
I'm working on this one :)
Comment 2 Luis de Bethencourt [:luisbg] 2013-03-23 11:20:45 PDT
Hello Dae and Joe.

Are you working on a patch locally?

I would be happy to help if you don't have time to work on it.
Comment 3 Luis de Bethencourt [:luisbg] 2013-03-23 16:26:55 PDT
Created attachment 728669 [details] [diff] [review]
Patch to use high-quality scaler both for downscaling and upscaling.

Patch to use high-quality scaler both for downscaling and upscaling.

Running the quality and memory tests now.
Comment 4 Luis de Bethencourt [:luisbg] 2013-03-23 16:33:48 PDT
Created attachment 728670 [details]
The difference in quality of the scaled image is noticeable

As you can see in this image the difference of quality is noticeable.
Comment 5 Luis de Bethencourt [:luisbg] 2013-03-23 16:37:24 PDT
Created attachment 728671 [details]
memory usage comparison between scalers

The memory usage is smaller with the high-quality scaler.
Comment 6 Luis de Bethencourt [:luisbg] 2013-03-23 20:33:53 PDT
Created attachment 728693 [details] [diff] [review]
quality scale up as well unless target size is bigger than 10mb

Added some code to contain the situation where the upscaling can get too big in the memory.

When we use the high quality scaler the target size is stored in memory, and not in the GPU like the standar scaler, so we need to control this.
Comment 7 Luis de Bethencourt [:luisbg] 2013-03-23 20:36:28 PDT
and by standard scaler I meant Imagelib.
Comment 8 Joe Drew (not getting mail) 2013-03-23 20:46:39 PDT
Comment on attachment 728693 [details] [diff] [review]
quality scale up as well unless target size is bigger than 10mb

Review of attachment 728693 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks great. While I can't give it r+ yet (r+ for me means that it can go into mozilla-central as long as it passes Try[1] and any style nits I have are addressed), it's a very strong first patch. Congratulations!

When you address the below issues, upload another patch and ask me for review. I'll give it a once-over and then push it to Try for you.

Note: In future, you'll want to flag someone (for example, me!) for review below. We were in the middle of a conversation, so in this case it didn't matter, but in general it's very easy for this sort of thing to get lost in the deluge of bugmail people get every day.

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch has a lot more information. :)

1. https://wiki.mozilla.org/ReleaseEngineering/TryServer

::: image/src/RasterImage.cpp
@@ +2929,2 @@
>  {
> +  int32_t scaled_size;

In C++ this declaration can be anywhere, so you may as well put it down where you assign to it.

@@ +2934,1 @@
>      return false;

Here, surround the if's body with {} (like below); even though it's only one line, it *looks* like more than one line, so it's safer to put the braces in.

@@ +2935,5 @@
>  
> +  if (scale.width > 1.0 || scale.height > 1.0) {
> +    // To save memory don't quality upscale images bigger than 10mb.
> +    scaled_size = mSize.width * mSize.height * scale.width * scale.height;
> +    if (scaled_size > 2621440)

This part is going to need to change, likely to a preference value. You can look at how the preference "image.high_quality_downscaling.min_factor" is handled, and do exactly the same thing.

You'll also want to have this pref be stored in bytes, which means multiplying by 4 above. And as you have written this, it'll give us a warning because we're implicitly converting from a double to an integer. Silence that warning by explicitly casting.

@@ +3031,4 @@
>        frame = mScaleResult.frame;
>        userSpaceToImageSpace.Multiply(gfxMatrix().Scale(scale.width, scale.height));
>  
> +      // Since we're switching to a scaled image, we need to transform the

Looks like maybe you accidentally changed some whitespace here?
Comment 9 Luis de Bethencourt [:luisbg] 2013-03-23 20:57:19 PDT
Ah yes! I agree in all fronts.

I posted this to see if the logic and use of data made sense to you. I will move scaled_size to a preference value.
Comment 10 ferongr 2013-03-23 21:01:25 PDT
Does the size limit (assuming size calculation for RGBA) means that a user with a 2560x1600 display will rarely have the HQ upscaler kick in?
Comment 11 Luis de Bethencourt [:luisbg] 2013-03-23 21:18:28 PDT
John, we can make the limit bigger. What value do you suggest?
Comment 12 ferongr 2013-03-23 21:24:12 PDT
I think 15MB should be enough to cover all cases, including 2560x1600 displays.
Comment 13 Luis de Bethencourt [:luisbg] 2013-03-23 21:27:16 PDT
John, with the comment Joe made about multiplying the pixels by 4 to make it bytes. The value to compare against is 10,485,760.

This is higher than the amount of pixels in the screen size you mention.
Which is 4,096,000.
Comment 14 Luis de Bethencourt [:luisbg] 2013-03-23 21:30:41 PDT
15,728,640 bytes (15mb) / 4 (bytes per pixel) =  3,932,160
Your target display of 2560x1600 has 4,096,000 pixels.

We are short still by a little.

Joe and John, how about 20mb?
Comment 15 Joe Drew (not getting mail) 2013-03-23 21:39:40 PDT
As long as there's a limit, we can always tweak it later. Sounds good to me.
Comment 16 Luis de Bethencourt [:luisbg] 2013-03-23 21:44:06 PDT
Created attachment 728695 [details] [diff] [review]
quality scale up as well unless target size is bigger than 20mb

Fixed with review comments from Joe.
Comment 17 Joe Drew (not getting mail) 2013-03-25 09:22:23 PDT
Comment on attachment 728695 [details] [diff] [review]
quality scale up as well unless target size is bigger than 20mb

Review of attachment 728695 [details] [diff] [review]:
-----------------------------------------------------------------

You need to add the pref to modules/libpref/src/init/all.js as well.

Once you've done that and reuploaded your patch, I'll push this to try for you!

::: image/src/RasterImage.cpp
@@ +70,4 @@
>  static bool gHQDownscaling = false;
>  // This is interpreted as a floating-point value / 1000
>  static uint32_t gHQDownscalingMinFactor = 1000;
> +// The the amount of pixels in a 20mb image

s/The the amount/The number/

Also change "20 mb" to a number of megapixels; possibly 20 :)
Comment 18 Luis de Bethencourt [:luisbg] 2013-03-25 10:43:56 PDT
Created attachment 729059 [details] [diff] [review]
new version of the patch with review fixes

Thanks a lot for the review! :)

I have made the changes you mentioned.
20 megapixel is 80 mb. So changed it up to 80 mb and updated the comment to 20 megapixel. Sounds very big to me but that is what you suggested, as far as I understood.
Comment 19 Luis de Bethencourt [:luisbg] 2013-03-25 11:00:34 PDT
Created attachment 729069 [details] [diff] [review]
fixed comments to specify size in megapixels and resolution

Done :)
Comment 20 Joe Drew (not getting mail) 2013-03-25 11:09:19 PDT
Comment on attachment 729069 [details] [diff] [review]
fixed comments to specify size in megapixels and resolution

Review of attachment 729069 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! I've pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=2b37db614185

If that all comes back clean, you'll want to request checkin-needed as a keyword on this bug, and someone will get to it after not too long. To do so, be sure your patch is formatted correctly: https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

(Incidentally, this patch will also need to be rebased on top of current mozilla-central.)

Note that, when you're fixing nits or other review comments, you don't have to request another review (from me, anyways); r+ means "I trust you to make these changes and commit."

::: image/src/RasterImage.cpp
@@ +70,4 @@
>  static bool gHQDownscaling = false;
>  // This is interpreted as a floating-point value / 1000
>  static uint32_t gHQDownscalingMinFactor = 1000;
> +// The amount of pixels in a 5 megapixel decoded image.

nit: number of pixels.
Comment 21 Luis de Bethencourt [:luisbg] 2013-03-25 11:14:00 PDT
Cool!

I forgot to mention that I rebased the commit on current mozilla-central.
Since there was a change yesterday applied to the same file, and the number lines of the patch didn't match anymore.
Comment 22 Luis de Bethencourt [:luisbg] 2013-03-25 11:17:25 PDT
Created attachment 729082 [details] [diff] [review]
s/amount of pixels/number of pixels

Thanks for all the help! In both explaining the process and reviewing the patch :)
Comment 23 Luis de Bethencourt [:luisbg] 2013-03-25 18:10:45 PDT
To whoever checks this in, can you confirm with me please that you can pull the patch description/author easily from the git formatted patch?
Comment 24 Luis de Bethencourt [:luisbg] 2013-03-26 14:54:57 PDT
Comment on attachment 728669 [details] [diff] [review]
Patch to use high-quality scaler both for downscaling and upscaling.

>diff --git a/image/src/RasterImage.cpp b/image/src/RasterImage.cpp
>index 2546511..28be3c8 100644
>--- a/image/src/RasterImage.cpp
>+++ b/image/src/RasterImage.cpp
>@@ -2925,12 +2925,8 @@ RasterImage::SyncDecode()
> }
> 
> static inline bool
>-IsDownscale(const gfxSize& scale)
>+IsScaled(const gfxSize& scale)
> {
>-  if (scale.width > 1.0)
>-    return false;
>-  if (scale.height > 1.0)
>-    return false;
>   if (scale.width == 1.0 && scale.height == 1.0)
>     return false;
> 
>@@ -2946,8 +2942,9 @@ RasterImage::CanScale(gfxPattern::GraphicsFilter aFilter,
>   // We don't use the scaler for animated or multipart images to avoid doing a
>   // bunch of work on an image that just gets thrown away.
>   if (gHQDownscaling && aFilter == gfxPattern::FILTER_GOOD &&
>-      !mAnim && mDecoded && !mMultipart && IsDownscale(aScale)) {
>+      !mAnim && mDecoded && !mMultipart && IsScaled(aScale)) {
>     gfxFloat factor = gHQDownscalingMinFactor / 1000.0;
>+
>     return (aScale.width < factor || aScale.height < factor);
>   }
> #endif
>@@ -3024,7 +3021,7 @@ RasterImage::DrawWithPreDownscaleIfNeeded(imgFrame *aFrame,
>       frame = mScaleResult.frame;
>       userSpaceToImageSpace.Multiply(gfxMatrix().Scale(scale.width, scale.height));
> 
>-      // Since we're switching to a scaled image, we we need to transform the
>+      // Since we're switching to a scaled image, we need to transform the
>       // area of the subimage to draw accordingly, since imgFrame::Draw()
>       // doesn't know about scaled frames.
>       subimage.ScaleRoundOut(scale.width, scale.height);
Comment 26 Luis de Bethencourt [:luisbg] 2013-03-26 15:24:41 PDT
Thanks Matt :)

\o/ First patch
Comment 27 Matt Brubeck (:mbrubeck) 2013-03-26 15:44:32 PDT
I accidentally botched a few lines of the patch; pushed a follow-up fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4239df4eac9
Comment 29 Franpa_999 2013-10-09 21:49:22 PDT
Which version of Firefox first implemented the supplied patch? I've just noticed that the image quality when upscaling is indeed notably improved (Great work!), but upscaling still isn't excellent for stuff like comic strips and pixel art (Introduces some blur still).

Is it at all possible to get a optional nearest neighbor upscaler as that would work best for comic strips and pixel art amongst other stuff.

Thanks.
Comment 30 Luis de Bethencourt [:luisbg] 2013-10-10 08:54:54 PDT
The patch was merged in 2013-03-26. So when Firefox 23 was in nightlies.

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