Closed Bug 1228354 Opened 8 years ago Closed 8 years ago

(mask-image) Support 'luminance' and 'match-source' values for 'mask-mode'

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: u459114, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 9 obsolete files)

Implementation in Bug 686281 only partially support alpha mask-mode.

The mask-mode property indicates whether the <mask-reference> is treated as luminance mask or alpha mask. (See Mask processing.)

<masking-mode>
 = alpha | luminance | auto 

luminance
A value of luminance indicates that the luminance values of the mask layer image should be used as the mask values. See Calculating mask values. 

auto
If the <mask-reference> of the mask-image property is of type <mask-source> the luminance values of the mask layer image should be used as the mask values.
If the <mask-reference> of the mask-image property is of type <image> the alpha values of the mask layer image should be used as the mask values.
Assignee: cku → ethlin
Ehtan, we also need a reftest for mask-mode in layout/reftests/w3c-css/submitted/masking
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1247560
Attached patch Implement luminance mask mode (obsolete) — Splinter Review
Add the implementation of luminance for mask image.
Attachment #8719353 - Flags: feedback?(cku)
Comment on attachment 8719353 [details] [diff] [review]
Implement luminance mask mode

Review of attachment 8719353 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Types.h
@@ +172,5 @@
>    FULL
>  };
>  
> +enum class MaskOp : int8_t {
> +  OP_NONE,

+ OP_ALPHA,
+ OP_AUTO,

::: layout/base/nsCSSRendering.h
@@ +809,5 @@
>    }
>  
> +  static MaskOp GetGFXMaskOp(uint8_t aMaskMode) {
> +    switch (aMaskMode) {
> +      case NS_STYLE_MASK_MODE_ALPHA:      return MaskOp::OP_NONE;

return MaskOp::OP_ALPHA;

@@ +811,5 @@
> +  static MaskOp GetGFXMaskOp(uint8_t aMaskMode) {
> +    switch (aMaskMode) {
> +      case NS_STYLE_MASK_MODE_ALPHA:      return MaskOp::OP_NONE;
> +      case NS_STYLE_MASK_MODE_LUMINANCE:  return MaskOp::OP_LUMINANCE;
> +      case NS_STYLE_MASK_MODE_AUTO:       return MaskOp::OP_NONE;

return MaskOp::OP_AUTO;

::: layout/base/nsLayoutUtils.cpp
@@ +212,5 @@
> +      pixel += 4;
> +    }
> +    pixel += offset;
> +  }
> +}

Should we have a follow up bug to optimize this function by using libyuv?

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +538,5 @@
>  
>      Matrix maskTransform;
>      RefPtr<SourceSurface> maskSurface;
>      if (svgMaskFrame) {
> +      aContext.SetMaskOp(nsCSSRendering::GetGFXMaskOp(svgReset->mLayers.mLayers[0].mMaskMode));

indent

::: layout/svg/nsSVGMaskFrame.cpp
@@ +306,5 @@
>      return nullptr;
>    }
>  
> +  if (StyleSVGReset()->mMaskType == NS_STYLE_MASK_TYPE_LUMINANCE ||
> +      aContext->CurrentMaskOp() == MaskOp::OP_LUMINANCE) {

mask spec wrote:
The mask-type property allows the author of the mask element to specify the preferred masking mode. However, the author can override this preference by setting the mask-mode value to something different than auto on the masked content.

if aContext->CurrentMaskOp() == MaskOp::OP_AUTO, follow StyleSVGReset()->mMaskType, otherwise, follow aContext->CurrentMaskOp()
Attachment #8719353 - Flags: feedback?(cku) → feedback+
Depends on: 1224424
Comment on attachment 8719353 [details] [diff] [review]
Implement luminance mask mode

Review of attachment 8719353 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Types.h
@@ +172,5 @@
>    FULL
>  };
>  
> +enum class MaskOp : int8_t {
> +  OP_NONE,

Okay.

::: layout/base/nsCSSRendering.h
@@ +809,5 @@
>    }
>  
> +  static MaskOp GetGFXMaskOp(uint8_t aMaskMode) {
> +    switch (aMaskMode) {
> +      case NS_STYLE_MASK_MODE_ALPHA:      return MaskOp::OP_NONE;

Okay.

::: layout/base/nsLayoutUtils.cpp
@@ +212,5 @@
> +      pixel += 4;
> +    }
> +    pixel += offset;
> +  }
> +}

