Bug 93471 - -webkit-filter prevents rendering at retina scale
: -webkit-filter prevents rendering at retina scale
Status: ASSIGNED
: WebKit
Images
: 528+ (Nightly build)
: Macintosh Mac OS X 10.8
: P2 Major
Assigned To:
:
: InRadar
:
: 68469 119300 126398
  Show dependency treegraph
 
Reported: 2012-08-08 06:01 PST by
Modified: 2014-04-11 11:54 PST (History)


Attachments
Patch (36.00 KB, patch)
2013-07-30 19:02 PST, Dean Jackson
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (838.62 KB, application/zip)
2013-07-31 02:48 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.36 MB, application/zip)
2013-07-31 05:41 PST, Build Bot
no flags Details
Patch (35.88 KB, patch)
2013-08-07 20:05 PST, Dean Jackson
dino: review?
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (730.16 KB, application/zip)
2013-08-08 06:04 PST, Build Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-08 06:01:01 PST
If -webkit-filter is applied to an element and that element contains an image/background-image
that picture will not render at the correct scale on HiDPI displays.

I tested it on a MacBook Pro with Retina Display and the filters output is @1x instead of @2x
using a css selector in conjunction with a min-device-pixel-ratio/min-resolution @media query.

If no filter is applied the image renders correctly @2x
------- Comment #1 From 2012-08-08 10:36:27 PST -------
<rdar://problem/12057066>
------- Comment #2 From 2012-11-13 02:37:44 PST -------
+1 reproducible: http://stackoverflow.com/questions/13347947/css-filter-on-retina-display-fuzzy-images
------- Comment #3 From 2013-01-07 14:20:22 PST -------
*** Bug 106241 has been marked as a duplicate of this bug. ***
------- Comment #4 From 2013-01-07 14:31:26 PST -------
Per the duplicate, this happens even when there are now images at all.
------- Comment #5 From 2013-01-07 14:35:54 PST -------
s/now/no/.
------- Comment #6 From 2013-01-08 09:40:00 PST -------
*** Bug 106331 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2013-01-29 09:25:59 PST -------
This is similar to the issue I fixed for -webkit-canvas. I suspect filters are using ImageBuffer::copyImage and need to pass the Unscaled parameter. See: https://bugs.webkit.org/show_bug.cgi?id=100611

Maybe Unscaled should be the default to prevent this in the future. And the limited clients that need Scaled should pass it instead.
------- Comment #8 From 2013-03-11 18:44:38 PST -------
I don’t think it’s copy that’s the issue. The following functions allocate buffers, always with a hardcoded device scale factor of 1:

BitmapTexture::updateContents
BitmapTextureImageBuffer::didReset
CanvasRenderingContext2D::createCompositingBuffer
CrossfadeGeneratedImage::drawPattern
FilterEffect::asImageBuffer
FilterEffect::createImageBufferResult
FilterEffect::openCLImageToImageBuffer
FilterEffectRenderer::allocateBackingStoreIfNeeded
ImageBuffer::putByteArray
SVGImage::drawPatternForContainer
SVGRenderingContext::createImageBuffer
SVGRenderingContext::createImageBufferForPattern
ScratchBuffer::getScratchBuffer
WebGLRenderingContext::LRUImageBufferCache::imageBuffer

At least some of these functions need to pass the device scale factor, not a factor of 1.
------- Comment #9 From 2013-03-20 13:04:12 PST -------
Marking this as duplicate of bug 112717. This bug has more information what is going on and how to fix this issue.

*** This bug has been marked as a duplicate of bug 112717 ***
------- Comment #10 From 2013-03-20 13:32:28 PST -------
Unduping, since this is a different issue.
------- Comment #11 From 2013-07-30 19:02:45 PST -------
Created an attachment (id=207796) [details]
Patch
------- Comment #12 From 2013-07-30 19:09:37 PST -------
This patch fixes all the shorthand filters, as well as combinations of shorthands. It also allows "referenced" filters (SVG-style filters accessed via url()) to operate at retina scale, but with some limitations:

- <feDropShadow> <feGaussianBlur> <feColorMatrix> <feComponentTransfer> should all work, since this patch updates their behaviour (possibly indirectly through ImageBuffer or FilterEffect).
- Other operations might revert back to 1x or produce incorrect results (say they are iterating over pixels but do not expect to see 4x as many).

