Closed Bug 1003505 Opened 6 years ago Closed 6 years ago

Printed SVG-as-an-image is fuzzy

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox32 --- affected
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: dholbert, Assigned: jwatt)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

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.)
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)
-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
QA Whiteboard: [bugday-20140526]
Thanks. In there, Bug 764299 seems like the most likely candidate to have regressed this. Tentatively marking as regression from that bug.
OS: Linux → All
Hardware: x86_64 → All
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → jwatt
Attachment #8442822 - Flags: review?(seth)
Depends on: 1027645
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. =)
Attachment #8442822 - Flags: review?(seth) → review-
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>
(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.
Attached patch patchSplinter Review
Attachment #8442822 - Attachment is obsolete: true
Attachment #8445820 - Flags: review?(seth)
Comment on attachment 8445820 [details] [diff] [review]
patch

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

OK, sounds good. Thanks Jonathan!
Attachment #8445820 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/814bda3bac4e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Duplicate of this bug: 1057477
QA Whiteboard: [bugday-20140526] → [good first verify]
[Tracking Requested - why for this release]: Regression would be good to uplift as far as JWatt is comfortable with.
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]
Status: RESOLVED → VERIFIED
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).
Pierre, please file a new bug rather than commenting on an old fixed one.
You need to log in before you can comment on or make changes to this bug.