Closed Bug 430906 Opened 16 years ago Closed 16 years ago

Add moz-opaque attribute on canvas

Categories

(Core :: Graphics: Canvas2D, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

We can get a significant performance win if the canvas can know ahead of time that translucency is not important on the canvas; this is, the entire canvas will be fully painted opaquely.  To get that information, I've like to add a moz-hint attribute to canvas that can take a comma-separated list of hints.  The one hint defined here is 'opaque'.

In the future, I anticipate an additional hint for 'always on top' behaviour -- that no element will ever be placed on top of the canvas.  (This is so that things like the 3D canvas can allocate a widget in that case and draw directly to the screen.)

When the hint is changed, the canvas is recreated in the same way as when the size is changed.

Don't care about this in 1.9; would like it in 1.9.1.
Attachment #317817 - Flags: review?(roc)
I'm not sure we want an "always-on-top" hint. Seems like that would make it impossible for chrome to guarantee it's drawing on top of content in security-relevant situations.

Would it be more author-friendly to just offer an "opaque" DOM attribute?
Attached patch change attribute to moz-opaque (obsolete) — Splinter Review
Updated to get rid of moz-hint, and instead have an explicit moz-opaque attribute.  I kept the internals mostly the same, though maybe I should rename "Hints" to "Flags" since they're a stronger promise than just a hint?
Assignee: nobody → vladimir
Attachment #317817 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #318731 - Flags: review?(roc)
Attachment #317817 - Flags: review?(roc)
+    gfxContext::GraphicsOperator op = ctx->CurrentOperator();
+    if (HasOpaqueHint())
+        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);

Do we really need this? Shouldn't cairo or lower do this since we have an RGB source surface?

I think instead of "hints" we should just have explicit API for each feature you want to add. It's not really going to save much to have multiple flags bundled into a word and the code's easier to read with individual methods.

+  } else if (aAttribute == nsGkAtoms::moz_opaque)
+  {
+    NS_UpdateHint(retval, NS_STYLE_HINT_VISUAL);

Does this have any real effect? Seems to me the surface type doesn't change so changing the attribute doesn't actually work. It probably should clear the canvas like a resize would.
(In reply to comment #3)
> +    gfxContext::GraphicsOperator op = ctx->CurrentOperator();
> +    if (HasOpaqueHint())
> +        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> Do we really need this? Shouldn't cairo or lower do this since we have an RGB
> source surface?
> 
> I think instead of "hints" we should just have explicit API for each feature
> you want to add. It's not really going to save much to have multiple flags
> bundled into a word and the code's easier to read with individual methods.

My worry here is that any other canvas backend implementations (e.g. canvas3d) would then have to implement the new methods, instead of just being able to ignore the flags they don't understand.  But then again, since they're not hints any more and have to be honored, maybe that's not such a bad thing.

> +  } else if (aAttribute == nsGkAtoms::moz_opaque)
> +  {
> +    NS_UpdateHint(retval, NS_STYLE_HINT_VISUAL);
> 
> Does this have any real effect? Seems to me the surface type doesn't change so
> changing the attribute doesn't actually work. It probably should clear the
> canvas like a resize would.

It does, no?  That's just the attribtue change hint, in SetAttr() UpdateContext() is called.
(In reply to comment #4)
> My worry here is that any other canvas backend implementations (e.g. canvas3d)
> would then have to implement the new methods, instead of just being able to
> ignore the flags they don't understand.  But then again, since they're not
> hints any more and have to be honored, maybe that's not such a bad thing.

Yeah, for actual ignorable hints that would make sense, but right now the number of those we have is zero :-).

> It does, no?  That's just the attribtue change hint, in SetAttr()
> UpdateContext() is called.

Sorry, of course you're right.
Attached patch updated backend API (obsolete) — Splinter Review
Ok, updated again; added a SetIsOpaque() instead of SetHints.  I left the OPERATOR_SOURCE bit in here -- in theory things should work as you suggest, but I know there were some bugs where such an optimization was missed.  Would rather ensure that it happens for now.
Attachment #318731 - Attachment is obsolete: true
Attachment #319666 - Flags: review?(roc)
Attachment #318731 - Flags: review?(roc)
+    if (mOpaque)
+        ctx->SetOperator(op);

This might as well be unconditional.

+  if (GetParsedAttr(nsGkAtoms::moz_opaque))
+    return PR_TRUE;
+
+  return PR_FALSE;

Does this do the right thing for mozOpaque="false"?
(In reply to comment #7)
> +    if (mOpaque)
> +        ctx->SetOperator(op);
> 
> This might as well be unconditional.

True.

> +  if (GetParsedAttr(nsGkAtoms::moz_opaque))
> +    return PR_TRUE;
> +
> +  return PR_FALSE;
> 
> Does this do the right thing for mozOpaque="false"?

Depends what you mean by the right thing.. if the moz-opaque attribute is present, then like other boolean attributes it means "true" :)

(In reply to comment #9)
> Why not use HasAttr then?

I'll switch it; I wasn't thinking of that when I changed the code from using a parsed string.
Summary: Add hint attribute on canvas; define 'opaque' hint → Add moz-opaque attribute on canvas
Updated with HasAttr, marking roc's verbal r+sr with that change.
Attachment #319666 - Attachment is obsolete: true
Attachment #320289 - Flags: superreview+
Attachment #320289 - Flags: review+
Attachment #319666 - Flags: review?(roc)
Blocks: 436061
(In reply to comment #11)
> Created an attachment (id=320289) [details]
> updated with HasAttr
> 

mOpaque - uninitialised value,
mOpaque need to be initialized properly in constructor.
Thanks, updated -- it's always initialized before it's used though, but I've added initialization in the constructor.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Depends on: 878149
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: