1345388 - Add webrender support for AltFeedback

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

This bug tries to convert AltFeedback display item to WR draw command.

Comment 1

[Mason Chang [Inactive] [:mchang]]

I'll take this unless you have something close to working on this.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Hmm looks like we need a couple of things to support alt feedback:

1) Border renderer needs to figure out what we're going to do with sync decoded images.
2) Need paths for ellipses in some cases.
3) A bunch of bidi text processing.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

nsDisplayAltFeedback will not draw anything if the image is loading. We currently end up creating an empty blob image for each of these. If we have a basic implementation that just does nothing if we're not going to draw anything we'll already be better off.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

We'll hit this on page load quite a bit and not having it will negatively impact performance numbers. I think we need to raise its priority.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's re-triage.

Comment 7

[Marco Mucci [:MarcoM]]

Deprioritized to P3 during triage - moving to Reserve Backlog.

Comment 8

[Peter Chang[:pchang]]

Kevin, please help on this.

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

The basic implementation is almost done, I will create some tests to verify the result.

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: [WIP] Bug 1345388 - Add WR support for AltFeedback;

Review commit: https://reviewboard.mozilla.org/r/213060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213060/

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8942824 [details]
[WIP] Bug 1345388 - Add WR support for AltFeedback;

https://reviewboard.mozilla.org/r/213060/#review218738


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsImageFrame.cpp:1546
(Diff revision 1)
> +                                             mozilla::wr::DisplayListBuilder& aBuilder,
> +                                             mozilla::wr::IpcResourceUpdateQueue& aResources,
> +                                             const StackingContextHelper& aSc,
> +                                             mozilla::layers::WebRenderLayerManager* aManager,
> +                                             nsDisplayListBuilder* aDisplayListBuilder,
> +                                             nsPoint aPt, uint32_t aFlags)

Warning: The parameter 'apt' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

Comment 12

[Kevin Chen[:kechen] (UTC + 8)]

This is the WIP patch. I will merge DisplayAltFeedbackWithoutLayer and DisplayAltFeedback since they have similar code flow.

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

I'm not actively working on this.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Things seem to work without this.

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Factor out a function we will need

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Main patch

I just finished up Kevin's patch.

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9023521 [details] [diff] [review]
Main patch

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

::: layout/generic/nsImageFrame.cpp
@@ +1432,5 @@
>  
>      nsDisplayItemGenericImageGeometry::UpdateDrawResult(this, result);
>    }
>  
> +  virtual bool CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,

nit: remove virtual keyword, override is enough (according to style guide).

