Consider making the calculation in nsSVGMaskFrame::ComputeMaskAlpha one-pass

RESOLVED FIXED in mozilla22

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mstange, Assigned: O S K Chaitanya)

Tracking

({perf})

Trunk
mozilla22
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
From bug 593962:

nsSVGMaskFrame::ComputeMaskAlpha first converts the mask image data from sRGB to linear RGB, and then to luminance. For performance we might want to consider handcoding an sRGB version of the luminance calculation, so that we can avoid doing two passes.
(Assignee)

Comment 1

4 years ago
Created attachment 717544 [details] [diff] [review]
Moved the UnPreMultiplyImageDataAlpha into the block for NS_STYLE_MASK_TYPE_LUMINANCE

I have started working on a fix for this bug. Since I have never contributed to the mozilla project before, I am trying to start with small steps. I noticed that in nsSVGMaskFrame::ComputeMaskAlpha, nsSVGUtils::UnPremultiplyImageDataAlpha is called before the test: StyleSVGReset()->mMaskType == NS_STYLE_MASK_TYPE_LUMINANCE.

However, the actual R, G, B values are only used when the above condition returns true. So, we can call nsSVGUtils::UnPremultiplyImageDataAlpha only in the block corresponding to it being true.

I have attached a patch with this change (it has enough lines of context to easily see the point I have made above).

If my review request should be directed to someone else, please let me know.
Also, could you also please point me to any automated tests for these changes.
Attachment #717544 - Flags: review?(jwatt)

Comment 2

4 years ago
This is great but as it doesn't fix this bug could you create another bug and add your patch there please? I'd give it an r+ if you did that :-)

Comment 3

4 years ago
Excellent spot that it's possible to do that BTW, very impressive.
(Assignee)

Comment 4

4 years ago
This was meant to be an intermediate step to the full fix. However, I will do as you have suggested and create a new bug and add the patch to that one instead.
(Assignee)

Comment 5

4 years ago
Here it is: https://bugzilla.mozilla.org/show_bug.cgi?id=844511

Updated

4 years ago
Attachment #717544 - Attachment is obsolete: true
Attachment #717544 - Flags: review?(jwatt)

Updated

4 years ago
Assignee: nobody → osk
(Assignee)

Comment 6

4 years ago
Created attachment 718161 [details] [diff] [review]
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass

The following lines in nsSVGMaskFrame.cpp are anyway multiplying with a/255.0 at the end. So, I have eliminated the part where we are doing (255 * r)/a (likewise for 'g', 'b') in the unpremulitply step. Unless the difference from truncation in the earlier case and the current case is of significance, this:
              alpha =                                                                                     
                static_cast<uint8_t>                                                                      
                           ((pixel[GFX_ARGB32_OFFSET_R] * 0.2125 +                                        
                                pixel[GFX_ARGB32_OFFSET_G] * 0.7154 +                                     
                                pixel[GFX_ARGB32_OFFSET_B] * 0.0721) *                                    
                               (pixel[GFX_ARGB32_OFFSET_A] / 255.0) * aOpacity);
being done after unpremultiply is the same as:
              alpha =                                                                                     
                static_cast<uint8_t>                                                                      
                           ((pixel[GFX_ARGB32_OFFSET_R] * 0.2125 +                                        
                                pixel[GFX_ARGB32_OFFSET_G] * 0.7154 +                                     
                                pixel[GFX_ARGB32_OFFSET_B] * 0.0721) *                                    
                                aOpacity);
without a preceding unpremultiply step.

So overall, we have cut down a few operations (not just made it one pass).
Attachment #718161 - Flags: review?(longsonr)
(Assignee)

Comment 7

4 years ago
Comment on attachment 718161 [details] [diff] [review]
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass

Found a logical issue with this. My bad. I'll make an updated one and upload again.
Attachment #718161 - Attachment is obsolete: true
Attachment #718161 - Flags: review?(longsonr)
(Assignee)

Comment 8

4 years ago
Created attachment 718193 [details] [diff] [review]
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass

I have exposed the mapping array for sRGB to linearRGB through a function in nsSVGUtils.cpp and have used it to make the operation complete in a single pass. I have tried to stick to the coding conventions followed in the file as far as possible. Please let me know if I need to make any changes to this patch.

I have removed (a/255.0) and premultiply in the case where these two operations were cancelling each other out.
Attachment #718193 - Flags: review?(longsonr)

Comment 9

4 years ago
I think it would be better if you created a new function in nsSVGUtils rather than having a GetsRGBToLinearRGBMap function. You should be able to make a common method to do the unpremultiply in that case.

Comment 10

4 years ago
So when we've got the unpremultiply -> sRGB -> linearRGB -> intensity case do that all in nsSVGUtils.
(Assignee)

Comment 11

4 years ago
Comment on attachment 718193 [details] [diff] [review]
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass

