Closed Bug 1167215 Opened 5 years ago Closed 4 years ago

video doesn't invalidate after dragging tab out to new window

Categories

(Core :: Graphics: Layers, defect, P2)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
e10s m8+ ---
firefox41 --- wontfix
firefox42 --- verified

People

(Reporter: blassey, Assigned: bholley)

References

Details

Attachments

(2 files)

I was just invited to a hello call, clicked on the link and it opened in a new tab. I then dragged that tab out into its own window. At that point both my video preview and the remote video started playing at one frame every few seconds. Audio continued working fine.

After a while I left that call and rejoined by right clicking on the link and opening it in a new window. Performance was fine.
I was able to reproduce this on Firefox Dev Edition (40) as well. I'm on Mac OS.

I also noticed that if I placed the tab back onto the window from which it was detached, I would get the spinner and it never shows the content.
Priority: -- → P2
For an easier reproduction of the issue (which seems to arise only when e10s is turned on):

1. Open a new tab.
2. In the new tab, go to http://mozilla.github.io/webrtc-landing/gum_test.html
3. Select "video", and grant access to the camera.
4. Drag the tab out of the browser window.

You'll observe that video performance plummets from a smooth experience to approximately one frame every two seconds.
Summary: terrible video performance on hello call after dragging tab out to new window → terrible camera capture performance call after dragging tab out to new window
Summary: terrible camera capture performance call after dragging tab out to new window → terrible camera capture performance after dragging tab out to new window
Verified on Linux64 e10s as well.  I note I get no frames unless I click on a window frame/top-bar, then I get one frame.  Also: dragging it back into the original window gets infinite-spinner.  It will also update if I hover/cross tab 'x' boxes, etc.

I suspect a compositor issue... and maybe more.

I don't think this has anything to do with WebRTC per se. 

I have similar (if not identical) issues with http://video.webmfiles.org/big-buck-bunny_trailer.webm (with that, video freezes but plays if and only if the mouse is over it).  And it also infinite-spins on drag-back

-> Video/Audio; may need to go to the Compositor
Component: WebRTC: Audio/Video → Video/Audio
Assignee: nobody → bobbyholley
Component: Video/Audio → DOM
Summary: terrible camera capture performance after dragging tab out to new window → video doesn't invalidate after dragging tab out to new window
STR:

1) open the video in a tab and let it start playing
2) drag the video tab out to a new window

result: audio plays but video doesn't update. 

3) drag your mouse across the video pane in the tab

result: the video advances to the right position and plays

4) stop moving mouse over video pane

result: video stops playing while audio continues
OS: Unspecified → All
I'm pretty close to having a handle on what's going on here, need to run some experiments and confirm.

Here's an IRC log discussing it with mattwoodrow and roc: https://pastebin.mozilla.org/8841297
So, the issue here is actually different than the one suspected in the pastebin above (though we should get a patch in for that issue as well).

The problem is that our CompositeAgainAt call happens in ImageHost::Composite, which gets skipped when we don't need to composite anything. Since "CompositeAgainAt" really just causes us to be called back at the next vsync, and since the vsync is much faster than the frame rate, we end up scheduling one no-op composite, and then never scheduling another one.

The reason this only affects background tabs is that something else seems to be causing us to Composite the main window on every vsync already. I'm not sure what, but unless that's weird I'm not going to investigate it further.

Given how we do this, it seems we need to track the latest CompositeAgain time (rather than the earliest), and composite on every vsync until we hit that. I'll see if I can work up a patch.
Patch almost works, but not quite. I'll investigate more tomorrow.
The issue was that, when the client sends updated samples with adjusted timestamps (due to resyncing with the audio clock), the timestamp of the next sample may change, so our old CompositeUntil() might not be valid anymore. I've fixed that in an additional patch.

Everything seems to work now.
Component: DOM → Graphics: Layers
Blocks: 1190117
Comment on attachment 8642103 [details] [diff] [review]
Part 1 - Composite on every vsync until the scheduled one. v1

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

Excellent. We'll need this to get this on the branches where bug 1143575 landed.
Attachment #8642103 - Flags: review?(roc) → review+
Keywords: regression
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 8642103 [details] [diff] [review]
> Part 1 - Composite on every vsync until the scheduled one. v1
> 
> Review of attachment 8642103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent. We'll need this to get this on the branches where bug 1143575
> landed.

That's just trunk, right? Or am I missing something?

Oddly though, this behavior was reproducible during the e10s triage at whistler, which was before bug 1143575 landed. So it must have existed (in different form) before.
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/6d2e3f71205b
https://hg.mozilla.org/mozilla-central/rev/e3b8e08187ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Brad, this should be fixed in today's Nightly. Can you please check?
Flags: needinfo?(blassey.bugs)
tested with http://mozilla.github.io/webrtc-landing/gum_test.html and this works in nightly now (and doesn't work in dev edition)
Flags: needinfo?(blassey.bugs)
Thanks for confirming this Brad. Can we get this nominated for uplift?
Status: RESOLVED → VERIFIED
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #18)
> Thanks for confirming this Brad. Can we get this nominated for uplift?

This patch is on top of bug 1143575, and would need to be rewritten to be uplifted. Given that the trains depart in six days and e10s isn't riding to Beta, why do we need to uplift it?
I'm not qualified to decide whether it *needs* uplift. I guess I chose my words poorly, I was meaning to ask if this needed to get uplifted since Brad marked Firefox 41 as affected. I suppose based on your comment that this shouldn't be uplifted and therefore should be marked wontfix for Firefox 41.
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #20)
> I'm not qualified to decide whether it *needs* uplift. I guess I chose my
> words poorly, I was meaning to ask if this needed to get uplifted since Brad
> marked Firefox 41 as affected. I suppose based on your comment that this
> shouldn't be uplifted and therefore should be marked wontfix for Firefox 41.

Sounds good - thanks! :-)
You need to log in before you can comment on or make changes to this bug.