Closed Bug 484150 Opened 11 years ago Closed 11 years ago

Support image-rendering attribute

Categories

(Core :: SVG, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
http://www.w3.org/TR/SVG11/painting.html#ImageRenderingProperty

See also bug 436932

I think this will initially only do things on Windows. Linux has bug 422179 and according to bug 436932 Mac doesn't work.

I'll ask roc or maybe vlad for a sr to cover the SVG bit that actually does the work.
Attachment #368213 - Flags: review?(dbaron)
Did you run the mochitests in layout/style (and did they pass)?
Attachment #368213 - Flags: review?(dbaron) → review+
Comment on attachment 368213 [details] [diff] [review]
patch

From a CSS perspective, the code looks fine, although I'm not sure if it's really a good idea to implement this property; is this really something we want authors to control?  I suppose given the 'auto' value it's probably ok.
Something like this would give authors the control they want in bug 423756; but I really don't like "optimizeSpeed" vs. "optimizeQuality" in the spec... they really don't have any kind of meaning that authors can depend on, beyond just hints that can be ignored.
Attached patch updated patch (obsolete) — Splinter Review
Adds two missing lines. This version passes all the tests in layout/style.
Attachment #368213 - Attachment is obsolete: true
Attachment #368332 - Flags: superreview?(roc)
image-rendering really needs an additional setting, something like disableResampling that would make it more dependable. Perhaps jwatt can take that up with the SVG wg. At the moment this is as close as we can get to as a cross-browser solution.
Attached patch updated patch (obsolete) — Splinter Review
remove repeated redundant line from patch.
Attachment #368332 - Attachment is obsolete: true
Attachment #368362 - Flags: superreview?(roc)
Attachment #368332 - Flags: superreview?(roc)
We're free to add a -moz-disable-resampling value. I'd support that.
Comment on attachment 368362 [details] [diff] [review]
updated patch

Looks good.
Attachment #368362 - Flags: superreview?(roc) → superreview+
Actually it would be awesome if we supported this for <img> too ... and if we supported -moz-disable-resampling.
Yep, I was just thinking the same thing, we should be able to add a -moz value here.  -moz-disable-filtering? or resampling?  Then we can support it on <img>, <canvas> and wherever else we render image-like things.
Attached patch with -moz-disable-resampling (obsolete) — Splinter Review
I'll meet you halfway. This includes the style changes for -moz-disable-resampling but it still only does SVG images.
Attachment #368362 - Attachment is obsolete: true
Attachment #368449 - Flags: review?(dbaron)
Oh, and it still passes all the layout/style mochitests.
Attachment #368449 - Flags: review?(dbaron) → review+
Comment on attachment 368449 [details] [diff] [review]
with -moz-disable-resampling

I'm not sure how -moz-disable-resampling differs from optimizeSpeed, but I'll trust roc and vlad here.

One minor comment is that you might want to leave the "MOZ_" out of the NS_STYLE_IMAGE_RENDERING_MOZ_DISABLE_RESAMPLING constant name.

r=dbaron
On a hypothetical heavily GPU-accelerated graphics backend, optimizeSpeed might do something really fancy because it's "free". disable-resampling would still be required to do nearest-neighbour sampling.

This patch should actually pass CAIRO_FILTER_NEAREST for disable-resampling, though.
Attached patch updated patchSplinter Review
addresses review comments and includes part of patch in bug 423756 for increased readability
Attachment #368449 - Attachment is obsolete: true
Checked in http://hg.mozilla.org/mozilla-central/rev/8b8f4cc66c80
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
For html I can see
nsImageFrame::PaintImage 
  nsLayoutUtils::DrawSingleImage
        ... 
other internal nsLayoutUtils functions
    nsThebesImage::Draw

I can get the css style in nsImageFrame but I don't have a pattern till nsThebesImage::Draw.
Oops a bit early there.

Anyway, what's the best way to get the filter from the image frame to nsThebesImage?

extra argument? not sure how many callers I'd have to change.
extra methods in nsIRenderingContext and/or nsIImage to store/retrieve the filter state?
some combination of the above?

This is really bug 423756 but there's 43 people would get spammed there.
For <img> (and CSS background images, which we should probably also handle), I think adding a parameter to nsLayoutUtils::Draw* and nsThebesImage::Draw is the best approach.
Also, could you mention this extension to www-svg?
(In reply to comment #20)
> Also, could you mention this extension to www-svg?

I've already done that.
-moz-disable-resampling has become -moz-crisp-edges now that bug 423756 has landed.
... but with the canvas specific property (mozImageSmoothingEnabled), it works well on Linux.
Firstly, you have the wrong bug. This one is for the image-rendering attribute on SVG images, not html images. html is bug 423756. Posting to that bug will spam a lot of people though.

Secondly what Firefox version are you testing on because the earliest this is supported in is Firefox 3.6

Thirdly it does not work on Linux. See bug 423756 comment 44 for why.
Also you are scaling the images smaller so you've hit bug 486918. You need to scale images larger than they are to see an effect.
You need to log in before you can comment on or make changes to this bug.