Closed Bug 1436190 Opened 2 years ago Closed 2 years ago
Hang/crash on WR capturing with Ctrl+Shift+3
Getting a WR capture from the nightly has the correct keys but behaves undesirably by hanging on Windows and crashing on Linux. The problem appears to come from the recent change in `RenderNotifier` logic.
No need to increase the pending count (any more!) since the notification coming from the capture has `composite_needed: false`. Tested on Windows so far.
Hmm, interesting. Linux is hitting assertion "info.mPendingCount > 0" with the patch. I wonder why the behavior is different on Windows.
The devtools component is for stuff related to the debugger, inspector and other built-in tools for web developers. About the frame counting issue, Sotaro is probably the only person who fully understands the ins and outs of it and I don't have an idea off the top of my head. It's not uncommon for this type of issue to happen a lot more on a platform than another because it is timing sensitive (different test machines and other factors).
Component: Developer Tools → Graphics: WebRender
Product: Firefox → Core
Sotaro, would you know why on Linux we have to increase the pending count (or otherwise hitting an assert) while on Windows we get a hang if we don't increase it?
2 years ago
Priority: -- → P2
Ok, let's try again. What I found is that FF expected a frame coming after `wake_up` call, which is incorrect, and that's what was bugging the capturing. I'm yet to test this on anything but Linux, but it should be ready for review.
Now tested (positively) on Windows 7 and Linux. Also fixes Bug 1433932.
(In reply to Dzmitry Malyshau [:kvark] from comment #4) > Sotaro, would you know why on Linux we have to increase the pending count > (or otherwise hitting an assert) while on Windows we get a hang if we don't > increase it? You already seems to address the problem. RenderThread::IncPendingFrameCount() is used to throttle WebRender's frame generation, since ideally WebRender's rendering should happen only after VSync, though actual throttling implementation relaxes a bit. If we tries to generate more frames, some of them are not rendered to Display and WebRender just spend more cpu/gpu resources and the more tasks just add more latency. WebRenderBridgeParent expects that all frame generation happens by CompositeToTarget() like the following sequence. WebRenderBridgeParent::CompositeToTarget() ->wr::RenderThread::IncPendingFrameCount(mApi->GetId()); // inc pending frame count ->txn.GenerateFrame(); // Add generate frame transaction. ->mApi->SendTransaction(txn); ->// ******** WebRender handle transaction ********** ->new_document_ready() // call back from WebRender ->NewFrameReady() ->RenderThread::IncRenderingFrameCount(aWindowId); ->RenderThread::NewFrameReady(mozilla::wr::WindowId(aWindowId)); ->RendererOGL::UpdateAndRender() // render frame ->RenderThread::DecPendingFrameCount() Current implementation wake_up(&self) calls wr_notifier_new_frame_ready(self.window_id). The call breaks the way of managing PendingFrameCount. Then RenderThread::IncPendingFrameCount() in WebRenderAPI::Capture() also break PendingFrameCount. And attachment 8949283 [details] [diff] [review] addresses PendingFrameCount handling breaks.
Attachment #8949283 - Flags: review?(nical.bugzilla) → review+
You should really split the patch into two and put the revision.txt changes on bug 1433932. In mozilla-central the convention is to not lump together fixes for unrelated bugs into the same commit.
The same patch but without the path checking (moving to bug 1433932).
Comment on attachment 8949360 [details] [diff] [review] capture-notify.patch Review of attachment 8949360 [details] [diff] [review]: ----------------------------------------------------------------- You can carry nical's r+ since he already reviewed it.
Attachment #8949360 - Flags: review?(bugmail) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c590c7dac39 Fixed render frame notification in wake_up(). r=nical
You need to log in before you can comment on or make changes to this bug.