@@ +1439,5 @@
> +                                       mozilla::layers::WebRenderLayerManager* aManager,
> +                                       nsDisplayListBuilder* aDisplayListBuilder) override
> +  {
> +    if (!mFrame->IsImageFrame()) {
> +      MOZ_ASSERT(false);

Is there a reason to prefer MOZ_ASSERT(false) over MOZ_ASSERT_UNREACHABLE("Frame is not image frame!") or something? I see the former elsewhere but the latter should always be clearer in the logs, no?

@@ +1450,5 @@
> +       f->DisplayAltFeedbackWithoutLayer(this, aBuilder, aResources, aSc,
> +                                         aManager, aDisplayListBuilder,
> +                                         ToReferenceFrame(), flags);
> +
> +    return (result == ImgDrawResult::SUCCESS);

I think we should always return true here. We shouldn't need fallback anymore.

@@ +1632,5 @@
> +  // Display a recessed one pixel border
> +  nscoord borderEdgeWidth = nsPresContext::CSSPixelsToAppUnits(ALT_BORDER_WIDTH);
> +
> +  // if inner area is empty, then make it big enough for at least the icon
> +  if (inner.IsEmpty()){

nit: add space between ) and {

@@ +1676,5 @@
> +  LayoutDeviceRect bounds = LayoutDeviceRect::FromAppUnits(
> +        inner, PresContext()->AppUnitsPerDevPixel());
> +  wr::LayoutRect transformedRect = wr::ToRoundedLayoutRect(bounds);
> +  auto clipId = aBuilder.DefineClip(Nothing(), transformedRect);
> +  aBuilder.PushClip(clipId);

Is it possible to eliminate this extra clip? We should be able to use transformedRect as our clip in PushImage (needs signature change to provide it), PushBorder, and PushRect? I believe TextDrawTarget should apply the right clip already since we pass in inner as its bounds.

@@ +1728,5 @@
> +      result &=
> +        imgCon->GetImageContainerAtSize(aManager, decodeSize, svgContext,
> +                                        aFlags, getter_AddRefs(container));
> +      if (!container) {
> +        return ImgDrawResult::INCOMPLETE;

I think we should treat the lack of a container the same as NOT_READY.

@@ +1791,5 @@
> +    }
> +  }
> +
> +  // Draw text
> +  //RenderToContext(captureCtx, aDisplayListBuilder, true);

nit: Comment still relevant?

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9023521 [details] [diff] [review]
Main patch

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

::: layout/generic/nsImageFrame.cpp
@@ +1676,5 @@
> +  LayoutDeviceRect bounds = LayoutDeviceRect::FromAppUnits(
> +        inner, PresContext()->AppUnitsPerDevPixel());
> +  wr::LayoutRect transformedRect = wr::ToRoundedLayoutRect(bounds);
> +  auto clipId = aBuilder.DefineClip(Nothing(), transformedRect);
> +  aBuilder.PushClip(clipId);

Yeah, I'll upload another patch doing this on top of what we have.

@@ +1728,5 @@
> +      result &=
> +        imgCon->GetImageContainerAtSize(aManager, decodeSize, svgContext,
> +                                        aFlags, getter_AddRefs(container));
> +      if (!container) {
> +        return ImgDrawResult::INCOMPLETE;

You're right. I reworked this.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Save using an extra clip.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9026282 [details] [diff] [review]
Part 3. Save using an extra clip.

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

Nice :).

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: handle TextDrawTarget fallback

Changes to TextDrawTarget exposed that we didn't handle the case where it needed fallback, which is tricky to handle since we need to rollback a border, image, and text if the text fails (and the DisplayListBuilder only supports one level of save/restore).

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9029878 [details] [diff] [review]
handle TextDrawTarget fallback

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

::: layout/generic/nsImageFrame.cpp
@@ +1530,5 @@
> +  bool textDrawResult = true;
> +  class AutoSaveRestore {
> +  public:
> +    explicit AutoSaveRestore(mozilla::wr::DisplayListBuilder& aBuilder,
> +                                  bool *aTextDrawResult)

Use a reference since this is never null? I think you need to run clang-format on the diff too.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83b545fe5dd
Factor out CreateWebRenderCommandsForBorderWithStyleBorder so we can pass an nsStyleBorder of our choice. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/3878d1245a34
Draw the alt feedback of an image with WedRender so we don't have to use fallback. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f34ff46ab95
Save using one extra clip. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/deb35d6b4f83
Handle TextDrawTarget fallback. r=mattwoodrow

Comment 24

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/a83b545fe5dd
https://hg.mozilla.org/mozilla-central/rev/3878d1245a34
https://hg.mozilla.org/mozilla-central/rev/6f34ff46ab95
https://hg.mozilla.org/mozilla-central/rev/deb35d6b4f83

Comment 25

[Florin Strugariu [:Bebe]]

This caused a performance improvemnt for awsy

== Change summary for alert #18147 (as of Wed, 12 Dec 2018 03:27:58 GMT) ==

Improvements:

  7%  Heap Unclassified opt stylo     58,366,474.74 -> 54,365,681.66

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18147

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Florin Strugariu [:Bebe] from comment #25)
> This caused a performance improvemnt for awsy
> 
> == Change summary for alert #18147 (as of Wed, 12 Dec 2018 03:27:58 GMT) ==
> 
> Improvements:
> 
>   7%  Heap Unclassified opt stylo     58,366,474.74 -> 54,365,681.66
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=18147

Unless that test is running with webrender enabled then I think some other bug must have caused this improvement.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

The alert link doesn't work for me, but looking from TreeHerder, the improvement was in fact on the windows-qr AWSY job, so yes, it has webrender enabled. Yay improvement!

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

I assume it is:
https://treeherder.mozilla.org/perf.html#/alerts?id=18157

odd why that alert doesn't exist- maybe it was all moved or there is some other regression.

:igoldan, is this something you can figure out with your treeherder/perfherder development?  if not, we could ask the expert :sclements to help out.

Comment 29

[Ionuț Goldan [:igoldan]]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #28)
> I assume it is:
> https://treeherder.mozilla.org/perf.html#/alerts?id=18157
> 
> odd why that alert doesn't exist- maybe it was all moved or there is some
> other regression.
> 
> :igoldan, is this something you can figure out with your
> treeherder/perfherder development?  if not, we could ask the expert
> :sclements to help out.

I am able to see the alert now. Though I admit at first I received a couple of 404 errors.
I've attached the proof.

Comment 30

[Ionuț Goldan [:igoldan]]

Attachment: able-to-see-alert.png