Closed Bug 594019 Opened 14 years ago Closed 11 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: mstange, Assigned: osk)

Details

(Keywords: perf)

Attachments

(2 files, 5 obsolete files)

Changed "Luminance" to "luminance" in comment (for consistenty) as per the last review comment.
7.52 KB, patch
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
Details 
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.
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)
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 :-)
Excellent spot that it's possible to do that BTW, very impressive.
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.
Attachment #717544 - Attachment is obsolete: true
Attachment #717544 - Flags: review?(jwatt)
Assignee: nobody → osk
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)
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)
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)
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.
So when we've got the unpremultiply -> sRGB -> linearRGB -> intensity case do that all in nsSVGUtils.
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)
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 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.
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 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)
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).
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)
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)
(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.
Attachment #718880 - Flags: review?(longsonr) → review+
Is it okay to now add the checkin-needed keyword?
Did you run the reftests in layout/reftests/svg and they passed? If so then yes you can add, add checkin-needed.
Flags: in-testsuite-
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.
cd layout/reftests/svg
<path-to-firefox>/firefox.exe -reftest reftest.list > logfile.txt
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.
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.)
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.)
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)
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.
https://hg.mozilla.org/mozilla-central/rev/a00c10c44814
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: