873944 - Black flashing on resize on OS X with OMTC

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Markus Stange [:mstange]]

What's the plan here? Enforced sync recompositions during resize-induced drawRect?

Comment 2

[Matt Woodrow (:mattwoodrow)]

Yes, exactly.

We'll also be stopping all the nsIWidget::Invalidate/setNeedsDisplayInRect calls when we have OMTC, so drawRect should only be called when the OS decides we need it (i.e focus change and resize).

Comment 3

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Yes, exactly.

OK, great.

> We'll also be stopping all the nsIWidget::Invalidate/setNeedsDisplayInRect
> calls when we have OMTC, so drawRect should only be called when the OS
> decides we need it (i.e focus change and resize).

Interesting. Bug 676241 part 5 will also make setNeedsDisplayInRect not cause drawRect for OMTC any more, but stopping the Invalidate calls at a higher level sounds good.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't invalidate the widget and do a synchronous composite

Almost works, just a few intermittent reftest failures on 10.6/7

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't invalidate the widget and do a synchronous composite

This seems to work, and only hits the new assert during secondary layer manager painting.

Because this isn't OMTC, we allow the invalidation, but don't flush it to the widget until we've switched back to the primary layer manager.

This seems like a real bug, and will be fixed by bug 676241.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 752635 [details] [diff] [review]
Don't invalidate the widget and do a synchronous composite

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

Please split the patch into a patch that removes the invalidations, and anothe rpatch that does the sync composite stuff.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Add FlushRendering api to compositor

This synchronously waits for the compositor to complete the in-progress or scheduled composite before returning.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove unnecessary callers of nsView::Invalidate

Since all invalidation should go through DLBI now, these callers shouldn't be doing anything useful. I hope :)

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't invalidate the widget for OTMC

When we have OMTC, we skip sending invalidations to the view/widget and instead fire DidPaint immediately.

If we do end up getting a widget paint event (which must be from the OS initiating it), then we should synchronously wait for the compositor to draw.

Comment 10

[Nick Cameron [:nrc]]

Comment on attachment 755157 [details] [diff] [review]
Add FlushRendering api to compositor

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

::: gfx/layers/Layers.h
@@ +467,5 @@
>    virtual void SetIsFirstPaint() {}
>  
> +  /**
> +   * Make sure that the previous transaction has been entirely
> +   * completed.

add a note about this potentially making a sync call and therefore potentially blocking for 10,000 years.

::: gfx/layers/client/ClientLayerManager.h
@@ +51,5 @@
>    virtual already_AddRefed<ImageLayer> CreateImageLayer();
>    virtual already_AddRefed<CanvasLayer> CreateCanvasLayer();
>    virtual already_AddRefed<ColorLayer> CreateColorLayer();
>    virtual already_AddRefed<RefLayer> CreateRefLayer();
> +  

whitespace

::: gfx/layers/ipc/CompositorParent.cpp
@@ +757,5 @@
>    virtual bool RecvResume() MOZ_OVERRIDE { return true; }
>    virtual bool RecvMakeSnapshot(const SurfaceDescriptor& aInSnapshot,
>                                  SurfaceDescriptor* aOutSnapshot)
>    { return true; }
> +  virtual bool RecvFlushRendering() MOZ_OVERRIDE { return true; }

Why do you do this rather than using the CompositorParent impl?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755158 [details] [diff] [review]
Remove unnecessary callers of nsView::Invalidate

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

Hurrah!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755159 [details] [diff] [review]
Don't invalidate the widget for OTMC

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

Let's have a method bool LayerManager::NeedsWidgetInvalidation() and call that instead of this little expression you've sprinkled around.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't invalidate the widget for OTMC v2

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 761888 [details] [diff] [review]
Don't invalidate the widget for OTMC v2

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

::: view/src/nsViewManager.cpp
@@ +334,5 @@
>        }
>  #endif
> +      uint32_t paintFlags = nsIPresShell::PAINT_COMPOSITE;
> +      LayerManager *manager = widget->GetLayerManager();
> +      if (!manager->NeedsWidgetInvalidation()) {

Drop ! and switch the order of clauses instead

Comment 15

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bcd1eada79
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75f6184008a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b87ce69f43f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7bcd1eada79
https://hg.mozilla.org/mozilla-central/rev/b75f6184008a
https://hg.mozilla.org/mozilla-central/rev/0b87ce69f43f