I'll upload a new patch with the changes suggested by Robert Longson.
Attachment #718193 - Attachment is obsolete: true
Attachment #718193 - Flags: review?(longsonr)
(Assignee)

Comment 12

4 years ago
Created attachment 718487 [details] [diff] [review]
Moved the per pixel code into nsSVGUtils.cpp

I have moved the per pixel code into nsSVGUtils.cpp. This looks much cleaner than before (IMO). However, if this needs further changes, please let me know.
Attachment #718487 - Flags: review?(longsonr)

Comment 13

4 years ago
Comment on attachment 718487 [details] [diff] [review]
Moved the per pixel code into nsSVGUtils.cpp

> # HG changeset patch
> # User O S K Chaitanya <osk@medhas.org>
> Bug 594019 - Consider making the calculation in nsSVGMaskFrame::ComputeMaskAlpha one-pass

Add r=longsonr at the end of that please.

>+      nsSVGUtils::ConvertsRGBToLinearRGBInterpLuminance(data,
>+                                                        stride,
>+                                                        rect,
>+                                                        aOpacity);

This can be nsSVGUtils::ComputeLinearRGBLuminanceMask

>+    } else {
>+      nsSVGUtils::ConvertsRGBToLuminance(data, stride, rect, aOpacity);

This can be nsSVGUtils::ComputesRGBLuminanceMask

>-    }
>+    nsSVGUtils::ConvertRGBToAlphaAsLuminance(data, stride, rect, aOpacity);

And so let's call this nsSVGUtils::ComputeAlphaMask

> +void
> +nsSVGUtils::ConvertsRGBToLuminance(uint8_t *data,
> +                                   int32_t stride,
> +                                   const nsIntRect &rect,
> +                                   float opacity)

Please use "a" on arguments so data becomes aData, stride is aStride etc.

> +        /* sRGB -> intensity (unpremultiply cancels out the
> +         * (a/255.0) multiplicatin with aOpacity */

multiplicatin should be multiplication I think.

If you make these changes I think we'll be done.
(Assignee)

Comment 14

4 years ago
I am making these changes now. I had seen that the functions to convert to and from Linear RGB, and the premultiply, unpremultiply functions use 'data', 'stride' etc. So, though I had seen the coding conventions for Mozilla specify that we use the 'aData' form, I chose the 'data' form for consistency with the functions I just mentioned.

Would you like me to also change those to the 'aData' form?
(Assignee)

Comment 15

4 years ago
Comment on attachment 718487 [details] [diff] [review]
Moved the per pixel code into nsSVGUtils.cpp

About to submit a newer patch.
Attachment #718487 - Attachment is obsolete: true
Attachment #718487 - Flags: review?(longsonr)
(Assignee)

Comment 16

4 years ago
Created attachment 718831 [details] [diff] [review]
updated with changes suggested by review of the earlier patch

I have updated it with the changes suggested in the earlier review. I have also changed the comments in nsSVGUtils.h to say "luminance" instead of "intensity".

Please let me know if this will serve.
Attachment #718831 - Flags: review?(longsonr)
Comment on attachment 718831 [details] [diff] [review]
updated with changes suggested by review of the earlier patch

