489151 - Masking and clipping with objectBoundingBox and non-trivial transforms is broken

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Ryo Onodera]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8

When I open an html page which loads the svg file inside it by <object> or <embed> element, it correctly renders.
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-masking-intro-01-f.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-masking-intro-01-f.html

However, when I open the image directly (to be specific, context menu->This Frame->Show Only This Frame), these circles disappear.

When I open the svg file by other applications:
On Ubuntu 9.04
Inkscape 0.46: only the bottom circles are shown.
Batik 1.7(using Squiggle): renders almost correctly (the upper part of the bottom circles are slightly clipped).

On Windows XP SP3(this box may be outdated)
Firefox 3.0.4, Opera 9.62: renders almost correctly (the width of the left bottom circle is narrower than expected)(To my surprise, both render almost in the same way, including this out-of-spec behaviour)

Chrome 1.0.154.43, Safari 3.2: the top circles are correct but, the bottom ones became a single big rectangle!

I don't know why this svg file is rendered correctly on Windows. At first, I presumed this bug would be platform-independent. So, I haven't checked this bug on Windows extensively...
As for linux, This happened on Debian lenny-testing, recent several Ubuntu versions, which shipping Firefox 3.x regardless of i386 and x86_64 and video driver or video hardware...
This also happens on mozilla-central built on Ubuntu 9.04.

When opening the html page with Firefox on Linux and Firefox on Windows,
the left bottom circle's stroke-width is differently rendered (another bug?)
So, It seems that masking are done through different code path depending on platform. Then, this explains this bug is also affected by platforms.

I guess this is related to objectBoundingBox, which is used in this svg file.

Reproducible: Always

Steps to Reproduce:
1. Open the URL
2.
3.
Actual Results:  
No circles are shown