I've filed https://bugs.webkit.org/show_bug.cgi?id=119300 to cover cleanup.
------- Comment #13 From 2013-07-30 20:13:59 PST -------
(From update of attachment 207796 [details])
Attachment 207796 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1289616
------- Comment #14 From 2013-07-31 02:47:59 PST -------
(From update of attachment 207796 [details])
Attachment 207796 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1273018

New failing tests:
fast/hidpi/filters-shadow.html
------- Comment #15 From 2013-07-31 02:48:04 PST -------
Created an attachment (id=207822) [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
------- Comment #16 From 2013-07-31 05:41:03 PST -------
(From update of attachment 207796 [details])
Attachment 207796 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1276013

New failing tests:
fast/hidpi/filters-invert.html
fast/hidpi/filters-blur.html
fast/hidpi/filters-shadow.html
fast/hidpi/filters-hue-rotate.html
------- Comment #17 From 2013-07-31 05:41:08 PST -------
Created an attachment (id=207842) [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
------- Comment #18 From 2013-07-31 06:38:28 PST -------
(In reply to comment #13)
> (From update of attachment 207796 [details] [details])
> Attachment 207796 [details] [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/1289616

Internal compiler error!?
------- Comment #19 From 2013-07-31 06:39:06 PST -------
(In reply to comment #16)
> (From update of attachment 207796 [details] [details])
> Attachment 207796 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/1276013
> 
> New failing tests:
> fast/hidpi/filters-invert.html
> fast/hidpi/filters-blur.html
> fast/hidpi/filters-shadow.html
> fast/hidpi/filters-hue-rotate.html

Not sure why these are failing since they are the tests I added for the patch.
------- Comment #20 From 2013-08-07 20:05:39 PST -------
Created an attachment (id=208310) [details]
Patch
------- Comment #21 From 2013-08-07 20:06:26 PST -------
Retrying.
------- Comment #22 From 2013-08-08 01:34:43 PST -------
(From update of attachment 208310 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298
>      calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY);
> +    kernelSizeX *= filter()->filterScale();
> +    kernelSizeY *= filter()->filterScale();

Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)?

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301
> +    paintSize.scale(filter()->filterScale());

There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there.
------- Comment #23 From 2013-08-08 01:55:02 PST -------
(In reply to comment #22)
> (From update of attachment 208310 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review
> 
> > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298
> >      calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY);
> > +    kernelSizeX *= filter()->filterScale();
> > +    kernelSizeY *= filter()->filterScale();
> 
> Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)?
> 
> > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301
> > +    paintSize.scale(filter()->filterScale());
> 
> There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there.

Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.
------- Comment #24 From 2013-08-08 02:35:10 PST -------
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 208310 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review
> > 
> > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298
> > >      calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY);
> > > +    kernelSizeX *= filter()->filterScale();
> > > +    kernelSizeY *= filter()->filterScale();
> > 
> > Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)?
> > 
> > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301
> > > +    paintSize.scale(filter()->filterScale());
> > 
> > There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there.
> 
> Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.

It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution.

That said, I might be able to change applyH/VScale - I'll check. IIRC I tried to do this first and there were a lot of side effects.
------- Comment #25 From 2013-08-08 06:03:55 PST -------
(From update of attachment 208310 [details])
Attachment 208310 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1365229

New failing tests:
fast/hidpi/filters-blur.html
------- Comment #26 From 2013-08-08 06:04:00 PST -------
Created an attachment (id=208336) [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
------- Comment #27 From 2013-08-08 06:08:16 PST -------
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (From update of attachment 208310 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=208310&action=review
> > > 
> > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:298
> > > >      calculateKernelSize(filter(), kernelSizeX, kernelSizeY, m_stdX, m_stdY);
> > > > +    kernelSizeX *= filter()->filterScale();
> > > > +    kernelSizeY *= filter()->filterScale();
> > > 
> > > Shouldn't that be done in calculateKernelSize (which asks Filter or SVGFilter)?
> > > 
> > > > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:301
> > > > +    paintSize.scale(filter()->filterScale());
> > > 
> > > There is an extra calculation step before entering platformApplySoftware() that calculates absolutePaintRect. The code should go there.
> > 
> > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.
> 
> It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution.

I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place.

> 
> That said, I might be able to change applyH/VScale - I'll check. IIRC I tried to do this first and there were a lot of side effects.

Please comment in which problems you run.
------- Comment #28 From 2013-08-08 14:04:04 PST -------
> > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.
> > 
> > It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution.
> 
> I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place.

Here's an example of what I mean: drop shadow uses applyH/VScale for both calculating the kernel size and offsetting the shadow. If we apply the filterScale (or resolution if you call it that), one or the other will break because they use different units. The kernel is applying to pixels, the offset is CSS.

Another is displacement map. It (indirectly) uses applyH/VScale for color scaling as well as distance. I haven't verified that this will produce incorrect results yet.

Maybe I'm misunderstanding the intent of things like absolutePaintRect - is it supposed to be scaled or not? You're suggesting I should do the scale before platformApplySoftware. I guess this means you think that as far as each individual filter is concerned it is always working on actual pixels, and thus applyH/VScale should be simply converting filter units into pixels. Is this right?
------- Comment #29 From 2013-08-08 14:48:35 PST -------
The layout test failures are strange. It looks like maybe i need to force a reload after changing the page scale.
------- Comment #30 From 2013-08-08 15:09:50 PST -------
(In reply to comment #28)
> > > > Looking at the code, I think applyHorizontalScale() and VerticalScale() in Filter.h should do the adaption to retina. Just take you retina scale level into account when calculating the value. IIRC, the kernel and the image size should be scaled up automatically. Should actually be a straight forward patch.
> > > 
> > > It's not, unfortunately. There seems to be little consistency throughout the entire filter code as to the way scaling and resolution are used. I'm not looking forward to the more complicated filters like convolution.
> > 
> > I am not talking about convolution. I think we need to revisit the implementation of convolution anyway. A lot of people worked on the code and it looks a lot messier than it used to be. I implemened applyH/VScale exactly for the use case of scaling the filter resolution. If that doesn't work, then this code is broken and needs to be fixe. Please don't make the code even more messier by doing scaling in a lot of different places, when it just should be done in one place.
> 
> Here's an example of what I mean: drop shadow uses applyH/VScale for both calculating the kernel size and offsetting the shadow. If we apply the filterScale (or resolution if you call it that), one or the other will break because they use different units. The kernel is applying to pixels, the offset is CSS.
> 
> Another is displacement map. It (indirectly) uses applyH/VScale for color scaling as well as distance. I haven't verified that this will produce incorrect results yet.
> 
> Maybe I'm misunderstanding the intent of things like absolutePaintRect - is it supposed to be scaled or not? You're suggesting I should do the scale before platformApplySoftware. I guess this means you think that as far as each individual filter is concerned it is always working on actual pixels, and thus applyH/VScale should be simply converting filter units into pixels. Is this right?

Yes, that was the intention. absoluteRect actually defines the size in screen coordinates - the real size of your pixel buffer. In the past screen coordinates have been in CSS pixel. It worked quite well, but with HiDPI displays this is no longer true. absoluteRect needs to take the devicePixel to CSS pixel ratio into account as well. I should mention the reasoning behind it

1) with SVG Filters users actually can specify  the resolution of the filtered result to speed it up during animations for instance by reducing the pixel size.
2) SVG demands pixel perfect results under all circumstances. Means if you e.g. apply a scale(10), then a blurred element should not look... well ... more blurry then expected. So we transform the filtered object into the screen coordinate space and do the filter operation there. Then we draw the result with inverse transform back on the canvas.

I need to think about your use case with drop shadow. As you describe it, it actually does the right thing, doesn't it? Both, the radius as well as the offset are defined in CSS pixels. (For HTML we do not try to map local coordinates to screen coordinates sadly, which makes results more blurry on CSS transformations.) So when it produces the wrong results, then this seems to be the result of giving the wrong input to the filter.
------- Comment #31 From 2013-08-08 16:06:35 PST -------
Yeah, I was aware of the filter resolution attribute.

I'll see if I can get the patch to simply update your scaling methods. I realised that my changelog comment about breaking svg filters is incorrect since I only set the filterScale (which I should rename to filterDevicePixelRatio or something) on the shorthand filters.
------- Comment #32 From 2013-12-10 00:31:56 PST -------
Hi. Any update on this? It is still broken.
------- Comment #33 From 2014-03-02 09:30:59 PST -------
Dean, there is a high demand that the fix is going to be in Safari next. Could you please take a look at this again?