>+  /*
>+   * Converts image data from sRGB to luminance assuming
>+   * Linear RGB Interpolation
>+   */
>+  static void ComputeLinearRGBLuminanceMask(uint8_t *aData,
>+                                            int32_t aStride,
>+                                            const nsIntRect &aRect,
>+                                            float aOpacity);
>+  /*
>+   * Converts image data to Luminance using the value of alpha as luminance
>+   */
>+  static void ComputeAlphaMask(uint8_t *aData,

nit: don't capitalize "Luminance" in that comment (for consistency).
(Assignee)

Comment 18

4 years ago
Comment on attachment 718831 [details] [diff] [review]
updated with changes suggested by review of the earlier patch

About to upload newer version.
Attachment #718831 - Attachment is obsolete: true
Attachment #718831 - Flags: review?(longsonr)
(Assignee)

Comment 19

4 years ago
Created attachment 718880 [details] [diff] [review]
Changed "Luminance" to "luminance" in comment (for consistenty) as per the last review comment.

Moved all per pixel code to nsSVGUtils.cpp from nsSVGMaskFrame.cpp (for ComputeMaskAlpha)

I just changed that one character in the patch from 'L' to 'l' and submitted it. It will be a few hours before I get home and have access to the source to be able to create a fresh patch if that is desired.
Attachment #718880 - Flags: review?(longsonr)

Comment 20

4 years ago
(In reply to O S K Chaitanya from comment #14)
> I am making these changes now. I had seen that the functions to convert to
> and from Linear RGB, and the premultiply, unpremultiply functions use
> 'data', 'stride' etc. So, though I had seen the coding conventions for
> Mozilla specify that we use the 'aData' form, I chose the 'data' form for
> consistency with the functions I just mentioned.
> 
> Would you like me to also change those to the 'aData' form?

You can if you want, that would be another bug though.

Updated

4 years ago
Attachment #718880 - Flags: review?(longsonr) → review+
(Assignee)

Comment 21

4 years ago
Is it okay to now add the checkin-needed keyword?

Comment 22

4 years ago
Did you run the reftests in layout/reftests/svg and they passed? If so then yes you can add, add checkin-needed.

Updated

4 years ago
Flags: in-testsuite-
(Assignee)

Comment 23

4 years ago
I could not figure out how to run tests only in the layout/reftests/svg directory. The full suite at the top level runs a very long time on my machine.

Comment 24

4 years ago
cd layout/reftests/svg
<path-to-firefox>/firefox.exe -reftest reftest.list > logfile.txt

Comment 25

4 years ago
I've sent this to the try server

https://tbpl.mozilla.org/?tree=Try&rev=81f15c0ef074

If that's green I can push it for you.

Comment 26

4 years ago
Don't worry I'll fix your name before landing on mozilla-inbound.
(In reply to Robert Longson from comment #24)
> cd layout/reftests/svg
> <path-to-firefox>/firefox.exe -reftest reftest.list > logfile.txt

(Alternately, from your mozilla-central source directory:
> TEST_PATH=layout/reftests/svg ./mach reftest
That's the "recommended" way to run reftests -- it sets up a temporary profile with a few prefs toggled that are required for some reftests (outside of SVG probably) to run.)
(Assignee)

Comment 28

4 years ago
I took the approach suggested by Daniel Holbert. I don't know if this is normal but I get 480 lines with TEST-UNEXPECTED-FAIL in the output with the latest mozilla-central code (pulled a few minutes ago). This was _without_ my patch:
$ export TEST_PATH=./layout/reftests/svg
$ ./mach reftest > reftest_master.out 2> reftest_master.err
$ grep TEST-UNEXPECTED-FAIL reftest_master.out | wc -l
480

after checking out the branch corresponding to this patch, I followed the same steps and I saw 630 as output.

This confuses me all the more because I see this on the Tinderbox page (link given by Robert Longson in Comment 25):

    OS X 10.6 opt C R
    OS X 10.6 debug C R
    OS X 10.7 optB C R
    OS X 10.7 debug B C R
    OS X 10.8 opt C R
    OS X 10.8 debug C R
Where all the 'R's are green.

However, the lines there say OS X while I am on a Ubuntu 12.04 (x86_64).
That "./mach reftest" run should generate a file called "reftest.log" in your build directory.  Could you attach that file to this bug?
(I'm curious to see what your local test-failures look like.  Incidentally, you can examine them yourself by visiting
 http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml
and using the file-picker box to choose your reftest.log file.)
(Assignee)

Comment 31

4 years ago
Created attachment 719170 [details]
logs of svg reftest with unmodified mozilla-central code and another reftest with the patch in this bug

I have attached a tar.bz2 which has reftest_master.log (unmodified mozilla-central code svg reftest result) and reftest_patched.log (this patch applied over mozilla-central code and svg reftest done on it). I had to compress them because I am on a slow conneciton.
That's bizarre -- your system's reftest run is hitting some serious failures.  Are you building with system-cairo or anything else non-standard?

If you get a chance, find me in irc.mozilla.org (e.g. by visiting http://irc.lc/mozilla/developers/osk ) and we can investigate in real-time a bit better.  (I'm "dholbert" there)
(Assignee)

Comment 33

4 years ago
Alright. I'll log in once I am at home. That will be around 8-9 hours from the time of this post.
Cool! I might be logging on a few hours after that (maybe around 8-9 hours from the time of this post).

If you're still on at that point, cool.  If not, then I'd I can just post here -- I'm basically interested interested in:

 a) whether you're using any non-standard build configuration options (for example, "enable-system-cairo"), which might explain your huge number of reftest failures

 b) whether you see the same sorts of issues reflected in your reftest log when you load the files directly in your build. (e.g. if you load https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/as-image/lime100x100-ref.html in your build, do you see a light-green square?  That's the reference case that seems to be loading as white in your first few test-failures

In any case -- for the purposes of this bug's patch -- in my local build (on Ubuntu 12.10), I get the same set of SVG reftest-failures with the patch as I do without the patch*.  So this patch doesn't seem to break anything for me.  Given that it doesn't break anything on Mac or Linux (at least, my linux system), I think it's good to land.

Robert, want to push it?

* My local reftest run (with and without the patch) just has 4 "unexpected" failures in SVG, w/ visually-imperceptible rendering differences.

Comment 35

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00c10c44814
https://hg.mozilla.org/mozilla-central/rev/a00c10c44814
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.