Closed Bug 1207489 Opened 9 years ago Closed 8 years ago

Precompute mix-blend-modes contained in stacking contexts

Categories

(Core :: Layout, defect)

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 2 obsolete files)

Mix blend modes have a rather annoying property that they effect their parent stacking context, but the parent stacking context doesn't know which (if any) mix-blend-modes it contains until it has traversed all of its children.

This poses a problem in at least two cases. First, for APZ, we don't know whether scroll frames will be flattened until we've visited the entire frame tree, and that makes hoisting scroll info items very complicated.

Second, we want to create and cache ContainerLayers during display list construction (bug 1205129). The current mix-blend-mode behavior means we could cache a ContainerLayer for the widget layer manager, but then have a sibling frame introduce a mix-blend-mode. The parent stacking context will need to flatten everything and we'd have to find all cached child ContainerLayers and recreate them.

That sounds horrible, so I'd like to kill two birds with one stone and make sure mix-blend-modes are pre-computed on their parent stacking context.
There were a few approaches that came up when I asked around about this:
 (1) Always use BasicLayers for mix-blend-mode.
 (2) Require full mix-blend-mode in all four compositors.
 (3) Perform some level of caching on parent stacking contexts.

(1) seems like a step back, and (2) is a lot of work that seems much less important than what we actually want to achieve in bug 1205129. So I went with caching.

The approach here detects when a mix-blend-mode is introduced on a frame, finds its parent stacking context, and adds a frame property that accumulates the set of mix-blend-modes in its descendants. If an existing mix-blend-mode is removed or changed on a frame, or the stacking context hierarchy changes, we set a dirty bit on the frame property.

When we go to paint a stacking context with such a frame property, if it has the dirty bit set, we recompute the full mix-blend-mode set via a frame tree walk. The resulting mix-blend-mode set may be wider than the one that gets painted, but it must be a strict superset of the painted set.

This mechanism is a little complicated so I'm not 100% sold on it, but I don't have any other ideas yet.
Attachment #8664700 - Flags: feedback?(roc)
Can you estimate how hard #2 actually would be?
I agree it's less important than what we want to do in bug 1205129. But if it reduces complexity, maybe it's worth it! And I don't know why it would be much work other than having to write a bunch of boilerplate code.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> I agree it's less important than what we want to do in bug 1205129. But if
> it reduces complexity, maybe it's worth it! And I don't know why it would be
> much work other than having to write a bunch of boilerplate code.

It's really the boilerplate code, I guess. Especially on d3d9/d3d11 where we don't have a shader generator. But I just realized, even doing this, or even using BasicLayers, wouldn't help: we still need to know a priori which stacking contexts will have mix-blend-modes, and 99% of the patch here is doing just that. The code dedicated to tracking *which* modes are used is just a few extra lines.

If we can do the tracking even simpler though, that'd be great.
Flags: needinfo?(roc)
Flags: needinfo?(dvander)
Comment on attachment 8664693 [details] [diff] [review]
part 1, helper functions for finding stacking contexts

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

I do not understand how the commit message is related to the content of this patch.
Attachment #8664693 - Flags: review?(roc) → review-
Comment on attachment 8664700 [details] [diff] [review]
part 2, cache mix-blend-modes on stacking contexts

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

::: layout/generic/MixBlendModesCache.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace layout {
> +
> +class MixBlendModesCache

Give this class an overall description --- what it's caching, why, when the cache should be invalidated.

@@ +32,5 @@
> +  static void RemoveMixBlendMode(nsIFrame* aFrame);
> +  static void ChangeMixBlendMode(nsIFrame* aFrame, int aNewMixBlendMode);
> +  static bool HasCache(nsIFrame* aFrame);
> +  static void Reparent(nsIFrame* aFrame);
> +  static void ChangeStackingContext(nsIFrame* aFrame);

Document these methods!
Attachment #8664700 - Flags: feedback?(roc) → feedback-
(In reply to David Anderson [:dvander] from comment #5)
> It's really the boilerplate code, I guess. Especially on d3d9/d3d11 where we
> don't have a shader generator. But I just realized, even doing this, or even
> using BasicLayers, wouldn't help: we still need to know a priori which
> stacking contexts will have mix-blend-modes, and 99% of the patch here is
> doing just that. The code dedicated to tracking *which* modes are used is
> just a few extra lines.

Yeah OK.
Flags: needinfo?(roc)
Attached the wrong patch. Sorry about that.
Attachment #8664693 - Attachment is obsolete: true
Attachment #8665263 - Flags: review?(roc)
Added a bunch of comments.

I had convinced myself earlier that we needed this, even if we required all compositors to support all mix-blend-modes. But now I can't see why that's true, so I might be wrong. We would definitely need something like this if we went with (1) and only used BasicLayers though.
Attachment #8664700 - Attachment is obsolete: true
Attachment #8665287 - Flags: feedback?(roc)
(In reply to David Anderson [:dvander] from comment #10)
> I had convinced myself earlier that we needed this, even if we required all
> compositors to support all mix-blend-modes. But now I can't see why that's
> true, so I might be wrong.

Yeah, I don't know why we would need this if compositors support all mix-blend-modes. I think we would not. I took your word for it too easily :-).

So then we're back to the earlier question: how much work is it to support all blend modes in all compositors, really?
Comment on attachment 8665287 [details] [diff] [review]
part 2, cache mix-blend-modes on stacking contexts

Talked about this on IRC. We decided to require full-mix-blend mode support in all compositors, and if it looks too ... something (dangerous? difficult?) to fallback to this. I'll leave this bug open for now and file a new one.
Attachment #8665287 - Flags: feedback?(roc)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: