Add mozImageSmoothingEnabled property to canvas

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

({dev-doc-complete})

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

This is modeled on the SVG image-rendering property, at http://www.w3.org/TR/SVG/painting.html#ImageRenderingProperty:

 attribute string imageRenderingQuality;

'auto' (default): The user agent shall make appropriate tradeoffs to balance speed and quality, but quality shall be given more importance than speed.

'optimizeQuality': Emphasize quality over rendering speed.

'optimizeSpeed': Emphasize speed over rendering quality.

No specific image sampling algorithm is specified for any of these properties, with the exception that, at a minimum, nearest-neighbour resampling should be used.  One alternative is to specify 'best', 'good', 'fast', with "good" being the default, as opposed to the SVG names; I think those names are more descriptive, but there might be value in keeping the names consistent with SVG, especially if that property bubbles up into general CSS usage.
Add imageRenderingQuality to the canvas 2d context, and have 'optimizeSpeed' translate into CAIRO_FILTER_FAST.
Attachment #323966 - Flags: review?(roc)
Should this be mozImageRenderingQuality?

@@ -2728,6 +2744,12 @@ nsCanvasRenderingContext2D::CreateImageD
     rv = ncc->GetJSContext(&ctx);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    PRUint32 argc;
+    jsval *argv = nsnull;
+
+    ncc->GetArgc(&argc);
+    ncc->GetArgvPtr(&argv);
+
     JSAutoRequest ar(ctx);
 
     int32 w, h;

I presume this hunk was not intended here.
Any chance this could go in for the next release of firefox?  I've ran into an IBM issue where we were resizing images in the canvas and needed the resizing to not try to smooth the image (it was making some images with text in them less readable for example).
(In reply to comment #2)
> Should this be mozImageRenderingQuality?

I think vlad was trying to get this added to the spec
Doron, this is not intended to guarantee that "low" or "optimizeSpeed" gives nearest-neighbour sampling, so I would not recommend using it for your case.

After the WHATWG discussion about this, I'm kind of down on the idea. I'd much prefer to have compositor and do automatic quality reduction when necessary to sustain the desired frame rate.
So would it be worth filing a bug about turning off image resize "smoothing" or is that something that probably won't go in?
Maybe you want to use the imagedata API, load your image into a canvas and then manually upscale by drawing one rect for each pixel.
(In reply to comment #7)
> Maybe you want to use the imagedata API, load your image into a canvas and then
> manually upscale by drawing one rect for each pixel.

This is probably a bad idea -- it ensures that you'll get horribly slow performance if you ever need to do quick animation.  But yes, the intent isn't to force nearest-neighbour (though perhaps a way to do that should be added, since I think an interpolation on/off switch is generally useful).

I guess I need to catch up on the whatwg discussion; I think that this API makes sense, because even in the presence of a compositor that can take all things into account, there is value in the author being able to say "I'm ok with a lower quality rendering here, just make it fast", or conversely, "I want this to be the highest quality possible, regardless of the time it takes".  We should not take that control away from the author.
Comment on attachment 323966 [details] [diff] [review]
add imageRenderingQuality

I don't think the imagedata part of your test is appropriate, since as we discussed, there's no reason optimizeSpeed should necessarily disable sampling. We need something else for that.

Other than that, this is OK, but I think I prefer Philip's suggestion on the list to just support "auto" and "high" values. And I don't think we should ship this without slightly more WHATWG consensus than we've got.
Attachment #323966 - Flags: review?(roc)
So maybe we don't do imageRenderingQuality, but instead just have a boolean imageSmoothingEnabled property?  There are valid reasons to disable image smoothing for visual effects anyway, so that seems like it would be simpler.
We'd love to get some kind of filter property to support zooming in Fennec. Currently, scaling with drawWindow takes about 220ms per call on an N8x0. We want to animate the zoom, so we make several calls over a short time period. These scaled images should be "optimizeSpeed" and the final scaled image can be "optimizeQuality"
I don't think imageSmoothingEnabled directly addresses the use cases that inspired this bug, but I certainly think it's a reasonable thing to add. Call it mozImageSmoothingEnabled until we get some more WHATWG feedback.
Well I guess it does "directly address" them, but it's a less appropriate tool for the job than comment #0 or comment #9.
(In reply to comment #13)
> Well I guess it does "directly address" them, but it's a less appropriate tool
> for the job than comment #0 or comment #9.

Yeah; the original job was really "have a way for fennec to specify that it wants fast image scaling"; I wanted to do that without hardcoding nearest-neighbour.  The Quality idea turned into a bit of a quagmire, so we should just add it directly.
Posted patch add mozImageSmoothingEnabled (obsolete) — Splinter Review
Simpler variant to get us to the same end state as the other approach to this bug.  Add a boolean to indicate whether image smoothing/filtering shuold be enabled or not.  If it's not enabled, nearest neighbour sampling is used.  Enabled by default to match current state.
Attachment #341173 - Flags: review?(roc)
Comment on attachment 341173 [details] [diff] [review]
add mozImageSmoothingEnabled

+        nsCanvasRenderingContext2D::ImageRenderingQuality imageRenderingQuality;

We don't want this, nor the definition of ImageRenderingQuality.

+    retVal = ((CurrentState().imageSmoothingEnabled)) == PR_TRUE;

Just "retVal = CurrentState().imageSmoothingEnabled"

+        PRBool imageSmoothingEnabled;

PRPackedBool
Forgot to qrefresh.
Attachment #341173 - Attachment is obsolete: true
Attachment #341177 - Flags: review?(roc)
Attachment #341173 - Flags: review?(roc)
Now with qrefresh and review comments addressed!
Attachment #341177 - Attachment is obsolete: true
Attachment #341178 - Flags: review?(roc)
Attachment #341177 - Flags: review?(roc)
Summary: Add imageRenderingQuality property to canvas → Add mozImageSmoothingEnabled property to canvas
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Reopening -- this was failing on Mac, likely the a bug in the quartz surface not passing the filter down correctly.  Argh. :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Vlad - any time to look at this?
(In reply to comment #20)
> Reopening -- this was failing on Mac, likely the a bug in the quartz surface
> not passing the filter down correctly.  Argh. :(

cairo-quartz doesn't treat |CAIRO_FILTER_FAST| as |CAIRO_FILTER_NEAREST|.
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-quartz-surface.c#446

CAIRO_FILTER_FAST -> kCGInterpolationLow
CAIRO_FILTER_NEAREST -> kCGInterpolationNone

Should we use |CAIRO_FILTER_NEAREST| (ie. 3) for this?
# Actually I don't have any mac boxes so I can't test it :(
Ah, that's probably it.  I should probably change that in the quartz surface as well.  Will fix and test, still have time to get this in.
http://hg.mozilla.org/mozilla-central/rev/cfd5af38abc6 -- FAST/NEAREST was indeed the problem on OSX.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.