I don't find a suitable function in libyuv. I think we can open a follow up bug to optimize it by neon and sse.

::: layout/svg/nsSVGMaskFrame.cpp
@@ +306,5 @@
>      return nullptr;
>    }
>  
> +  if (StyleSVGReset()->mMaskType == NS_STYLE_MASK_TYPE_LUMINANCE ||
> +      aContext->CurrentMaskOp() == MaskOp::OP_LUMINANCE) {

Right, I will fix it.
Address CJ's comments.
Attachment #8719353 - Attachment is obsolete: true
Add reftest.
Blocks: mask-ship
Status: NEW → ASSIGNED
Attachment #8723358 - Flags: review?(mstange)
Comment on attachment 8723358 [details] [diff] [review]
Part1 - Implement luminance mask mode

Review of attachment 8723358 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Types.h
@@ +176,5 @@
> +  OP_AUTO,
> +  OP_ALPHA,
> +  OP_LUMINANCE
> +};
> +

in bug 1224424, I rename auto to match-source
(In reply to C.J. Ku[:cjku] from comment #7)
> Comment on attachment 8723358 [details] [diff] [review]
> Part1 - Implement luminance mask mode
> 
> Review of attachment 8723358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Types.h
> @@ +176,5 @@
> > +  OP_AUTO,
> > +  OP_ALPHA,
> > +  OP_LUMINANCE
> > +};
> > +
> 
> in bug 1224424, I rename auto to match-source

Okay, I will change AUTO to MATCH_SOURCE as well.
Change the AUTO to MATCH_SOURCE.
Attachment #8723358 - Attachment is obsolete: true
Attachment #8723358 - Flags: review?(mstange)
Attachment #8726051 - Flags: review?(mstange)
Summary: (mask-image) Support luminance and auto mask-mode → (mask-image) Support 'luminance' and 'match-source' values for 'mask-mode'
Comment on attachment 8726051 [details] [diff] [review]
Part1 - Implement luminance mask mode

