Bug 110505 - Expose a list of all reasons that qualify a RenderLayer to be composited
Summary: Expose a list of all reasons that qualify a RenderLayer to be composited
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 110559
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-21 12:59 PST by Antoine Quint
Modified: 2013-02-22 13:47 PST (History)
8 users (show)

See Also:


Attachments
Patch (12.54 KB, patch)
2013-02-21 13:03 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (12.64 KB, patch)
2013-02-22 07:12 PST, Antoine Quint
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2013-02-22 09:16 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2013-02-21 12:59:56 PST
Expose a list of all reasons that qualify a RenderLayer to be compositedd
Comment 1 Antoine Quint 2013-02-21 13:03:03 PST
Created attachment 189582 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2013-02-21 13:06:28 PST
<rdar://problem/13266329>
Comment 3 Eric Seidel (no email) 2013-02-21 13:07:30 PST
Sounds very useful to the inspector. :)
Comment 4 WebKit Review Bot 2013-02-21 13:10:44 PST
Comment on attachment 189582 [details]
Patch

Attachment 189582 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16697265
Comment 5 Antoine Quint 2013-02-21 13:14:00 PST
(In reply to comment #3)
> Sounds very useful to the inspector. :)

That's what I'm thinking too, and that's why I'm adding it.
Comment 6 Dean Jackson 2013-02-21 13:23:04 PST
Comment on attachment 189582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189582&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
> +    if (inCompositingMode() && layer->isRootLayer())
> +        reason |= CompositingReasonRoot;

As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
> +    if (reason & CompositingReasonRoot)
>          return "root";
> -

So what happens if you have this reason and another one. You'll never report "root" because you exit early above.

This makes me wonder why you're using a bitmask over just a regular enum?
Comment 7 Antoine Quint 2013-02-21 13:30:53 PST
(In reply to comment #6)
> (From update of attachment 189582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189582&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
> > +    if (inCompositingMode() && layer->isRootLayer())
> > +        reason |= CompositingReasonRoot;
> 
> As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.

You're looking at the wrong method, this is only related to logging. There can definitely be several reasons as reported in the new version of reasonsForCompositing(). For instance, you can have a <video> element with a 3D transform applied.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
> > +    if (reason & CompositingReasonRoot)
> >          return "root";
> > -
> 
> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
> 
> This makes me wonder why you're using a bitmask over just a regular enum?

The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.
Comment 8 Dean Jackson 2013-02-21 13:35:53 PST
Comment on attachment 189582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189582&action=review

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1779
>>> +        reason |= CompositingReasonRoot;
>> 
>> As far as I can tell (sorry, I'm terrible at reading diffs), this is the only case where you'd have more than one reason for compositing. See below.
> 
> You're looking at the wrong method, this is only related to logging. There can definitely be several reasons as reported in the new version of reasonsForCompositing(). For instance, you can have a <video> element with a 3D transform applied.

Yep - I screwed up.

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
>>> -
>> 
>> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
>> 
>> This makes me wonder why you're using a bitmask over just a regular enum?
> 
> The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.

So I still think you should fix this method to return multiple reasons. If reason has both CompositingReasonVideo and CompositingReason3DTransform then the string should have "3d transform + video" or something.
Comment 9 Simon Fraser (smfr) 2013-02-21 13:37:47 PST
CA has no support for non-separable modes, BTW.
Comment 10 Simon Fraser (smfr) 2013-02-21 13:39:16 PST
(In reply to comment #9)
> CA has no support for non-separable modes, BTW.
Sorry, wrong bug.

What I want to say here is that I'd rather not have the expense of running all the "requires compositing for" in order to compute those flags (tho if we only do it when the inspector needs it, that would be OK).
Comment 11 Antoine Quint 2013-02-21 13:41:01 PST
(In reply to comment #10)
> (In reply to comment #9)
> > CA has no support for non-separable modes, BTW.
> Sorry, wrong bug.
> 
> What I want to say here is that I'd rather not have the expense of running all the "requires compositing for" in order to compute those flags (tho if we only do it when the inspector needs it, that would be OK).

This would only be for the inspector and logging scenarios. In the case of the inspector, I intend on exposing a per-layer call into WebCore to gather this information as needed from the front-ned, so it would only be done as necessary and not upfront for all layers pushed to the front-end.
Comment 12 Simon Fraser (smfr) 2013-02-21 13:42:58 PST
Comment on attachment 189582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189582&action=review

>>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1782
>>>> -
>>> 
>>> So what happens if you have this reason and another one. You'll never report "root" because you exit early above.
>>> 
>>> This makes me wonder why you're using a bitmask over just a regular enum?
>> 
>> The bit mask will be used for the Web Inspector to be able to several reasons a RenderLayer was composited. The logReasonsForCompsiting() method replaces the old reasonsForCompositing() and its behavior which only reports the first matching reason.
> 
> So I still think you should fix this method to return multiple reasons. If reason has both CompositingReasonVideo and CompositingReason3DTransform then the string should have "3d transform + video" or something.

It's fine for this logging to not change.

> Source/WebCore/rendering/RenderLayerCompositor.h:85
> +typedef unsigned CompositingReason;

CompositingReasons (plural), since this is a bitmask.

> Source/WebCore/rendering/RenderLayerCompositor.h:269
> +    CompositingReason reasonForCompositing(const RenderLayer*);

This should be const.
Comment 13 Antoine Quint 2013-02-21 22:36:40 PST
http://trac.webkit.org/changeset/143691
Comment 14 WebKit Review Bot 2013-02-21 23:18:12 PST
Re-opened since this is blocked by bug 110559
Comment 15 Antoine Quint 2013-02-22 07:12:21 PST
Created attachment 189769 [details]
Patch
Comment 16 WebKit Review Bot 2013-02-22 08:32:45 PST
Comment on attachment 189769 [details]
Patch

Attachment 189769 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16716012
Comment 17 Antoine Quint 2013-02-22 09:16:19 PST
Created attachment 189781 [details]
Patch
Comment 18 WebKit Review Bot 2013-02-22 13:36:09 PST
Comment on attachment 189781 [details]
Patch

Rejecting attachment 189781 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189781, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Full output: http://queues.webkit.org/results/16709254
Comment 19 WebKit Review Bot 2013-02-22 13:47:41 PST
Comment on attachment 189781 [details]
Patch

Clearing flags on attachment: 189781

Committed r143787: <http://trac.webkit.org/changeset/143787>
Comment 20 WebKit Review Bot 2013-02-22 13:47:46 PST
All reviewed patches have been landed.  Closing bug.