Closed Bug 873944 Opened 7 years ago Closed 6 years ago

Black flashing on resize on OS X with OMTC

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jrmuizel, Assigned: mattwoodrow)

References

Details

Attachments

(3 files, 3 obsolete files)

No description provided.
What's the plan here? Enforced sync recompositions during resize-induced drawRect?
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).
(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.
Almost works, just a few intermittent reftest failures on 10.6/7
Blocks: 756601
Depends on: 676241
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.
Attachment #752635 - Flags: review?(roc)
Attachment #752091 - Attachment is obsolete: true
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.
Blocks: 875335
This synchronously waits for the compositor to complete the in-progress or scheduled composite before returning.
Attachment #755157 - Flags: review?(ncameron)
Since all invalidation should go through DLBI now, these callers shouldn't be doing anything useful. I hope :)
Attachment #752635 - Attachment is obsolete: true
Attachment #752635 - Flags: review?(roc)
Attachment #755158 - Flags: review?(roc)
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.
Attachment #755159 - Flags: review?(roc)
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?
Attachment #755157 - Flags: review?(ncameron) → review+
Comment on attachment 755158 [details] [diff] [review]
Remove unnecessary callers of nsView::Invalidate

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

Hurrah!
Attachment #755158 - Flags: review?(roc) → review+
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.
Attachment #755159 - Attachment is obsolete: true
Attachment #755159 - Flags: review?(roc)
Attachment #761888 - Flags: review?(roc)
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
Attachment #761888 - Flags: review?(roc) → review+
Depends on: 849399
Depends on: 1186061
No longer depends on: 1186061
See Also: → 1436056
You need to log in before you can comment on or make changes to this bug.