Closed Bug 918203 Opened 6 years ago Closed 6 years ago

Defect - Several seconds delay before adjusting when transitioning into filled view

Categories

(Firefox for Metro Graveyard :: Browser, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: kjozwiak, Assigned: jimm)

References

()

Details

(Whiteboard: [block28] feature=defect c=Browser_views u=metro_firefox_user p=2)

Attachments

(4 files)

When sliding in another application so that Firefox Metro transitions into filled view, there's a 2-5 second delay before everything in Firefox Metro is aligned correctly for filled view. This is mostly noticeable when you have app bars opened while transitioning into filled view (Navigation App Bar, Find Bar etc..).

- Attached a video to illustrate the issue (notice the delay before Firefox Metro is aligned correctly)

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to wikipedia.org
3) Once the website has loaded, slide in the Navigation App Bar from the bottom of the screen
4) Once the Navigation App Bar is visible, slide in the Desktop from the left side and place it into snapped view. You should notice Firefox Metro go into filled view and after a few seconds, correctly adjust itself)

Current Behavior:

- When transitioning to filled view, there's a few seconds of delay before Firefox Metro is aligned correctly

Expected Behavior:

- This should be very smooth without any delays. This is a recent regression as Firefox Metro transitioned into filled view without any delays.
Whiteboard: feature=defect c=Browser_views u=metro_firefox_user p=0 → [preview-triage] feature=defect c=Browser_views u=metro_firefox_user p=0
Whiteboard: [preview-triage] feature=defect c=Browser_views u=metro_firefox_user p=0 → [preview] feature=defect c=Browser_views u=metro_firefox_user p=0
Blocks: metrov1it16
No longer blocks: metrov1backlog
Whiteboard: [preview] feature=defect c=Browser_views u=metro_firefox_user p=0 → [preview] feature=defect c=Browser_views u=metro_firefox_user p=2
Assignee: nobody → netzen
Status: NEW → ASSIGNED
No longer blocks: MetroPreviewRelease
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [preview] feature=defect c=Browser_views u=metro_firefox_user p=2 → feature=defect c=Browser_views u=metro_firefox_user p=2
Attached patch Patch rev1Splinter Review
This improves things a lot for me. I've been playing around with this all day and this is the best I can get it.
Attachment #815577 - Flags: review?(jmathies)
Comment on attachment 815577 [details] [diff] [review]
Patch rev1

A little out of my scope. Bas might be a better reviewer since he wrote the d3d compositor, or roc.
Attachment #815577 - Flags: review?(jmathies) → review?(bas)
Blocks: metrov1it17
No longer blocks: metrov1it16
Whiteboard: feature=defect c=Browser_views u=metro_firefox_user p=2 → [block28] feature=defect c=Browser_views u=metro_firefox_user p=2
Blocks: metrov1it18
No longer blocks: metrov1it17
Hey Bas, have you had a chance to look at this one? Did you have an alternate suggestion?
Hey Bas, any chance we can get the review this week so we can close the bug before the end of the iteration next Monday?
Flags: needinfo?(bas)
Comment on attachment 815577 [details] [diff] [review]
Patch rev1

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

I don't really feel competent to give an r+ on this. It looks fine to me, but mattwoodrow understands this sort of stuff better than I do.
Attachment #815577 - Flags: review?(matt.woodrow)
Attachment #815577 - Flags: review?(bas)
Attachment #815577 - Flags: review+
Thanks Bas :)
Comment on attachment 815577 [details] [diff] [review]
Patch rev1

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

I'm not actually sure how this works, since it seems like everything it's adding is just repeating work.

How things are supposed to work is:

* The OS sends us a resize event and we mark the view as resized and needing an update (inside mWidgetListener->WindowResized)
* The OS then immediately sends a paint event
* The paint event handler calls WillPaintWindow which sees the resized flag, and calls into layout to repaint layers and get the new ones sent to the compositor
* The paint event handles calls PaintWindow which blocks until the compositor has finished display the paint

Which one of these steps isn't happening correctly?

::: widget/windows/winrt/MetroWidget.cpp
@@ +1175,5 @@
>  
> +  // Do an early async composite so that we at least have something on screen
> +  // in the right place, even if the content is out of date.
> +  if (GetLayerManager()->GetBackendType() == LAYERS_CLIENT && mCompositorParent) {
> +    mCompositorParent->ScheduleRenderOnCompositorThread();

WillPaintWindow should have done the layer updates for the resize, and should have scheduled a composite to display that. This shouldn't be having any effect.

@@ +1389,5 @@
>      mWidgetListener->WindowResized(this, aWidth, aHeight);
>    }
> +
> +  if (mLayerManager) {
> +    FrameLayerBuilder::InvalidateAllLayers(mLayerManager);

We don't really want to invalidate everything on a resize, since some fixed size content can get away without being repainted usually. DLBI should be correctly figuring out what has changed size and what hasn't.

@@ +1393,5 @@
> +    FrameLayerBuilder::InvalidateAllLayers(mLayerManager);
> +  }
> +
> +  Invalidate();
> +  PaintUpdateRgn();

I would expect us to get a WM_PAINT event immediately after this, making this change unnecessary.
One difference in Metro that I suspect is related to this bug happening is that on Win8 for Metro apps you're required to use 2 buffers for the swap chain, calls will fail if you try to use 1:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#l273

Whereas on Desktop we always use 1:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#l308

Thoughts?
Flags: needinfo?(bas)
Blocks: metrov1it19
No longer blocks: metrov1it18
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> One difference in Metro that I suspect is related to this bug happening is
> that on Win8 for Metro apps you're required to use 2 buffers for the swap
> chain, calls will fail if you try to use 1:
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/
> CompositorD3D11.cpp#l273
> 
> Whereas on Desktop we always use 1:
> 
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/
> CompositorD3D11.cpp#l308
> 
> Thoughts?

