Last Comment Bug 594019 - Consider making the calculation in nsSVGMaskFrame::ComputeMaskAlpha one-pass
: Consider making the calculation in nsSVGMaskFrame::ComputeMaskAlpha one-pass
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: O S K Chaitanya
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-07 07:45 PDT by Markus Stange [:mstange]
Modified: 2013-02-28 09:57 PST (History)
5 users (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moved the UnPreMultiplyImageDataAlpha into the block for NS_STYLE_MASK_TYPE_LUMINANCE (2.95 KB, patch)
2013-02-23 13:28 PST, O S K Chaitanya
no flags Details | Diff | Splinter Review
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass (5.28 KB, patch)
2013-02-25 16:16 PST, O S K Chaitanya
no flags Details | Diff | Splinter Review
patch to make nsSVGMaskFrame::ComputeMaskAlpha one pass (6.04 KB, patch)
2013-02-25 17:55 PST, O S K Chaitanya
no flags Details | Diff | Splinter Review
Moved the per pixel code into nsSVGUtils.cpp (7.79 KB, patch)
2013-02-26 09:33 PST, O S K Chaitanya
no flags Details | Diff | Splinter Review
updated with changes suggested by review of the earlier patch (7.52 KB, patch)
2013-02-26 21:43 PST, O S K Chaitanya
no flags Details | Diff | Splinter Review
Changed "Luminance" to "luminance" in comment (for consistenty) as per the last review comment. (7.52 KB, patch)
2013-02-27 01:31 PST, O S K Chaitanya
longsonr: review+
Details | Diff | Splinter Review
logs of svg reftest with unmodified mozilla-central code and another reftest with the patch in this bug (1.10 MB, application/octet-stream)
2013-02-27 13:58 PST, O S K Chaitanya
no flags Details

Description Markus Stange [:mstange] 2010-09-07 07:45:02 PDT
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.
Comment 1 O S K Chaitanya 2013-02-23 13:28:42 PST
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.
Comment 2 Robert Longson 2013-02-23 13:36:21 PST
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 Robert Longson 2013-02-23 13:39:46 PST
Excellent spot that it's possible to do that BTW, very impressive.
Comment 4 O S K Chaitanya 2013-02-23 13:44:03 PST
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.
Comment 5 O S K Chaitanya 2013-02-23 13:54:17 PST
Here it is: https://bugzilla.mozilla.org/show_bug.cgi?id=844511
Comment 6 O S K Chaitanya 2013-02-25 16:16:16 PST
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).
Comment 7 O S K Chaitanya 2013-02-25 16:21:03 PST
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.
Comment 8 O S K Chaitanya 2013-02-25 17:55:31 PST
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.
Comment 9 Robert Longson 2013-02-26 01:48:51 PST
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 Robert Longson 2013-02-26 01:54:06 PST
So when we've got the unpremultiply -> sRGB -> linearRGB -> intensity case do that all in nsSVGUtils.
Comment 11 O S K Chaitanya 2013-02-26 09:30:18 PST
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.
Comment 12 O S K Chaitanya 2013-02-26 09:33:43 PST
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.
Comment 13 Robert Longson 2013-02-26 13:02:29 PST
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.
Comment 14 O S K Chaitanya 2013-02-26 21:28:47 PST
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?
Comment 15 O S K Chaitanya 2013-02-26 21:40:39 PST
Comment on attachment 718487 [details] [diff] [review]
Moved the per pixel code into nsSVGUtils.cpp

About to submit a newer patch.
Comment 16 O S K Chaitanya 2013-02-26 21:43:21 PST
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.
Comment 17 Daniel Holbert [:dholbert] 2013-02-26 23:35:10 PST
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).
Comment 18 O S K Chaitanya 2013-02-27 01:29:17 PST
Comment on attachment 718831 [details] [diff] [review]
updated with changes suggested by review of the earlier patch

About to upload newer version.
Comment 19 O S K Chaitanya 2013-02-27 01:31:45 PST
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.
Comment 20 Robert Longson 2013-02-27 01:35:39 PST
(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.
Comment 21 O S K Chaitanya 2013-02-27 02:15:37 PST
Is it okay to now add the checkin-needed keyword?
Comment 22 Robert Longson 2013-02-27 04:20:19 PST
Did you run the reftests in layout/reftests/svg and they passed? If so then yes you can add, add checkin-needed.
Comment 23 O S K Chaitanya 2013-02-27 06:10:06 PST
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 Robert Longson 2013-02-27 06:14:51 PST
cd layout/reftests/svg
<path-to-firefox>/firefox.exe -reftest reftest.list > logfile.txt
Comment 25 Robert Longson 2013-02-27 08:58:55 PST
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 Robert Longson 2013-02-27 09:08:14 PST
Don't worry I'll fix your name before landing on mozilla-inbound.
Comment 27 Daniel Holbert [:dholbert] 2013-02-27 09:10:43 PST
(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.)
Comment 28 O S K Chaitanya 2013-02-27 12:59:40 PST
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).
Comment 29 Daniel Holbert [:dholbert] 2013-02-27 13:05:15 PST
That "./mach reftest" run should generate a file called "reftest.log" in your build directory.  Could you attach that file to this bug?
Comment 30 Daniel Holbert [:dholbert] 2013-02-27 13:06:38 PST
(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.)
Comment 31 O S K Chaitanya 2013-02-27 13:58:19 PST
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.
Comment 32 Daniel Holbert [:dholbert] 2013-02-27 14:21:52 PST
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)
Comment 33 O S K Chaitanya 2013-02-27 22:58:56 PST
Alright. I'll log in once I am at home. That will be around 8-9 hours from the time of this post.
Comment 34 Daniel Holbert [:dholbert] 2013-02-28 00:16:18 PST
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 36 Gregory Szorc [:gps] 2013-02-28 09:57:15 PST
https://hg.mozilla.org/mozilla-central/rev/a00c10c44814

Note You need to log in before you can comment on or make changes to this bug.