Add support for reftests in webrender

RESOLVED FIXED in mozilla54

Status

()

Core
Graphics: WebRender
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Add support for reftests with the webrender branch. Depends on updating webrender to https://github.com/servo/webrender/pull/560.
(Assignee)

Comment 1

2 years ago
Created attachment 8811452 [details] [diff] [review]
Add support for reftests in webrender

If we have a EndTransactionWithTarget, we'll send a sync request to the parent side to do a readback of the pixels. This ends up as a readback via glReadPixels in bindings.rs. The data gets copied back out over IPC and drawn to the requested target.
Attachment #8811452 - Flags: review?(mstange)
(Assignee)

Comment 2

2 years ago
Also, I'm going to look at replicating how reftests are done in Servo, which is it uses an Epoch to determine when a frame has finished painting and waits for that. We don't really do anything useful with Epoch right now, but I hope to clean up some of this stuff once I take a look at that.
Comment on attachment 8811452 [details] [diff] [review]
Add support for reftests in webrender

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

This probably conflicts a lot with https://github.com/jrmuizel/gecko-dev/pull/53 .

The duplicated code in bindings.rs is unfortunate and the order of operations at the end of it looks strange to me, but since this is all temporary anyway, go ahead.

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +268,5 @@
> +  Rect dst(bounds.x, bounds.y, bounds.width, bounds.height);
> +  Rect src(0, 0, bounds.width, bounds.height);
> +
> +  // The data we get from webrender is upside down. So flip and translate up so the image is rightside up.
> +  SurfacePattern pattern(surf, ExtendMode::CLAMP, Matrix::Scaling(1.0, -1.0).PostTranslate(0.0, bounds.height));

Setting the matrix on the DrawTarget and using DrawSurface would probably be faster. Doesn't need to be fixed now though.

::: gfx/webrender/src/bindings.rs
@@ +214,5 @@
>  impl webrender_traits::RenderNotifier for Notifier {
>      fn new_frame_ready(&mut self) {
> +        let &(ref lock, ref cvar) = &*self.render_thread_notifier;
> +        let mut finished = lock.lock().unwrap();
> +        *finished = true;

I won't pretend to know what's going on here.
Attachment #8811452 - Flags: review?(mstange) → review+
Comment on attachment 8811452 [details] [diff] [review]
Add support for reftests in webrender

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

::: gfx/layers/wr/WebRenderLayerManager.h
@@ +130,5 @@
> +  // When we're doing a transaction in order to draw to a non-default
> +  // target, the layers transaction is only performed in order to send
> +  // a PLayers:Update.  We save the original non-default target to
> +  // mShadowTarget, and then perform the transaction using
> +  // mDummyTarget as the render target.  After the transaction ends,

I don't see mDummyTarget in this patch. Is this comment still accurate?

@@ +134,5 @@
> +  // mDummyTarget as the render target.  After the transaction ends,
> +  // we send a message to our remote side to capture the actual pixels
> +  // being drawn to the default target, and then copy those pixels
> +  // back to mShadowTarget.
> +  RefPtr<gfxContext> mShadowTarget;

Why not call this just mTarget?
(In reply to Mason Chang [:mchang] from comment #0)
> Add support for reftests with the webrender branch. Depends on updating
> webrender to https://github.com/servo/webrender/pull/560.

Presumably we'll get this update as part of bug 1318440.
Blocks: 1311790
Depends on: 1318440
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: 21 Branch → 53 Branch

Comment 6

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/f5bea7e99fdc
Add support for reftests in webrender. r=mstange
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

2 years ago
> ::: gfx/webrender/src/bindings.rs
> @@ +214,5 @@
> >  impl webrender_traits::RenderNotifier for Notifier {
> >      fn new_frame_ready(&mut self) {
> > +        let &(ref lock, ref cvar) = &*self.render_thread_notifier;
> > +        let mut finished = lock.lock().unwrap();
> > +        *finished = true;
> 
> I won't pretend to know what's going on here.

I cleaned up a lot of the horribleness :). Just have the flush notifier now.

Comment 8

2 years ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/9af4655d0edc
Follow up clean up of WebrenderBridgeChild

Comment 9

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/0fc195181db2
Minor cosmetic cleanup. r=mchang?
(Assignee)

Updated

2 years ago
Blocks: 1319170
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.