It's not obvious why that would make a difference to this patch, but I'm not overly familiar with the d3d11 code. CC'ing Nick Cameron who might know more.

I think the best thing to do is figure out which of my list of steps isn't happening. I would guess the second based on the patch.
Attachment #815577 - Flags: review?(matt.woodrow) → review-
I wonder if this could be an issue: http://msdn.microsoft.com/en-us/library/windows/desktop/bb205075%28v=vs.85%29.aspx#Multithread_Considerations

I don't see any difference here between using one or two buffers in the swap chain. Does it mean we have to call Present twice before we see changes? (I don't think so)
> How things are supposed to work is:
> 
> * The OS sends us a resize event and we mark the view as resized and needing
> an update (inside mWidgetListener->WindowResized)
> * The OS then immediately sends a paint event
> * The paint event handler calls WillPaintWindow which sees the resized flag,
> and calls into layout to repaint layers and get the new ones sent to the
> compositor
> * The paint event handles calls PaintWindow which blocks until the
> compositor has finished display the paint

If 2 isn't happening, we could force it with an invalidate call in MetroWidget::Resize.

From debugging I know we don't get a lot of paint requests in widget in this platform.
I'm going to try to fix up resizing over in bug 936984.
k sounds good, please mark this as a dupe or wfm if fixed there :D
Assignee: netzen → nobody
Attached patch invalidate patchSplinter Review
That other bug turned out to be something in apzc. Logging our resize events in widget, everything appear to be correct. Here's the invalidate patch I suggested. If someone can reproduce this bug (I can't on a surface pro) please take this for a spin and see if it helps.
Blocks: metrov1backlog
No longer blocks: metrov1it19
Status: ASSIGNED → NEW
QA Contact: jbecerra
Whiteboard: [block28] feature=defect c=Browser_views u=metro_firefox_user p=2 → [block28] feature=defect c=Browser_views u=metro_firefox_user p=0
Depends on: 937626
Why don't we just add the invalidate call on resize for starters, and see if it improves things.
Attachment #830138 - Flags: review?(netzen)
Comment on attachment 830138 [details] [diff] [review]
invalidate patch

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

I'm a bit surprised this helps (based on trying a bunch of different similar things before), but looks fine to me if it does.
Attachment #830138 - Flags: review?(netzen) → review+
Whiteboard: [block28] feature=defect c=Browser_views u=metro_firefox_user p=0 → [block28][leave-open] feature=defect c=Browser_views u=metro_firefox_user p=0
shouldn't
>    1.12 +  Resize(0, 0, aHeight, aHeight, aRepaint);
read
>    1.12 +  Resize(0, 0, aWidth, aHeight, aRepaint);

?
(In reply to henryfhchan from comment #19)
> shouldn't
> >    1.12 +  Resize(0, 0, aHeight, aHeight, aRepaint);
> read
> >    1.12 +  Resize(0, 0, aWidth, aHeight, aRepaint);
> 
> ?

yeah, sorry, copy paste error.
This improves things further, although it's not perfect. One thing I've notices, the white band you see sometimes depends on the app you drag in. For example, dragging the calender in reproduces pretty well (although not always) while dragging in a Windows Search results page doesn't reproduce.

Anyway, chipping away at this, I think not setting the display port on resize is a bug, so lets get that fixed.
Assignee: nobody → jmathies
Attachment #8336744 - Flags: review?(mbrubeck)
Attachment #8336744 - Flags: review?(mbrubeck) → review+
Blocks: metrov1it19
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: [block28][leave-open] feature=defect c=Browser_views u=metro_firefox_user p=0 → [block28][leave-open] feature=defect c=Browser_views u=metro_firefox_user p=2
No longer blocks: metrov1it19
Please reopen if you're still seeing heavy redraw delays when resizing.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [block28][leave-open] feature=defect c=Browser_views u=metro_firefox_user p=2 → [block28] feature=defect c=Browser_views u=metro_firefox_user p=2
Target Milestone: --- → Firefox 28
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

The transition goes smooth, without any delay on latest nightly (build ID: 20131209053402) using the STR from comment 0.
Kamil, can you confirm this also works for you now?
Flags: needinfo?(kamiljoz)
Went through the following bug for iteration #20 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-15-03-02-02-mozilla-central/

I updated all my OS's to Windows 8.1 so I don't have the original environment this was tested on. Everything looks like it's working with the latest build on Windows 8.1. Repainting is a lot smoother when resizing.

I also went through the following test cases:

- Went through the original issue from comment #0 without any issues (followed the same steps)
- Ensured that everything appeared correctly in the 50/50 view
- Increased/decreased the size of the Firefox Metro view and ensured that the window was being repainted quickly and smoothly
- Swiped in several different Windows Apps and ensured that Firefox Metro is being repainted quickly and smoothly
- Swiped in Calendar, Calculator, Evernote Touch, Facebook Touch, Twitter, Khan Academy without any issues (Firefox Metro was repainted quickly)
- Used the website that was attached by Jim in comment # 23 without any issues (repainted quickly/smoothly when resizing views with that website)
- Had two different Windows Apps including Firefox Metro visible (three in total) on the screen and played around with the resizing. Everything was being repainted quickly and smoothly
- Swiped in different Windows Apps (including Firefox Metro) from the "App Bar" on the left side of the screen and ensured everything was repainted correctly and smoothly

Played around with the resizing and views for about an hour without running into any of the issues from comment #0 (slow repainting)
Flags: needinfo?(kamiljoz)
Thank you Kamil! Marking this issue as verified.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.