Expected Results:  
Four circles should be shown, which are scaled up proportionally to the reference raster image(http://www.w3.org/Graphics/SVG/Test/20061213/png/full-masking-intro-01-f.png).

Comment 1

[Ryo Onodera]

I noticed that while directly opening the svg file, resizing the window affects behaviour of the bug. When the window is small, Firefox DOES draw those circles. however, the bigger the window is, the more the four circles are clipped out as if there is clipping rectangle. Maybe, zooming isn't taken into account?
As long as I briefly check this resizing issue on Windows, Firefox proportionally renders the svg file as I resize the window.

Comment 2

[Ryo Onodera]

Attachment: An equivalent svg file not using objectBoundingBox.

I've removed the objectBoundingBox part, and instead added appropriate transformations computed by hand.
When I open this modified version of the problematic svg file, I don't encounter erroneous rendering.
I think Firefox should render the unmodified version as same as the modified one.

Comment 3

[Ryo Onodera]

Attachment: A patch

Somehow, I've managed to fix this bug.
This patch is fairly tiny.

Because this is the first time I fixed a bug, I don't know much about exactly how I should do next. To begin with, I created a patch with the following command.
$ hg diff > test_files/patch_v1.diff

While I'm familiarising myself with firefox's codebase, this bug was fixed. So, I'm not sure exactly why this bug was fixed.

Comment 4

[Jonathan Watt [:jwatt]]

Nice! The reason your patch fixes this is because the GetBBox method on frames relies on SetMatrixPropagation having been called in order to work.

I just took a look through the code and we should also be using nsSVGUtils::GetBBox in nsSVGPatternFrame::GetCallerGeometry and nsSVGSVGElement::GetBBox instead of doing the SetMatrixPropagation thing manually there. Can you submit a patch with those two locations fixed up too?

Comment 5

[Ryo Onodera]

Well, I found a regression or another bug. When I go to http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-masking-intro-01-f.html, with the patch applied, and I zoom the page in or out, circles aren't correctly rendered. This doesn't happen when not applying the patch.
As for zooming while viewing directly the SVG file, this always happens with or without it. Of cource, without it, transformation is broken in the first place, but apparently when I zoom firefox, the transformation becomes even worse. Because the svg file has attribute "viewBox", no matter how I zoom in or out, the size of these circles shouldn't change.

Thus, I think I should use other API or calculation which is also taking zooming into account.

I'm still studying how exactly the transformation matrix/coordinate system interacts between browser zooming, viewBox, viewing directly SVG, embedding SVG in HTML.

Comment 6

[Ryo Onodera]

Attachment: patch v2

I have questions regarding the API cleaning suggested by Comment #4:
1. In nsSVGPatternFrame::GetCallerGeometry, we do additional check at nsSVGPatternFrame.cpp:646 and if true, call getParent(), which doesn't exist in nsSVGUtils::GetBBox. Is changing this to nsSVGUtils::GetBBox really OK? (I think it is not.)
2. nsSVGGraphicElement::GetBBox at nsSVGGraphicElement.cpp:87 looks very similar to nsSVGSVGElement::GetBBox, should I change this part too? (I think I should)
3. Also, at nsSVGSwitchFrame.cpp:195 in nsSVGSwitchFrame::GetBBox, I found the GetBBox method called without surrounding it with SetMatrixPropagation. Is that intentional? (I think it is, maybe?)

I think some of them turns to be silly as I study the code by myself, but as of now, I'm wondering about them... If you have time, and want to spoil me, can you answer any of them? Or, is there documentation about gecko's code for SVG and gfx? I'm reading the source code using lxr, mercurial.

Assuming my guess for the questions is correct, I cleaned some parts, compiling passed, but I'm not familiar firefox's test suite. At least, it seems reftest is OK...

My current patch does do_QueryFrame twice at the caller and callee site, but I don't know how I should elegantly integrate the error handling, because SVGUtils::GetBBox fall backs to GetSVGBBoxForNonSVGFrame when the redundant 'if' failed in it.

Which should I use?
rect.swap(*_retval);
Or
*_retval = rect;
NS_ADDREF(*_retval);

Comment 7

[Ryo Onodera]

Attachment: patch v3

Another patch, apparently which fixes the regression mentioned by Comment #5. Again, this is just a poorly-educated guess. Needs explanation by someone.

In short, I guessed this removed part was originally added to fix the zooming regression bug(zooming an html with the svg file) introduced by not using SetMatrixPropagation. Sadly, that fix turns out to be another regression bug and now is fixed by me.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #6)
> I have questions regarding the API cleaning suggested by Comment #4:
> 1. In nsSVGPatternFrame::GetCallerGeometry, we do additional check at
> nsSVGPatternFrame.cpp:646 and if true, call getParent(), which doesn't exist in
> nsSVGUtils::GetBBox. Is changing this to nsSVGUtils::GetBBox really OK? (I
> think it is not.)

Jonathan and I think it is OK. You need to call the frame version of nsSVGUtils::GetBBox and pass the correct callerSVGFrame so the GetParent logic will stay in nsSVGPatternFrame.

> 2. nsSVGGraphicElement::GetBBox at nsSVGGraphicElement.cpp:87 looks very
> similar to nsSVGSVGElement::GetBBox, should I change this part too? (I think I
> should)

Yes.

> 3. Also, at nsSVGSwitchFrame.cpp:195 in nsSVGSwitchFrame::GetBBox, I found the
> GetBBox method called without surrounding it with SetMatrixPropagation. Is that
> intentional? (I think it is, maybe?)

Yes, I think we should change things as you suggest there too.

> 
> I think some of them turns to be silly as I study the code by myself, but as of
> now, I'm wondering about them... If you have time, and want to spoil me, can
> you answer any of them? Or, is there documentation about gecko's code for SVG
> and gfx? I'm reading the source code using lxr, mercurial.

WYSIWYG I'm afraid.

> 
> Assuming my guess for the questions is correct, I cleaned some parts, compiling
> passed, but I'm not familiar firefox's test suite. At least, it seems reftest
> is OK...

Can you add an additional reftest for mask please? You would create two svg files, one that displays a mask with a non-trivial transform and uses objectBoundingBox and one without using objectBoundingBox then the reftest will check they display the same.

> 
> My current patch does do_QueryFrame twice at the caller and callee site, but I
> don't know how I should elegantly integrate the error handling, because
> SVGUtils::GetBBox fall backs to GetSVGBBoxForNonSVGFrame when the redundant
> 'if' failed in it.

I wouldn't worry about that.

> 
> Which should I use?
> rect.swap(*_retval);
> Or
> *_retval = rect;
> NS_ADDREF(*_retval);

You can't use NS_ADDREF because _retval could be null if the method failed. NS_IF_ADDREF is possible. Having said that swap would be better.

Please choose a formal reviewer for your next patch. Either of us will do.

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #8)
> > 3. Also, at nsSVGSwitchFrame.cpp:195 in nsSVGSwitchFrame::GetBBox, I found the
> > GetBBox method called without surrounding it with SetMatrixPropagation. Is that
> > intentional? (I think it is, maybe?)
> 
> Yes, I think we should change things as you suggest there too.

On further reflectio the switch code is fine as it is. It will call nsSVGDisplayContainerFrame::GetBBox which calls nsSVGUtilsGetBBox so it doesn't need changing.

One more thing. In nsSVGUtils, you don't need to prefix calls to other nsSVGUtils methods with nsSVGUtils::

Comment 10

[Jonathan Watt [:jwatt]]

Ah, midair collision. Ah well, I'll still give you my answers to your questions since I wrote them:

1) Yes, it is really OK. For both nsSVGPatternFrame::GetCallerGeometry and
nsSVGSVGElement::GetBBox we know the type of the frame that we're calling
GetBBox on, and we know it's not nsGkAtoms::svgGlyphFrame. That's the only type
me need to make that special check for.

2) Yes, you should not change nsSVGGraphicElement as well. I think I mistook
that for a frame and not an element when I was checking. Good catch!

3) Yes, it is intentional that nsSVGSwitchFrame::GetBBox does not call
SetMatrixPropagation. The GetBBox methods on nsSVGXxxFrame classes should not
do that when calling recursively into their children otherwise their children
would return a bbox that's relative to their parent frame's user space instead
of the user space of the frame on which GetBBox was first called.

The SVG code is not very well documented, sorry. We're working on that though.
For now best to ask questions.

For the do_QueryFrame, don't worry about that. The way the SVG code is set up
it will always get something for SVG frames. We only need the QI because
filters and masks can be applied to non-SVG content, but the code will do the
right thing with you're patch, so no worries.

For assigning to _retval you can just do:

  *_retval = nsSVGUtils::GetBBox(frame).get();

since _retval is expected to be addrefed.

Comment 11

[Robert Longson [:longsonr]]

Does this patch fix bug 480866 and or bug 457156 ?

Comment 12

[Jonathan Watt [:jwatt]]

It does indeed.

Comment 13

[Ryo Onodera]

Attachment: patch fixing not-antialiased clipPath

I've fixed really annoying bug of not-antialiased rendering of clipPath. Jaggy edges are too sore for my eyes. ;) This patch only changes one constant symbol. Can I mix this fix into the up-coming patch for bug489151, or should I file another bug at bugzilla even for such small fix? As for bug489151, I've created a reftest for it, but I need some more time to review API code cleaning part by myself with answers by you (thanks!). So, this attached patch contains only the tiny fix. It seems reftest is OK.

Comment 14

[Robert Longson [:longsonr]]

File a new bug please as that issue is unrelated.

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #13)
FWIW the fix is wrong, antialiasing should depend on mShapeRendering as it does for the NORMAL mode case. The right thing to do is to move up the mShapeRendering code up a bit (just below GeneratePath) and then remove the AntialiasMode call from the CLIP_MASK test.

Comment 16

[Ryo Onodera]

(In reply to comment #15)

I adopted my patch to your suggestion, and opened another bug at https://bugzilla.mozilla.org/show_bug.cgi?id=489718.

Comment 17

[Ryo Onodera]

Attachment: A patch.

I adapted some of answers from both of you(Jonathan Watt and Robert Longson) to my patch. Here it is.

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 374248 [details] [diff] [review]
A patch.

>+  nsCOMPtr<nsIDOMSVGRect> rect;
>+  if (callerType == nsGkAtoms::svgGlyphFrame) {
>+    *aBBox = nsSVGUtils::GetBBox(aSource->GetParent()).get();
>+  } else {
>+    *aBBox = nsSVGUtils::GetBBox(aSource).get();
>+  }

It seems 'rect' is something left over that you forgot to remove.

So the code changes here look spot on. I'd rather have a much simpler test though. We generally try to make our tests reduced testcases, and that means the minimum amount of code necessary to reproduce the problem. Also it's good if you can design the testcase to just fill the content area with green so that the comparison can be against pass.svg, and so that it's easy to see at a glance whether a test has passed or failed. One final thing, we generally put any human readable documentation in a comment. Actually, we should be using <desc> though.

In this case I'd like to use something like this:

<svg xmlns="http://www.w3.org/2000/svg">
  <title>Test objectBoundingBox clip-path on element with ancestor transform</title>
  <desc>
    This test checks that the bbox calculation for an objectBoundingBox
    clip-path is correctly getting the bbox in the userspace of the
    clipped element, and not it's bbox in an ancestor userspace or rootspace.
  </desc>
  <clipPath id="clip" clipPathUnits="objectBoundingBox">
    <circle cx="0.5" cy="0.5" r="0.48"/>
  </clipPath>
  <rect width="100%" height="100%" fill="lime"/>
  <circle cx="50" cy="50" r="46" fill="red"/>
  <g transform="translate(100,0)">
    <g clip-path="url(#clip)">
      <rect x="-100" width="100" height="100" fill="red"/>
      <circle cx="-50" cy="50" r="50" fill="lime"/>
    </g>
  </g>
</svg>


Unfortunately that won't pass due to bug 489776. Grr.

Comment 19

[Robert Longson [:longsonr]]

(In reply to comment #18)
> 
> Unfortunately that won't pass due to bug 489776. Grr.

What should Ryo do in this bug then?

Comment 20

[Jonathan Watt [:jwatt]]

Sorry, had to nip out. I was thinking that I had to use non-rectangular clipping to make sure the test will fail in implementations that don't implement clipPath at all, but of course that's not true. So we can just have:

<svg xmlns="http://www.w3.org/2000/svg">
  <title>Test objectBoundingBox clip-path on element with ancestor transform</title>
  <desc>
    This test checks that the bbox calculation for an objectBoundingBox
    clip-path is correctly getting the bbox in the userspace of the
    clipped element, and not it's bbox in an ancestor userspace or rootspace.
  </desc>
  <clipPath id="clip" clipPathUnits="objectBoundingBox">
    <rect x="0.5" width="0.5" height="1"/>
  </clipPath>
  <rect width="100%" height="100%" fill="lime"/>
  <rect x="100" width="100" height="100" fill="red"/>
  <g transform="translate(-100,0)">
    <g clip-path="url(#clip)">
      <rect x="100" width="100" height="100" fill="red"/>
      <rect x="200" width="100" height="100" fill="lime"/>
    </g>
  </g>
</svg>

Can you update the patch Ryo, with something like the above for clip, and a add a second similar test for mask?

Comment 21

[Ryo Onodera]

Attachment: patch v5

A patch v5 with the unused variable 'rect' removed and with refined test cases based on Comment 20.

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 374248 [details] [diff] [review]
A patch.

Sorry, I should have said, but can you create the final patch using 'hg export' so that when it gets committed and pushed by someone with access to mozilla-central, the changeset will credit you, and it will include your commit message. Your hg username should be something like 'Ryo Onodera <[e-mail]>', and the message should be something like: "Bug 489151. Masking and clipping with objectBoundingBox and non-trivial transforms is broken. r=jwatt".

Also, can you add the public domain dedication to the tests and a comment saying that they came frome this bug (as you had in your original testcase). Since you need to do that, can you also fix the <title> in the tests so that it isn't wrapped like in the bugzilla comment above. You also don't need the 'stroke' attribute on the <rect> in the mask test.

Finally, can you remove the following comment from nsSVGPatternFrame.cpp that's next to the change you made there. It was wrong before and after your change.

>   // Get the calling geometry's bounding box.  This
>   // will be in *device coordinates*

All that done, I think we're good to go. :-)

Comment 23

[Ryo Onodera]

Attachment: patch v6

An updated patch created by hg export with all suggestions applied.

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 374300 [details] [diff] [review]
patch v6

Thanks! I'll check this shortly.

Comment 25

[Robert Longson [:longsonr]]

The reftest files have no numbers in them. I don't know whether you care enough to fix them up on check-in Jonathan.

Comment 26

[Jonathan Watt [:jwatt]]

No numbers? In any case, too late. :-( I just pushed http://hg.mozilla.org/mozilla-central/rev/cf138fd828e6

Comment 27

[Jonathan Watt [:jwatt]]

Thanks a lot Ryo!

Comment 28

[Ryo Onodera]

You're welcome.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 374300 [details] [diff] [review]
patch v6

a191=beltzner

Comment 30

[Robert Longson [:longsonr]]

Checked in http://hg.mozilla.org/releases/mozilla-1.9.1/rev/83b8bdf6ef4b

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031307

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090512 Shiretoko/3.5b5pre ID:20090512031310