Closed
Bug 795376
Opened 12 years ago
Closed 12 years ago
Investigate using the high-quality scaler for upscaling too
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: joe, Assigned: luis)
References
Details
(Whiteboard: [lang=c++][mentor=joe@drew.ca])
Attachments
(5 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
joe
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
I'm working on this one :)
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → dae
Assignee | ||
Comment 2•12 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•12 years ago
|
||
Patch to use high-quality scaler both for downscaling and upscaling.
Running the quality and memory tests now.
Assignee | ||
Comment 4•12 years ago
|
||
As you can see in this image the difference of quality is noticeable.
Assignee | ||
Comment 5•12 years ago
|
||
The memory usage is smaller with the high-quality scaler.
Assignee | ||
Comment 6•12 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•12 years ago
|
||
and by standard scaler I meant Imagelib.
Reporter | ||
Comment 8•12 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•12 years ago
|
Assignee: dae → luis
Assignee | ||
Comment 9•12 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•12 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•12 years ago
|
||
John, we can make the limit bigger. What value do you suggest?
Comment 12•12 years ago
|
||
I think 15MB should be enough to cover all cases, including 2560x1600 displays.
Assignee | ||
Comment 13•12 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•12 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•12 years ago
|
||
As long as there's a limit, we can always tweak it later. Sounds good to me.
Assignee | ||
Comment 16•12 years ago
|
||
Fixed with review comments from Joe.
Attachment #728695 -
Flags: review?(joe)
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(joe)
Reporter | ||
Comment 17•12 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•12 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•12 years ago
|
Flags: needinfo?(joe)
Reporter | ||
Comment 20•12 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•12 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•12 years ago
|
||
Thanks for all the help! In both explaining the process and reviewing the patch :)
Reporter | ||
Updated•12 years ago
|
Attachment #729059 -
Flags: review?(joe)
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 23•12 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•12 years ago
|
Attachment #729069 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #728695 -
Attachment is obsolete: true
Assignee | ||
Comment 24•12 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
Comment 25•12 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 26•12 years ago
|
||
Thanks Matt :)
\o/ First patch
Comment 27•12 years ago
|
||
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
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cce3081b9bf6
https://hg.mozilla.org/mozilla-central/rev/d4239df4eac9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 29•11 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•11 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.
Description
•