Add webrender support for AltFeedback

RESOLVED FIXED in Firefox 66

Status

()

P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: jerry, Assigned: tnikkel)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected, firefox66 fixed)

Details

(Whiteboard: [wr-reserve])

Attachments

(5 attachments, 1 obsolete attachment)

This bug tries to convert AltFeedback display item to WR draw command.
I'll take this unless you have something close to working on this.
Assignee: hshih → 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.
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
status-firefox56: --- → unaffected
status-firefox57: --- → unaffected
Assignee: mchang → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Target Milestone: mozilla57 → ---
Duplicate of this bug: 1408136
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.
Priority: P2 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
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.
Flags: needinfo?(milan)
Let's re-triage.
Flags: needinfo?(milan)
Priority: P3 → P2
Whiteboard: [wr-reserve] → [wr-mvp] [triage]
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Assignee: nobody → kechen
Status: NEW → ASSIGNED
Priority: P2 → P1
Deprioritized to P3 during triage - moving to Reserve Backlog.
Assignee: kechen → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
Assignee: nobody → howareyou322
Kevin, please help on this.
Assignee: howareyou322 → kechen
Comment hidden (spam)
Comment hidden (mozreview-request)
Comment hidden (spam)
This is the WIP patch. I will merge DisplayAltFeedbackWithoutLayer and DisplayAltFeedback since they have similar code flow.
I'm not actively working on this.
Assignee: kev155266 → nobody
Assignee: nobody → aosmond
Blocks: 1386669
No longer blocks: 1386665
Priority: P1 → P2
Things seem to work without this.
Blocks: 1386674
No longer blocks: 1386669
(Assignee)

Comment 15

4 months ago
Assignee: aosmond → tnikkel
Attachment #8942824 - Attachment is obsolete: true
Attachment #9023520 - Flags: review?(aosmond)
(Assignee)

Comment 16

4 months ago
Posted patch Main patchSplinter Review
I just finished up Kevin's patch.
Attachment #9023521 - Flags: review?(aosmond)
Attachment #9023520 - Flags: review?(aosmond) → review+
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?
Attachment #9023521 - Flags: review?(aosmond) → review+
(Assignee)

Comment 18

4 months ago
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.
(Assignee)

Comment 19

4 months ago
Attachment #9026282 - Flags: review?(aosmond)
Comment on attachment 9026282 [details] [diff] [review]
Part 3. Save using an extra clip.

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

Nice :).
Attachment #9026282 - Flags: review?(aosmond) → review+
(Assignee)

Comment 21

4 months ago
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).
Attachment #9029878 - Flags: review?(matt.woodrow)
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.
Attachment #9029878 - Flags: review?(matt.woodrow) → review+

Comment 23

3 months ago
Pushed by tnikkel@gmail.com:
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

3 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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
(Assignee)

Comment 26

3 months ago
(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.
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!
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.
Flags: needinfo?(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.
Flags: needinfo?(igoldan)
You need to log in before you can comment on or make changes to this bug.