Closed Bug 1455422 (async-blob) Opened 2 years ago Closed Last year

Make blob rendering not block the render thread.

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jrmuizel, Assigned: nical)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

I recall Nical thinking this would be easy.
Assignee: nobody → nical.bugzilla
Priority: -- → P1
Alias: async-blob
Blocks: 1454810
This is the Gecko side of things, most of the work being in https://github.com/servo/webrender/pull/2785

It doesn't currently work: WebRender sometimes to requests tiles outside of the area generated by intersecting the viewport and the image display item so I guess something is wrong in the way I am trying to generate this visible rect. Might be that I can't assume that the reference frame of the first display item is enough, might be something else, not sure yet.
Another version that uses the wrapping display item's paint rect thanks to mstange's help.
Oh boy what did I just do?
I tried to use the intersection of the bounds and paint rect of the blob image display items to apply a local clip on the display item. This helps a lot with preventing webrender from requesting tiles that are not in Gecko's computed visible area, but it appears to cause some issues. The main one I saw was the animated map at the bottom of http://digitalocean.com/ rendering very differently.
Attachment #8989414 - Attachment is obsolete: true
Attachment #8992648 - Flags: review?(jmuizelaar)
Attachment #8992648 - Flags: review?(bugmail)
Attachment #8992648 - Flags: review?(a.beingessner)
Comment on attachment 8992648 [details] [diff] [review]
Implement the new async blob handler/rasterizer API.

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

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +69,5 @@
> +                        const LayoutDeviceRect& aImageRect,
> +                        const LayoutDeviceRect& aPaintRect)
> +{
> +  LayoutDeviceRect visibleRect = aImageRect.Intersect(aPaintRect);
> +  // Send the visible rect in normalized coordinates.

I'd prefer to not use normalized coordinates and instead use integer device pixels
Attachment #8992648 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8992648 [details] [diff] [review]
Implement the new async blob handler/rasterizer API.

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

There's too many kinds of visible rect for me to be certain that part is right, so mostly just reviewing the piping and stuff assuming that's right.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +1164,5 @@
>    group.mGroupBounds = groupBounds;
> +  group.mPaintRect = LayoutDeviceRect::FromAppUnits(
> +    aWrappingItem->GetPaintRect(),
> +    appUnitsPerDevPixel
> +  );

should you also pipe this value through at https://searchfox.org/mozilla-central/source/gfx/layers/wr/WebRenderCommandBuilder.cpp#979

?

::: gfx/webrender_bindings/src/moz2d_renderer.rs
@@ +432,5 @@
> +        unsafe { DeleteFontData(font); }
> +    }
> +
> +    fn delete_font_instance(&mut self, _key: FontInstanceKey) {
> +    }

can you explain (comment?) why this is blank?
Attachment #8992648 - Flags: review?(jmuizelaar)
Attachment #8992648 - Flags: review?(bugmail)
Attachment #8992648 - Flags: review?(a.beingessner)
Attachment #8992648 - Flags: review+
Comment on attachment 8992648 [details] [diff] [review]
Implement the new async blob handler/rasterizer API.

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

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +1164,5 @@
>    group.mGroupBounds = groupBounds;
> +  group.mPaintRect = LayoutDeviceRect::FromAppUnits(
> +    aWrappingItem->GetPaintRect(),
> +    appUnitsPerDevPixel
> +  );

Yeah I think so.

::: gfx/webrender_bindings/src/moz2d_renderer.rs
@@ +432,5 @@
> +        unsafe { DeleteFontData(font); }
> +    }
> +
> +    fn delete_font_instance(&mut self, _key: FontInstanceKey) {
> +    }

I can't explain it but this function already existed and was already blank before this patch.
Comment on attachment 8992648 [details] [diff] [review]
Implement the new async blob handler/rasterizer API.

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

Looks like some flags got reset. Restoring them
Attachment #8992648 - Flags: review?(jmuizelaar)
Attachment #8992648 - Flags: review?(bugmail)
Attachment #8992648 - Flags: review+
Same patch with Alexi's comment addressed and a proper commit title.
Attachment #8992648 - Attachment is obsolete: true
Attachment #8993009 - Attachment is patch: true
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80efe717b563
Implement the new blob image rasterization hooks. r=kats, gankro
Depends on: 1477400
No longer depends on: 1477400
https://hg.mozilla.org/mozilla-central/rev/80efe717b563
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1478084
Duplicate of this bug: 1425510
You need to log in before you can comment on or make changes to this bug.