Closed Bug 1072703 Opened 10 years ago Closed 3 years ago

Make "image-rendering: pixelated" use a better downscaling algorithm

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1731134

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

part 1 v2: Add GraphicsFilter enum, plus /layout & /image code
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
part 2 v2: handle "pixelated" in DrawTargetCairo.cpp
(deleted), patch
Details | Diff | Splinter Review
reftest patch
(deleted), patch
seth
: review+
Details | Diff | Splinter Review
(image file for use in testcase)
(deleted), image/png
Details
testcase 1
(deleted), text/html
Details
screenshot of testcase 1 in Firefox Nightly 94 on Linux
(deleted), image/png
Details
Per the first resolution in http://lists.w3.org/Archives/Public/www-style/2014Sep/0384.html , "image-rendering:pixelated" will optionally allow a smarter downscaling algorithm (smarter than nearest-neighbor). I'm filing this bug on adding that smarter-downscaling logic. We'll need to do something along the lines of what I describe here, in my dev.platform post (responding to dbaron, on my Intent to implement: "image-rendering: pixelated" thread): https://groups.google.com/d/msg/mozilla.dev.platform/0KYBjCdUMJw/IgMGOqm5QZcJ One motivation for this: favicons, where we'd like to pixellate small images when upscaling (particularly on HiDPI screens), but for too-large favicons, we'd like a smarter downscaling algorithm. See e.g. bug 839923 comment 0 and bug 1041845 comment 16 for more on this use-case.
We discussed this on IRC, but I think it's worth reiterating it here in case there's a delay before anyone actually works on this: It's important that we not try to simplify away this filter type outside of imagelib. (We shouldn't, for example, decide whether we're upscaling or downscaling in nsLayoutUtils and switch to explicitly requesting bilinear filtering or nearest-neighbor when calling imgIContainer::Draw.) The reason is that in the future it's going to be increasingly difficult to tell, before calling Draw, whether or not we're upscaling or downscaling. We could think we're going to downscale, but then (for example) the OS could free the volatile buffer containing our surface, and we might actually need to upscale using a different surface.
I'd initially thought we could simply catch this for the RasterImage::Draw() codepath by comparing the 'aSize' argument against "mSize" (the image's intrinsic size), but that doesn't work. In my upscaling reftest on bug 856337, we actually do the upscaling via a transform stored on the gfxContext, *not* via the 'aSize' parameter. (Specifically, the transform is applied nsLayoutUtils.cpp's DrawImageInternal() method, which calls ctx->SetMatrix() right before calling Draw.) We end up doing the following, under the hood: (1) Drawing the image to a temporary surface. (2) Drawing that surface to a DrawTarget from the passed-in GraphicsContext. Both (1) and (2) use the passed-in GraphicsFilter (which is e.g. ::PIXELATED or ::GOOD or ::NEAREST_NEIGHBOR). In my case, the surface in (1) happens to be exactly at the image's intrinsic size -- so no scaling is actually done. I'm not yet sure where the correct place to do the conversion is... (Particularly given that we're potentially scaling twice here.) We could pass PIXELATED all the way down and evaluate it before each of the steps above, but that might produce bad results, because we could in theory be doing an upscale in (1) and a downscale in (2), or vice versa. (I don't know if we have code-paths that would actually do that, but it seems like it could happen in theory.)
(In reply to Daniel Holbert [:dholbert] from comment #2) > We end up doing the following, under the hood: > (1) Drawing the image to a temporary surface. > (2) Drawing that surface to a DrawTarget from the passed-in GraphicsContext. > > Both (1) and (2) use the passed-in GraphicsFilter (which is e.g. ::PIXELATED > or ::GOOD or ::NEAREST_NEIGHBOR). In my case, the surface in (1) happens to > be exactly at the image's intrinsic size -- so no scaling is actually done. (Its probably obvious, but just in case: this leaves all the scaling to be actually done in part (2), in the testcase/codepath I'm stepping through. It happens inside of gfxSurfaceDrawable::DrawInternal()'s call to FillRect() -- the DrawTarget has the same scale-up transform that we stored on the gfxContext. So -- I could address just this testcase by adding some logic in gfxSurfaceDrawable::DrawInternal(), but as noted at the end of comment 2, that might do the wrong thing if we've already done some scaling before that point.)
The interesting stuff here will mostly be in the next patch (handling pixelated in /gfx), but here are the /layout & /image parts. In particular, this patch: (1) Adds a new GraphicsFilter enum for 'pixelated', and returns that from nsLayoutUtils::GetGraphicsFilterForFrame. (2) In RasterImage: Treats "pixelated" as "good" for the purposes of determining whether we can use HQ downscaling. (unless it looks like we're upscaling in at least one dimension.) (3) In VectorImage: Treats "pixelated" as "good", because the SVG document can scale crisply (no need for pixelation), and any images inside of it should be controlled by the internal SVG document's CSS. (I can imagine us revisiting how (3) should work at some point, and perhaps letting the caller's 'image-rendering' transfer through to images inside of an SVG document; but for now I think it's reasonable to let the internal document's CSS control that.) The actual "upscaling --> NN, downscaling --> GOOD" logic will come in the next patch, in /gfx code.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8499016 - Flags: review?(seth)
Attached patch part 2: handle FILTER_PIXELATED in gfx code (obsolete) (deleted) — — Splinter Review
This is a WIP patch to actually resolve FILTER_PIXELATED, at the point where we convert it to a different enum type (GraphicsFilter --> gfx::Filter), based on whether we're upscaling or downscaling. Notes: - ToFilter() is the conversion function that does a mapping from GraphicsFilter to gfx::Filter. I'd rather not add an enum to gfx::Filter if I don't have to, so I'm trying to resolve "pixelated" right before that conversion. (And if we haven't resolved it yet, then ToFilter asserts & then treats pixelated as nearest-neighbor.) - I've added gfxUtils::ResolveFilter(), which (based on a passed-in DrawTarget's transform) converts PIXELATED to either GOOD or NEAREST. I use that in BasicCanvasLayer, BasicImageLayer, and gfxSurfaceDrawable. It looks like other layers code that does filter-conversion doesn't necessarily have a draw target, though, so I'm not sure I can use this helper-function everywhere.
Attachment #8499061 - Flags: feedback?
Attachment #8499061 - Attachment description: part 2: handle PIXELATED in gfx code → part 2: handle FILTER_PIXELATED in gfx code
Attachment #8499061 - Flags: feedback? → feedback?(seth)
Comment on attachment 8499061 [details] [diff] [review] part 2: handle FILTER_PIXELATED in gfx code Review of attachment 8499061 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicImageLayer.cpp @@ +96,2 @@ > DrawOptions(GetEffectiveOpacity(), GetEffectiveOperator(this)), > aMaskLayer); I think this is an example of a place where it'd be simpler if we had Filter::PIXELATED. This would really be cleaner to handle inside FillRectWithMask, since it looks to me like the mask layer has a transform you need to take into account. But I honestly think it'd *really* be easiest to handle inside the Moz2D API - in other words, the best place to handle this is inside DrawTarget::MaskSurface and similar methods. ::: gfx/layers/d3d9/CanvasLayerD3D9.cpp @@ +170,3 @@ > if (mFilter == GraphicsFilter::FILTER_NEAREST) { > device()->SetSamplerState(0, D3DSAMP_MAGFILTER, D3DTEXF_LINEAR); > device()->SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR); Can't we implement FILTER_PIXELATED like this? device()->SetSamplerState(0, D3DSAMP_MAGFILTER, D3DTEXF_POINT); device()->SetSamplerState(0, D3DSAMP_MINFILTER, D3DTEXF_LINEAR); The sames for all the other D3D9 stuff. For D3D10, just based on what I'm seeing here in splinter, it looks to me like you might need to change the shader code. ::: gfx/thebes/gfx2DGlue.h @@ +98,5 @@ > + // gives us some freedom on how to downscale with "pixelated". But we'd > + // *really* like to use FILTER_GOOD for downscaling; hence, we spam an > + // assertion-failure to help us catch such cases so we can fix the caller. > + NS_ERROR("Caller should've resolved FILTER_PIXELATED to NEAREST or GOOD, " > + "depending on whether we're upscaling"); I dunno... I think we'd be better off just adding Filter::PIXELATED. How much more complex would it be? ::: gfx/thebes/gfxDrawable.cpp @@ +87,4 @@ > Rect fillRect = ToRect(aFillRect); > DrawTarget* dt = aContext->GetDrawTarget(); > > + Filter filter = ToFilter(gfxUtils::ResolveFilter(aFilter, dt)); Doesn't the pattern transform also matter here? ::: gfx/thebes/gfxUtils.cpp @@ +1346,5 @@ > + return aFilter; > + } > + > + // Check if our DrawTarget's transform is an upscale in at least one > + // dimension, by seeing what it does to a unit square. Isn't it enough to check the matrix components?
(In reply to Seth Fowler [:seth] from comment #6) > Comment on attachment 8499061 [details] [diff] [review] > part 2: handle FILTER_PIXELATED in gfx code > > Review of attachment 8499061 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/basic/BasicImageLayer.cpp > @@ +96,2 @@ > > DrawOptions(GetEffectiveOpacity(), GetEffectiveOperator(this)), > > aMaskLayer); > > I think this is an example of a place where it'd be simpler if we had > Filter::PIXELATED. This would really be cleaner to handle inside > FillRectWithMask, since it looks to me like the mask layer has a transform > you need to take into account. But I honestly think it'd *really* be easiest > to handle inside the Moz2D API - in other words, the best place to handle > this is inside DrawTarget::MaskSurface and similar methods. Hmm, you may be right. I was trying to avoid polluting too much code with this filter-type -- it seems conceptually nice to resolve it to the *actual* filter-type as early as possible. But maybe we should pass it through further... > Can't we implement FILTER_PIXELATED like this? Maybe! I don't know this D3D code at all, really (and I haven't tested on Windows yet). I'll give this a shot, though. > For D3D10, just based on what I'm seeing here in splinter, it looks to me > like you might need to change the shader code. Boo. > ::: gfx/thebes/gfx2DGlue.h > I dunno... I think we'd be better off just adding Filter::PIXELATED. How > much more complex would it be? I'm not sure yet, but I'll take a look & get back to you. > ::: gfx/thebes/gfxDrawable.cpp > @@ +87,4 @@ > > Rect fillRect = ToRect(aFillRect); > > DrawTarget* dt = aContext->GetDrawTarget(); > > > > + Filter filter = ToFilter(gfxUtils::ResolveFilter(aFilter, dt)); > > Doesn't the pattern transform also matter here? Hmm, probably. > > + // Check if our DrawTarget's transform is an upscale in at least one > > + // dimension, by seeing what it does to a unit square. > > Isn't it enough to check the matrix components? Well, there might be a rotation, so I don't think we can just pull out the 1,1 and 2,2 components and check if they're greater than 1. We could do a fancier inspection of other components as well, but I think it boils down to being equivalent to what I've got (scaling a unit square & measuring the size), right? My matrix math is a little rusty, so I'm willing to believe that there's something better that I'm missing.
(In reply to Daniel Holbert [:dholbert] from comment #7) > Well, there might be a rotation, so I don't think we can just pull out the > 1,1 and 2,2 components and check if they're greater than 1. We could do a > fancier inspection of other components as well, but I think it boils down to > being equivalent to what I've got (scaling a unit square & measuring the > size), right? My matrix math is a little rusty, so I'm willing to believe > that there's something better that I'm missing. Yeah, that's right - if you need to worry about rotations here, you can't do much better than scaling a unit square. If you were using a gfxMatrix I'd suggest gfxMatrix::ScaleFactors in that case, but unfortunately Moz2D's Matrix class doesn't seem to have it! So I withdraw my comment: what you have right now seems good to me.
Comment on attachment 8499016 [details] [diff] [review] part 1: Add GraphicsFilter enum, plus /layout & /image code Review of attachment 8499016 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, though I suppose if we push FILTER_PIXELATED deeper into the graphics system we may not need some parts of this patch. (In particular, maybe we don't need to touch VectorImage at all.) ::: gfx/thebes/GraphicsFilter.h @@ +16,5 @@ > FILTER_BILINEAR, > FILTER_GAUSSIAN, > + FILTER_SENTINEL, > + FILTER_PIXELATED // PIXELATED isn't its own scaling algorithm. It means we > + // should use NEAREST if upscaling, GOOD if downscaling. Surely FILTER_SENTINEL should be the last one?
Attachment #8499016 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #9) > Looks good to me, though I suppose if we push FILTER_PIXELATED deeper into > the graphics system we may not need some parts of this patch. (In > particular, maybe we don't need to touch VectorImage at all.) Good point; I'll try getting rid of the VectorImage chunk, as it's probably not needed; it should Just Work, I'd think. > ::: gfx/thebes/GraphicsFilter.h > Surely FILTER_SENTINEL should be the last one? Yup, already noticed & fixed that locally. Thanks!
Here's part 1, with: - ...the VectorImage chunk removed [it's indeed unnecessary, and I've got a test to prove that included in bug 856337's baseline "pixelated" reftests now] - ...the enum ordering swapped so FILTER_SENTINEL stays at the end (and with a shortened 1-liner comment next to the enum value) Carrying forward r+
Attachment #8499016 - Attachment is obsolete: true
Attachment #8502062 - Flags: review+
Here's an updated version of part 2, which pushes FILTER_PIXELATED one layer deeper (into gfx::Filter in gfx/2d/Types.h), and handles it in Cairo, which fixes this bug on my Linux system, AFAICT. (This still needs code for other layers backends.)
Attachment #8499061 - Attachment is obsolete: true
Attachment #8499061 - Flags: feedback?(seth)
Comment on attachment 8502069 [details] [diff] [review] part 2 v2: handle "pixelated" in DrawTargetCairo.cpp (er, never mind about "linux layers code" -- DrawTargetCairo seems to be all we need here. I'd mistakenly remembered there being another layers chunk that was in here.)
Attachment #8502069 - Attachment description: part 2 v2: handle "pixelated" in DrawTargetCairo.cpp & linux layers code → part 2 v2: handle "pixelated" in DrawTargetCairo.cpp
Attached patch reftest patch (deleted) — — Splinter Review
This patch includes two reftests for this bug, which are just modified copies of reftest files from bug 856337, but with downscaling instead of upscaling. (Files are created using 'hg cp' to show their ancestry; hopefully that doesn't make the bugzilla diff view too gross.) The tests render a 32x32 image (with a grid of 4 colors) in a 7x7 onscreen region, so that we're forced to merge different-colored pixels along the center lines. This reveals whether we're doing nearest-neighbor or GOOD. (Nearest-neighbor will end up picking a single source color for the merged pixel; GOOD will do some color-averaging.) The reference case is the same as the testcase, but without "image-rendering:pixelated" (so, it uses our default downscaling algorithm). I've included a "notref" case that's also the same as the testcase, except with with "image-rendering:-moz-crisp-edges", to prove that we can actually distinguish a nearest-neighbor downscaling result from a FILTER_GOOD downscaling result. Also: I picked a fairly-extreme downscale (> 4x) to make it more likely that we're *really* having to downscale, even on systems with multiple device pixels per CSS pixel.
Attachment #8502086 - Flags: review?(seth)
(So, the three image-rendering-pixelated-downscale-1* files are all identical, aside from having testcase-flavored headers/comments vs. reference-case-flavored headers/comments, and aside from the "image-rendering" value used. Same goes for the three image-rendering-pixelated-downscale-svg-1* files.)
Try run w/ current patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=68bf6c019701 (Expecting reftest failures on Mac & Windows, for now.)
Blocks: 1081224
Looks like we'll need fixes in all the various gfx/2d/DrawTarget{$WHATEVER}.cpp files, which is requires a bit more per-platform coding & testing. My initial motivation for working on "image-rendering:pixelated" was to get a quick path towards being able to submit interoperable reftests to the w3c for "object-fit" & "object-position", over on bug 624647. Given that this didn't turn out to be as quick as I was hoping, I'm de-prioritizing this for now, and I'm focusing on getting bug 624647 finished, with not-yet-interoperable reftests. (using "-moz-crisp-edges", which is all I really need but isn't suitable for tests that we submit to a shared testsuite. Once this bug & bug 856337 are fixed, we can swap out "-moz-crisp-edges" for "pixelated" in the reftests & move them to our w3c submission directory.)
Comment on attachment 8502086 [details] [diff] [review] reftest patch Review of attachment 8502086 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8502086 - Flags: review?(seth) → review+
Unassigning, since I'm not actively working on this for a while now (and this isn't a priority). (Comment 17 describes what's needed here -- the various platform-specific DrawTarget{WHATEVER}.cpp files need checks to detect if we're downscaling, & pick our scaling algorithm based on that.)
Assignee: dholbert → nobody
Status: ASSIGNED → NEW

Note: per https://github.com/w3c/csswg-drafts/issues/5837#issuecomment-785259691 , it looks like the pixelated spec text is being extended so that the "smarter downscaling algorithm" will be applied at any non-exact pixel value (not just downscales) -- e.g. for 2.5x upscale, now the spec will call for doing something like nearest-neighbor to 2.0x or 3.0x, followed by a smarter scaling algorithm to reach the 2.5x target.

Daniel, is this still relevant with WR fully deployed? Emilio and I were talking through this relative to FFXP-604. Thoughts, feedback?

Flags: needinfo?(dholbert)
Attached file testcase 1 (deleted) —

I've just posted a testcase[1] to demonstrate the issue that we would hypothetically be solving here. Essentially: our image-scaling:pixelated implementation looks considerably-worse than the default scaling algorithm when we downscale. With the image I'm using, it introduces breaks in the black diagonal "slash"; and in the second-to-last line, the diagonal "slash" disappears entirely.

Having said that, I have a few observations which lead me to believe we can close this:
(A) Chromium and WebKit have the same or similar downscaling artifacts on this testcase, so this isn't a case where we've fallen behind them, at least.

(B) The CSSWG resolution that prompted me to file this bug never actually made it into the spec, unfortunately (as I mentioned in https://github.com/w3c/csswg-drafts/issues/5837#issuecomment-785619937 )

(C) The spec has evolved a bit so that now pixelated will have an "optional better-than-Nearest-Neighbors behavior" for other fractional scale-factors as well (not just < 1), as noted in comment 20.

All three of these factors lead me to believe we should just close this out. I'll file a new (not-high-priority) bug on optionally coming up with a better scaling behavior for fractional scaling factors, and I'll duplicate this bug forward to that new bug, as its spiritual successor.

[1] Brief overview of the testcase: it shows the same image, with default image-scaling on the left and image-scaling:pixelated on the right, at a variety of sizes:

  • the default size
  • scaled up by 4x (to 128px in size), just to double-check that pixelated is doing something.
  • scaled down to a variety of widths
Flags: needinfo?(dholbert)

(In reply to Daniel Holbert [:dholbert] from comment #25)

All three of these factors lead me to believe we should just close this out. I'll file a new (not-high-priority) bug on optionally coming up with a better scaling behavior for fractional scaling factors, and I'll duplicate this bug forward to that new bug, as its spiritual successor.

--> Duping forward to bug 1731134

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: