Investigate using the high-quality scaler for upscaling too

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: joe, Assigned: luis)

Tracking

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

7 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.
Blocks: 517294
No longer blocks: 517294

Comment 1

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

Updated

7 years ago
Assignee: nobody → dae
Assignee

Comment 2

6 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

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

Running the quality and memory tests now.
Assignee

Comment 4

6 years ago
As you can see in this image the difference of quality is noticeable.
Assignee

Comment 5

6 years ago
The memory usage is smaller with the high-quality scaler.
Assignee

Comment 6

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

6 years ago
and by standard scaler I meant Imagelib.
Reporter

Comment 8

6 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

6 years ago
Assignee: dae → luis
Assignee

Comment 9

6 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

6 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

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

Comment 12

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

Comment 13

6 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

6 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

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

Comment 16

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

Updated

6 years ago
Flags: needinfo?(joe)
Reporter

Comment 17

6 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

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

6 years ago
Flags: needinfo?(joe)
Reporter

Comment 20

6 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

6 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

6 years ago
Thanks for all the help! In both explaining the process and reviewing the patch :)
Reporter

Updated

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

Updated

6 years ago
Keywords: checkin-needed
Assignee

Comment 23

6 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

6 years ago
Attachment #729069 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #728695 - Attachment is obsolete: true
Assignee

Comment 24

6 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
Assignee

Comment 26

6 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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Comment 29

6 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

6 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.