-webkit-filter prevents rendering at retina scale

• Component: Images
• Status: RESOLVED FIXED

[Erik Aigner]

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

[Radar WebKit Bug Importer]

<rdar://problem/12057066>

[Stefan Rechsteiner]

+1 reproducible: http://stackoverflow.com/questions/13347947/css-filter-on-retina-display-fuzzy-images

[Tim Horton]

*** Bug 106241 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Per the duplicate, this happens even when there are now images at all.

[Alexey Proskuryakov]

s/now/no/.

[Nico Weber]

*** Bug 106331 has been marked as a duplicate of this bug. ***

[Timothy Hatcher]

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.

[Darin Adler]

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.

[Dirk Schulze]

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 ***

[Stephen White]

Unduping, since this is a different issue.

[Dean Jackson]

Created attachment 207796 [details]
Patch

[Dean Jackson]

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.

[EFL EWS Bot]

Comment on attachment 207796 [details]
Patch

Attachment 207796 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1289616

[Build Bot]

Comment on attachment 207796 [details]
Patch

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

[Build Bot]

Created attachment 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

[Build Bot]

Comment on attachment 207796 [details]
Patch

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

[Build Bot]

Created attachment 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

[Dean Jackson]

(In reply to comment #13)
> (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

Internal compiler error!?

[Dean Jackson]

(In reply to comment #16)
> (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

Not sure why these are failing since they are the tests I added for the patch.

[Dean Jackson]

Created attachment 208310 [details]
Patch

[Dean Jackson]

Retrying.

[Dirk Schulze]

Comment on attachment 208310 [details]
Patch

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.

[Dirk Schulze]

(In reply to comment #22)
> (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.

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.

[Dean Jackson]

(In reply to comment #23)
> (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.

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.

[Build Bot]

Comment on attachment 208310 [details]
Patch

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

[Build Bot]

Created attachment 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

[Dirk Schulze]

(In reply to comment #24)
> (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.

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.

[Dean Jackson]

> > > 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?

[Dean Jackson]

The layout test failures are strange. It looks like maybe i need to force a reload after changing the page scale.

[Dirk Schulze]

(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.

[Dean Jackson]

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.

[Tomasz Mikolajewski]

Hi. Any update on this? It is still broken.

[Dirk Schulze]

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?

[Dirk Schulze]

Created attachment 231168 [details]
Patch from dino - rebaselined

[WebKit Commit Bot]

Attachment 231168 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366:  Declaration has space between type name and * in unsigned char *destinationPixel  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367:  Declaration has space between type name and * in unsigned char *sourcePixel  [whitespace/declaration] [3]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 231168 [details]
Patch from dino - rebaselined 

Attachment 231168 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5451066409746432

New failing tests:
svg/text/non-bmp-positioning-lists.svg
fast/hidpi/filters-invert.html
fast/hidpi/filters-blur.html
fast/hidpi/filters-shadow.html
fast/hidpi/filters-hue-rotate.html
fast/hidpi/filters-reference.html
fast/hidpi/filters-multiple.html

[Build Bot]

Created attachment 231176 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dirk Schulze]

Created attachment 231179 [details]
Patch from dino - rebaselined

[WebKit Commit Bot]

Attachment 231179 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366:  Declaration has space between type name and * in unsigned char *destinationPixel  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367:  Declaration has space between type name and * in unsigned char *sourcePixel  [whitespace/declaration] [3]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 231179 [details]
Patch from dino - rebaselined 

Attachment 231179 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6090052316692480

New failing tests:
fast/hidpi/filters-blur.html
fast/hidpi/filters-reference.html
fast/hidpi/filters-hue-rotate.html
fast/hidpi/filters-invert.html
fast/hidpi/filters-multiple.html

[Build Bot]

Created attachment 231186 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dean Jackson]

I think the wk2 image failures are due to subpixel. Check the difference images - it's the antialiasing boundary of the text element.

I'm going to send another patch for review, with those tests marked as image diffs on wk2. Then file a bug on the subpixel errors.

[Dean Jackson]

Created attachment 231207 [details]
Patch

[Dean Jackson]

Time for Dirk to review... again :)

[WebKit Commit Bot]

Attachment 231207 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:366:  Declaration has space between type name and * in unsigned char *destinationPixel  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/filters/FilterEffect.cpp:367:  Declaration has space between type name and * in unsigned char *sourcePixel  [whitespace/declaration] [3]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

WK2 error bugs are 132766

[Dean Jackson]

Committed r168577: <http://trac.webkit.org/changeset/168577>