Investigate using the high-quality scaler for upscaling too

RESOLVED FIXED in mozilla22

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: luisbg)

Tracking

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][mentor=joe@drew.ca])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Blocks: 517294

Updated

5 years ago
No longer blocks: 517294

Comment 1

5 years ago
I'm working on this one :)
(Reporter)

Updated

5 years ago
Assignee: nobody → dae
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 728671 [details]
memory usage comparison between scalers

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

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
and by standard scaler I meant Imagelib.
(Reporter)

Comment 8

4 years ago
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?
Attachment #728693 - Flags: review-
(Reporter)

Updated

4 years ago
Assignee: dae → luis
(Assignee)

Comment 9

4 years ago
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

4 years ago
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?
(Assignee)

Comment 11

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

Comment 12

4 years ago
I think 15MB should be enough to cover all cases, including 2560x1600 displays.
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
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?
(Reporter)

Comment 15

4 years ago
As long as there's a limit, we can always tweak it later. Sounds good to me.
(Assignee)

Comment 16

4 years ago
Created attachment 728695 [details] [diff] [review]
quality scale up as well unless target size is bigger than 20mb

Fixed with review comments from Joe.
Attachment #728695 - Flags: review?(joe)
(Assignee)

Updated

4 years ago
Flags: needinfo?(joe)
(Reporter)

Comment 17

4 years ago
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 :)
Attachment #728695 - Flags: review?(joe) → review+
Flags: needinfo?(joe)
(Assignee)

Comment 18

4 years ago
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.
Attachment #729059 - Flags: review?(joe)
(Assignee)

Updated

4 years ago
Flags: needinfo?(joe)
(Assignee)

Comment 19

4 years ago
Created attachment 729069 [details] [diff] [review]
fixed comments to specify size in megapixels and resolution

Done :)
Attachment #729069 - Flags: review?(joe)
(Reporter)

Comment 20

4 years ago
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.
Attachment #729069 - Flags: review?(joe) → review+
Flags: needinfo?(joe)
(Assignee)

Comment 21

4 years ago
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.
(Assignee)

Comment 22

4 years ago
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 :)
(Reporter)

Updated

4 years ago
Attachment #729059 - Flags: review?(joe)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 23

4 years ago
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?
(Assignee)

Updated

4 years ago
Attachment #729069 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #728695 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
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);
Attachment #728669 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce3081b9bf6
Keywords: checkin-needed
(Assignee)

Comment 26

4 years ago
Thanks Matt :)

\o/ First patch
I accidentally botched a few lines of the patch; pushed a follow-up fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4239df4eac9
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/cce3081b9bf6
https://hg.mozilla.org/mozilla-central/rev/d4239df4eac9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Comment 29

4 years ago
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.
(Assignee)

Comment 30

4 years ago
The patch was merged in 2013-03-26. So when Firefox 23 was in nightlies.
You need to log in before you can comment on or make changes to this bug.