Review of attachment 8726051 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Types.h
@@ +172,5 @@
>    NORMAL,
>    FULL
>  };
>  
> +enum class MaskOp : int8_t {

This file is for enums used in the Moz2D API (2D.h), and for things that have their rendering code somewhere in gfx/2d/. MaskOp is neither; it's a CSS concept that Moz2D knows nothing about.

::: gfx/thebes/gfxContext.h
@@ +368,5 @@
> +    /**
> +     * Sets the operator used for all further drawing. The operator
> +     * affects the alpha calculation of the source.
> +     */
> +    void SetMaskOp(MaskOp op);

gfxContext is not a good place for this either. You seem to use gfxContext purely as a value storage to forward the mask op from nsCSSRendering and from nsSVGIntegrationUtils to DrawImageInternal and to  nsSVGMaskFrame::GetMaskForMaskedFrame. Please find a different way of forwarding this piece of information, if you really have to forward it.

gfxContext is for things that code in gfxContext.cpp makes use of (usually by forwarding it to the wrapped DrawTarget).

::: layout/base/nsLayoutUtils.cpp
@@ +200,5 @@
> +RGBALuminanceOperation(uint8_t *aData,
> +                       int32_t aStride,
> +                       const IntSize &aSize)
> +{
> +  int32_t redFactor = 55;    // 255 * 0.2125

The numbers are multiplied by 255, but further down you divide by 256. Why don't you multiply by 256 here? With this code you get things like (255,255,255) mapping to 254 instead of 255.

@@ +6558,5 @@
> +    if (maskOp == MaskOp::OP_LUMINANCE) {
> +      RefPtr<SourceSurface> surf = destCtx->GetDrawTarget()->Snapshot();
> +      RefPtr<DataSourceSurface> maskData = surf->GetDataSurface();
> +      DataSourceSurface::MappedSurface map;
> +      if (!maskData->Map(DataSourceSurface::MapType::READ, &map)) {

What you're doing here is the following: You're lying to Map by passing READ, and then you write to the pixels and hope that this will change the original pixels in the DrawTarget, so that further down, snapshotting the DrawTarget will still contain your changes.

This is not how you should do it. It works purely by accident, and probably won't work with Direct2D.

What you instead want to do is:
if (!tmpDTRect.IsEmpty()) {
  RefPtr<SourceSurface> surf = destCtx->GetDrawTarget()->Snapshot();
  if (mask op is luminance) {
    RefPtr<DataSourceSurface> maskData = surf->GetDataSurface();
    ...
    if (!maskData->Map(...WRITE, ...)) {
      ...
    }
    RGBALuminanceOperation(map.mData, map.mStride, maskSize);
    maskData->Unmap();

    surf = maskData;
  }
  ... paint surf
}
Attachment #8726051 - Flags: review?(mstange)
How about this: In order to forward the mask op to GetMaskForMaskedFrame, just pass it as an argument.

And for nsCSSRendering, I would try moving the code that Bas added to nsLayoutUtils.cpp in https://hg.mozilla.org/mozilla-central/rev/fc6e7bbaf2ad back to the place in ImageRenderer where he removed the old code. I don't know why he moved it.
Then you can do your luminance adjustment in ImageRenderer and don't have to forward information into nsLayoutUtils.cpp.

Bas, can you explain why you moved the intermediate surface into nsLayoutUtils.cpp? Was it because it's easier to know the size of the intermediate surface there?
Flags: needinfo?(bas)
(In reply to Markus Stange [:mstange] from comment #11)
> How about this: In order to forward the mask op to GetMaskForMaskedFrame,
> just pass it as an argument.
> 
> And for nsCSSRendering, I would try moving the code that Bas added to
> nsLayoutUtils.cpp in https://hg.mozilla.org/mozilla-central/rev/fc6e7bbaf2ad
> back to the place in ImageRenderer where he removed the old code. I don't
> know why he moved it.
> Then you can do your luminance adjustment in ImageRenderer and don't have to
> forward information into nsLayoutUtils.cpp.
> 
> Bas, can you explain why you moved the intermediate surface into
> nsLayoutUtils.cpp? Was it because it's easier to know the size of the
> intermediate surface there?

It was to move it closer to the place where the 'problem existed', the operator being used really became a problem here, (i.e. if other functions would call this with a non-over operator it would have to be dealt with there as well) it seemed to make sense that this location should deal with exhibiting the expected behavior when a non-over operator is set.
Flags: needinfo?(bas)
Makes sense.

I suppose we could have the code in both places, and we'll still only get one intermediate surface. So I think that's what we should do here.
Address Markus's comments. I pass the mask mode to ImageRender directly and to SVG by function parameter.
Attachment #8726051 - Attachment is obsolete: true
Attachment #8729371 - Flags: review?(mstange)
Comment on attachment 8729371 [details] [diff] [review]
Part1 - Implement luminance mask mode

Review of attachment 8729371 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good except for the part that actually does the luminance operation. Once you've fixed that, please ask Bas to review that part. I'll give you r+ on the rest.
I'm redirecting this review to Bas so that he can comment on the PushGroup usage. I'm also interesting in what he thinks of the GFX_ARGB32_OFFSET stuff.

::: layout/base/nsCSSRendering.cpp
@@ +5202,5 @@
> +      RGBALuminanceOperation(map.mData, map.mStride, maskSize);
> +      maskData->Unmap();
> +    }
> +
> +    ctx->PopGroupAndBlend();

Does this work? I don't see how. You're not doing anything with maskData after you've done the luminance operation on it. And I'm not sure that PushGroupForBlendBack can even be used if you want to modify the pixels of the intermediate surface before the "blend back" operation.
Attachment #8729371 - Flags: review?(mstange) → review?(bas)
Comment on attachment 8729371 [details] [diff] [review]
Part1 - Implement luminance mask mode

Review of attachment 8729371 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +5202,5 @@
> +      RGBALuminanceOperation(map.mData, map.mStride, maskSize);
> +      maskData->Unmap();
> +    }
> +
> +    ctx->PopGroupAndBlend();

This will be a no-op on a bunch of backends, and work incorrectly on some others, you cannot modify the contents of a DrawTarget like this.
Attachment #8729371 - Flags: review?(bas) → review-
Fix the wrong operation of draw target.
Attachment #8729371 - Attachment is obsolete: true
Attachment #8730090 - Flags: review?(mstange)
Comment on attachment 8730090 [details] [diff] [review]
Part1 - Implement luminance mask mode

Review of attachment 8730090 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +5106,5 @@
> +  uint8_t *pixel = aData;
> +
> +  for (int32_t y = 0; y < aSize.height; y++) {
> +    for (int32_t x = 0; x < aSize.width; x++) {
> +      pixel[GFX_ARGB32_OFFSET_A] = (redFactor * pixel[GFX_ARGB32_OFFSET_R] +

As Markus mentioned, this seems like a poor way to do it, as far as I can see if we know the uint32_t way we look at this pixel, we'd be better off accessing it as a uint32_t and doing an or of the shifted components (in this case where r, g and b are 0 that should lead to a lot simpler code in general.
(In reply to Bas Schouten (:bas.schouten) from comment #18)
> Comment on attachment 8730090 [details] [diff] [review]
> Part1 - Implement luminance mask mode
> 
> Review of attachment 8730090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +5106,5 @@
> > +  uint8_t *pixel = aData;
> > +
> > +  for (int32_t y = 0; y < aSize.height; y++) {
> > +    for (int32_t x = 0; x < aSize.width; x++) {
> > +      pixel[GFX_ARGB32_OFFSET_A] = (redFactor * pixel[GFX_ARGB32_OFFSET_R] +
> 
> As Markus mentioned, this seems like a poor way to do it, as far as I can
> see if we know the uint32_t way we look at this pixel, we'd be better off
> accessing it as a uint32_t and doing an or of the shifted components (in
> this case where r, g and b are 0 that should lead to a lot simpler code in
> general.

Okay, I will try to use uint32_t to handle a pixel.
Attachment #8730090 - Flags: review?(mstange)
Addressed Bas's comments.
Attachment #8730090 - Attachment is obsolete: true
Attachment #8731900 - Flags: review?(bas)
Attachment #8731900 - Flags: review?(bas) → review+
Add reftest for mask-mode.
Attachment #8723359 - Attachment is obsolete: true
Attachment #8733902 - Flags: review?(cam)
Fix a typo.
Attachment #8733902 - Attachment is obsolete: true
Attachment #8733902 - Flags: review?(cam)
Attachment #8733915 - Flags: review?(cam)
Comment on attachment 8733915 [details] [diff] [review]
Part2 - Add reftest for mask mode.

Review of attachment 8733915 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/w3c-css/submitted/masking/mask-mode-a.html
@@ +6,5 @@
> +    <link rel="author" title="Ethan Lin" href="mailto:ethlin@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <link rel="help" href="https://www.w3.org/TR/css-masking-1/#propdef-mask-mode">
> +    <link rel="match" href="mask-mode-ref.html">
> +    <meta name="assert" content="Test checks that mask from vector-mask-image is correct with different mask mode.">

Maybe s/from vector-mask-image/an SVG image referenced by mask-image/?

@@ +20,5 @@
> +      }
> +
> +      div.auto {
> +        left: 10px;
> +        mask-mode: auto;

Update this to match-source.  (Since match-source is the initial value, and auto is invalid, this works as it is now, but we should use the right value.)

::: layout/reftests/w3c-css/submitted/masking/mask-mode-b.html
@@ +6,5 @@
> +    <link rel="author" title="Ethan Lin" href="mailto:ethlin@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <link rel="help" href="https://www.w3.org/TR/css-masking-1/#propdef-mask-mode">
> +    <link rel="match" href="mask-mode-ref.html">
> +    <meta name="assert" content="Test checks that mask from raster-mask-image is correct with different mask mode.">

Maybe s/from vector-mask-image/a PNG image referenced by mask-image/?

@@ +20,5 @@
> +      }
> +
> +      div.auto {
> +        left: 10px;
> +        mask-mode: auto;

Here too.

::: layout/reftests/w3c-css/submitted/masking/support/blue-luminance-100x50-transparent.svg
@@ +1,2 @@
> +<?xml version="1.0"?>
> +<svg xmlns="http://www.w3.org/2000/svg" version="1.1">

Nit: the version="1.1" has no effect so can be removed.

@@ +1,4 @@
> +<?xml version="1.0"?>
> +<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
> +  <rect x="0" y="0"  width="100%" height="50%" fill="blue" fill-opacity="0"/>
> +  <rect x="0" y="50" width="100%" height="50%" fill="RGB(238,238,255)" fill-opacity="1"/>

Did you mean for this to be y="50%" here, and in the other .svg files you're adding?
Attachment #8733915 - Flags: review?(cam) → review+
Comment on attachment 8733915 [details] [diff] [review]
Part2 - Add reftest for mask mode.

Review of attachment 8733915 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/w3c-css/submitted/masking/mask-mode-a.html
@@ +6,5 @@
> +    <link rel="author" title="Ethan Lin" href="mailto:ethlin@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <link rel="help" href="https://www.w3.org/TR/css-masking-1/#propdef-mask-mode">
> +    <link rel="match" href="mask-mode-ref.html">
> +    <meta name="assert" content="Test checks that mask from vector-mask-image is correct with different mask mode.">

Okay, will do.

@@ +20,5 @@
> +      }
> +
> +      div.auto {
> +        left: 10px;
> +        mask-mode: auto;

Okay.

::: layout/reftests/w3c-css/submitted/masking/mask-mode-b.html
@@ +6,5 @@
> +    <link rel="author" title="Ethan Lin" href="mailto:ethlin@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <link rel="help" href="https://www.w3.org/TR/css-masking-1/#propdef-mask-mode">
> +    <link rel="match" href="mask-mode-ref.html">
> +    <meta name="assert" content="Test checks that mask from raster-mask-image is correct with different mask mode.">

Okay.

@@ +20,5 @@
> +      }
> +
> +      div.auto {
> +        left: 10px;
> +        mask-mode: auto;

Okay, I will change it to match-source.

::: layout/reftests/w3c-css/submitted/masking/support/blue-luminance-100x50-transparent.svg
@@ +1,2 @@
> +<?xml version="1.0"?>
> +<svg xmlns="http://www.w3.org/2000/svg" version="1.1">

I'll remove it.

@@ +1,4 @@
> +<?xml version="1.0"?>
> +<svg xmlns="http://www.w3.org/2000/svg" version="1.1">
> +  <rect x="0" y="0"  width="100%" height="50%" fill="blue" fill-opacity="0"/>
> +  <rect x="0" y="50" width="100%" height="50%" fill="RGB(238,238,255)" fill-opacity="1"/>

I will change test images to a (0, 0, 100%, 100%) image since the tests just check if the mask-mode is correct.
Rebase and fix warning.
Attachment #8731900 - Attachment is obsolete: true
Addressed heycam's comments.
Attachment #8733915 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dad9f126f055
https://hg.mozilla.org/mozilla-central/rev/296179ddbd84
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Question for documentation:
The changes made here are still behind a compilation flag and everything will be shipped once bug 1224422 is fixed. Is that correct? Or will this require bug 1251161 to be fixed?

Sebastian
These are documented.
Depends on: 1407470
You need to log in before you can comment on or make changes to this bug.