1003505 - Printed SVG-as-an-image is fuzzy

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load attachment 479223 [details]
 2. Print (to file or paper)
 3. Compare the two Microsoft Office logos.

ACTUAL RESULTS: The second logo (under <img> is fuzzy)

EXPECTED RESULTS: Should be as crisp as the top logo.


Bug 600207 used to track a broader bug that included this symptom. This was fixed (by bug 600207) in Firefox 24, 25, and 26. Then, this symptom cropped up again in Firefox 27, and it's there in current Nightly as well.

(Rather than reopen bug 600207, I'm filing a new bug to track this, since it's already unmanageably huge, and its non-printing aspects do still seem to be fixed, and this is probably a separate [newly-introduced] underlying issue even though it triggers one of the same symptoms.)

Comment 1

[Daniel Holbert [:dholbert]]

Firefox versions mentioned in Comment 0 are 64-bit Firefox releases for Linux, FWIW.

My current nightly (which exhibits this):
Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
32.0a1 (2014-04-29)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: PDF printout (generated with Nightly)

Comment 3

[[:Aleksej]]

-g 2013-10-18-03-02-06-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
-g 2013-10-22-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 177bf37a49f5
-b 2013-10-23-03-02-05-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 21d97baadc05
-b 2013-10-24-03-02-04-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
-b 2014-04-29-03-02-01-mozilla-central-firefox-32.0a1.en-US.linux-x86_64
-b 2014-05-22-03-02-04-mozilla-central-firefox-32.0a1.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=177bf37a49f5&tochange=21d97baadc05

Comment 4

[Daniel Holbert [:dholbert]]

Thanks. In there, Bug 764299 seems like the most likely candidate to have regressed this. Tentatively marking as regression from that bug.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

When we print we use a gfxSurfaceType::PDF backed DrawTargetCairo. In this case we want all draw commands to go through to the DrawTargetCairo instance and get turned into vector PDF commands. What's happening instead is that we're using the surface cache and embedding a raster image into the PDF.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8442822 [details] [diff] [review]
patch

Review of attachment 8442822 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this, Jonathan!

I'm r-'ing but I approve the general idea. I mostly just want to see it again after you address my comment for nsLayoutUtils::DrawImageInternal, because I'm hoping you'll be able to move the check into VectorImage::Draw. If not, that's OK.

::: image/public/imgIContainer.idl
@@ +153,5 @@
>     * performance (by avoiding an upload to/readback from the GPU) when the
>     * caller knows they want a SourceSurface of type DATA.
>     */
>    const long FLAG_WANT_DATA_SURFACE = 0x20;
> +  const long FLAG_BYPASS_SURFACE_CACHE = 0x40;

Please add a comment describing this flag.

::: image/src/VectorImage.cpp
@@ +856,5 @@
>                                aSubimage, aViewportSize, aSVGContext, animTime, aFlags);
>  
>    // Check the cache.
> +  nsRefPtr<gfxDrawable> drawable;
> +  if (!(aFlags & FLAG_BYPASS_SURFACE_CACHE)) {

You don't need this. We couldn't possibly have a hit in the cache, since aFlags is part of the cache key, and we never write into the cache if FLAG_BYPASS_SURFACE_CACHE is set.

@@ +875,5 @@
>  }
>  
>  void
> +VectorImage::CreateDrawableAndShow(const SVGDrawingParameters& aParams,
> +                                   uint32_t aFlags)

You don't need to add the aFlags parameter here. |aParams.flags| contains exactly that value.

::: image/src/VectorImage.h
@@ +86,5 @@
>    virtual nsresult StopAnimation();
>    virtual bool     ShouldAnimate();
>  
> +  void CreateDrawableAndShow(const SVGDrawingParameters& aParams,
> +                             uint32_t aFlags);

Again, you don't need to add |aFlags| here.

