844511 - Only luminance masks need unpremultiplying and conversion to linear RGB

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[O S K Chaitanya]

Attachment: svg_UnPreMultiplyImageDataAlpha_only_for_relevant_masktype.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130218103317

Steps to reproduce:

Studied nsSVGMaskFrame.cpp.


Actual results:

nsSVGUtils::UnPremultiplyImageDataAlpha and nsSVGUtils::ConvertImageDataToLinearRGB are called before checking what  StyleSVGReset()->mMaskType == NS_STYLE_MASK_TYPE_LUMINANCE returns.


Expected results:

They should only be called when it returns true.

Comment 1

[O S K Chaitanya]

Comment on attachment 717549 [details] [diff] [review]
svg_UnPreMultiplyImageDataAlpha_only_for_relevant_masktype.patch

Moved the calls to nsSVGUtils::UnPremultiplyImageDataAlpha and nsSVGUtils::ConvertImageDataToLinearRGB to the block corresponding to StyleSVG()->mColorInterpolation == NS_STYLE_COLOR_INTERPOLATION_LINEARRGB being true

Comment 2

[O S K Chaitanya]

It would be very nice if someone could point me to some any existing test cases for the masking related code in SVG.

Comment 3

[Robert Longson [:longsonr]]

layout/reftests/svg/mask-type-01.svg layout/reftests/svg/mask-type-02.svg layout/reftests/svg/mask-type-03.svg all test alpha masks. The other files in layout/reftests/svg with mask in their titles test luminance masks.

Comment 4

[Robert Longson [:longsonr]]

I can probably land this for you tomorrow.

Comment 5

[O S K Chaitanya]

Forgive me, but I don't understand what you mean by "land this". Are you offering to commit this patch into trunk?

Comment 6

[Robert Longson [:longsonr]]

Yes.

Comment 7

[O S K Chaitanya]

Thank you! So, is it now safe to assume this means I need not add the "checkin-needed" keyword mentioned on this page: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch ?

Comment 8

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebe5600711d

Comment 9

[Robert Longson [:longsonr]]

(In reply to O S K Chaitanya from comment #7)
> Thank you! So, is it now safe to assume this means I need not add the
> "checkin-needed" keyword mentioned on this page:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch ?

If you want to add checkin-needed your patch ought to follow these rules: https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

As it's your first time I let you off jumping through those hoops ;-)

Your patch is on the mozilla-inbound trunk tree now and should make it to mozilla-central within the next 24 hours.

Comment 10

[Robert Longson [:longsonr]]

Oh and thanks for the patch.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0ebe5600711d