Black flashing on resize on OS X with OMTC

RESOLVED FIXED in mozilla24

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: jrmuizel, Assigned: mattwoodrow)

Tracking

unspecified
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
What's the plan here? Enforced sync recompositions during resize-induced drawRect?
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 752091 [details] [diff] [review]
Don't invalidate the widget and do a synchronous composite

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

Updated

5 years ago
Blocks: 756601
(Assignee)

Updated

5 years ago
Depends on: 676241
(Assignee)

Comment 5

5 years ago
Created attachment 752635 [details] [diff] [review]
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.
Attachment #752635 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 755157 [details] [diff] [review]
Add FlushRendering api to compositor

This synchronously waits for the compositor to complete the in-progress or scheduled composite before returning.
Attachment #755157 - Flags: review?(ncameron)
(Assignee)

Comment 8

5 years ago
Created attachment 755158 [details] [diff] [review]
Remove unnecessary callers of nsView::Invalidate

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)
(Assignee)

Comment 9

5 years ago
Created attachment 755159 [details] [diff] [review]
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.
Attachment #755159 - Flags: review?(roc)

Comment 10

5 years ago
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.
(Assignee)

Comment 13

5 years ago
Created attachment 761888 [details] [diff] [review]
Don't invalidate the widget for OTMC v2
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+
(Assignee)

Updated

5 years ago
Depends on: 849399

Updated

3 years ago
Depends on: 1186061
No longer depends on: 1186061

Updated

4 months ago
See Also: → bug 1436056
You need to log in before you can comment on or make changes to this bug.