::: layout/base/nsLayoutUtils.cpp
@@ +5026,5 @@
>                    const nsIntSize&       aImageSize,
>                    const SVGImageContext* aSVGContext,
>                    uint32_t               aImageFlags)
>  {
> +  if (aPresContext->Type() == nsPresContext::eContext_Print) {

I'm OK with this, but if it's easy to determine this information from the gfxContext it would be nice, because then we can move this check into imagelib (and ensure that callers can't screw it up).

I'd prefer that solution if it's reasonable.

If not, you'll need to update nsLayoutUtils::DrawPixelSnapped, as well. The fact that you didn't notice this means that we don't have test coverage for using -moz-element in border-image and printing it. Can't say I'm surprised. =)

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8442822 [details] [diff] [review]
patch

Review of attachment 8442822 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +5026,5 @@
>                    const nsIntSize&       aImageSize,
>                    const SVGImageContext* aSVGContext,
>                    uint32_t               aImageFlags)
>  {
> +  if (aPresContext->Type() == nsPresContext::eContext_Print) {

Whoops, sorry. Of course -moz-element doesn't use SVG images. The border-image uses the normal DrawImage* functions when it's not dealing with -moz-element, so you don't need to update nsLayoutUtils::DrawPixelSnapped.

<shakes head to dislodge cobwebs>

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Seth Fowler [:seth] from comment #6)
> > +  nsRefPtr<gfxDrawable> drawable;
> > +  if (!(aFlags & FLAG_BYPASS_SURFACE_CACHE)) {
> 
> You don't need this. We couldn't possibly have a hit in the cache, since
> aFlags is part of the cache key, and we never write into the cache if
> FLAG_BYPASS_SURFACE_CACHE is set.

It's just an optimization since it's really cheap to check. I've added a comment.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +5026,5 @@
> >                    const nsIntSize&       aImageSize,
> >                    const SVGImageContext* aSVGContext,
> >                    uint32_t               aImageFlags)
> >  {
> > +  if (aPresContext->Type() == nsPresContext::eContext_Print) {
> 
> I'm OK with this, but if it's easy to determine this information from the
> gfxContext it would be nice, because then we can move this check into
> imagelib (and ensure that callers can't screw it up).
> 
> I'd prefer that solution if it's reasonable.

I'm in the process of trying to make that possible when the gfxContext is wrapping a DrawTarget. Once that is done, and once thebes backed gfxContexts are dead (soon) I plan to have a follow-up to change things to use the DrawTarget rather than have this check. I'll file a bug for it.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8445820 [details] [diff] [review]
patch

Review of attachment 8445820 [details] [diff] [review]:
-----------------------------------------------------------------

OK, sounds good. Thanks Jonathan!

Comment 11

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/814bda3bac4e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/814bda3bac4e

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: Regression would be good to uplift as far as JWatt is comfortable with.

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Bah this is already in Beta.

Comment 16

[vasilicamihasca]

I was able to reproduce this bug on Nightly 32.0a1 (2014-04-29), using Windows 7 x64.

Verified fixed on Windows 7 x64, Mac OSX 10.9.5 and Ubuntu 14.04 x86 using Firefox 33.0 RC (20141007073543), Aurora 34.0a2 (2014-10-08) and Nightly 35.0a1 (2014-10-08).

This fix can be marked as verified.

[bugday-20141008]

Comment 17

[Pierre-Antoine Champin]

This might be considered as a different bug, but I got this problem again in FF 39 -- although with a specific condition : I'm using SVG filters (to drop shadows) in my SVG files. 

Anything subject to a filter is still badly rasterized when printed.
I put an example online: [http://champin.net/2015/ff-svg-filter-dpi/]

I understand that filters impose a rasterization, but the default resolution (DPI) of that rasterization when printing is clearly too small, and I didn't find any way to change it (neither in the print dialog nor in about:config).

Comment 18

[Robert Longson [:longsonr]]

Pierre, please file a new bug rather than commenting on an old fixed one.