795376 - Investigate using the high-quality scaler for upscaling too

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

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]

I'm working on this one :)

Comment 2

[Luis de Bethencourt [:luisbg]]

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]]

Attachment: 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]]

Attachment: 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]]

Attachment: memory usage comparison between scalers

The memory usage is smaller with the high-quality scaler.

Comment 6

[Luis de Bethencourt [:luisbg]]

Attachment: 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]]

and by standard scaler I meant Imagelib.

Comment 8

[Joe Drew (not getting mail)]

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]]

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]

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]]

John, we can make the limit bigger. What value do you suggest?

Comment 12

[ferongr]

I think 15MB should be enough to cover all cases, including 2560x1600 displays.

Comment 13

[Luis de Bethencourt [:luisbg]]

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]]

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)]

As long as there's a limit, we can always tweak it later. Sounds good to me.

Comment 16

[Luis de Bethencourt [:luisbg]]

Attachment: 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)]

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]]

Attachment: 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]]

Attachment: fixed comments to specify size in megapixels and resolution

Done :)

Comment 20

[Joe Drew (not getting mail)]

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]]

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]]

Attachment: 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]]

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]]

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 25

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce3081b9bf6

Comment 26

[Luis de Bethencourt [:luisbg]]

Thanks Matt :)

\o/ First patch

Comment 27

[Matt Brubeck (:mbrubeck)]

I accidentally botched a few lines of the patch; pushed a follow-up fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4239df4eac9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cce3081b9bf6
https://hg.mozilla.org/mozilla-central/rev/d4239df4eac9

Comment 29

[Franpa_999]

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]]

The patch was merged in 2013-03-26. So when Firefox 23 was in nightlies.