Closed
Bug 774811
Opened 12 years ago
Closed 12 years ago
Thumbnail_capture causes 125ms of jank during load of webgl aquarium.
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 17
People
(Reporter: jrmuizel, Assigned: ttaubert)
Details
(Whiteboard: [sps][snappy:P1])
Attachments
(1 file, 1 obsolete file)
1.09 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Here's the profile: http://people.mozilla.com/~bgirard/cleopatra/?report=285fcf510fa6aaeac977e741adcd35b88e6a3710
Reporter | ||
Comment 1•12 years ago
|
||
57ms of this is encoding the png. 52ms is glReadPixels reading back the webgl canvas
Reporter | ||
Comment 2•12 years ago
|
||
The easiest thing to fix here would be to encode the png in a separate event from the grabbing the data.
Comment 3•12 years ago
|
||
Maybe we can extract the webgl frame from the last resolved webgl frame. I imagine this might not be something we can do if the screenshoting isn't the same view as the layer widget.
Updated•12 years ago
|
Whiteboard: [sps][snappy]
Comment 4•12 years ago
|
||
We filed bug 744100 to do async screenshots
Comment 5•12 years ago
|
||
I don't think async screenshots is feasible until we can get off main thread painting (something I've been wanting for awhile now) so let's consider some more immediate solutions such as what jeff suggested.
Updated•12 years ago
|
Whiteboard: [sps][snappy] → [sps][snappy:P1]
Assignee | ||
Comment 6•12 years ago
|
||
After canvas.drawWindow() we call canvas.mozFetchAsStream(callback), with the callback invoked on the next tick. The first argument for the callback is an nsIInputStream that we copy to a file using NetUtil.asyncCopy(). I'm not sure we can split this into more steps... The imgIEncoder used by nsHTMLCanvasElement::ExtractData() encodes the data it reads on the fly, right?
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #6) > After canvas.drawWindow() we call canvas.mozFetchAsStream(callback), with > the callback invoked on the next tick. The first argument for the callback > is an nsIInputStream that we copy to a file using NetUtil.asyncCopy(). > > I'm not sure we can split this into more steps... The imgIEncoder used by > nsHTMLCanvasElement::ExtractData() encodes the data it reads on the fly, > right? No. The entire encoding is done in canvas.mozFetchAsStream(). The part to split is canvas.drawWindow() and canvas.mozFetchAsStream()
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #7) > No. The entire encoding is done in canvas.mozFetchAsStream(). The part to > split is canvas.drawWindow() and canvas.mozFetchAsStream() Oops, yeah I get it now :| Thanks!
Assignee | ||
Updated•12 years ago
|
Component: General → Tabbed Browser
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 9•12 years ago
|
||
Comment 10•12 years ago
|
||
Comment on attachment 645872 [details] [diff] [review] simple patch calling canvas.mozFetchAsStream() on the next tick Note this changes meaning of the fx_thumbnails_capture_time_ms histogram so you should remove it...though you can do that in a followup bug once you verify that this patch does significantly reduce pauses
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Taras Glek (:taras) from comment #10) > Note this changes meaning of the fx_thumbnails_capture_time_ms histogram so > you should remove it...though you can do that in a followup bug once you > verify that this patch does significantly reduce pauses Actually, it doesn't. We've always measured the capture_time_ms before we start encoding and fetching data.
Comment 12•12 years ago
|
||
Comment on attachment 645872 [details] [diff] [review] simple patch calling canvas.mozFetchAsStream() on the next tick >- canvas.mozFetchAsStream(aCallback, this.contentType); >+ // Fetch the canvas data on the next event loop tick. >+ Services.tm.currentThread.dispatch(function () { >+ canvas.mozFetchAsStream(aCallback, this.contentType); >+ }, Ci.nsIThread.DISPATCH_NORMAL); Looks like this.contentType isn't going to exist in that function.
Attachment #645872 -
Flags: review?(dao) → review-
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #12) > >+ // Fetch the canvas data on the next event loop tick. > >+ Services.tm.currentThread.dispatch(function () { > >+ canvas.mozFetchAsStream(aCallback, this.contentType); > >+ }, Ci.nsIThread.DISPATCH_NORMAL); > > Looks like this.contentType isn't going to exist in that function. Oops, thanks for catching that. The tests didn't complain about it, probably because they're just falling back to PNG as the default.
Attachment #645872 -
Attachment is obsolete: true
Attachment #646040 -
Flags: review?(dao)
Comment 14•12 years ago
|
||
Comment on attachment 646040 [details] [diff] [review] simple patch calling canvas.mozFetchAsStream() on the next tick >+ // Fetch the canvas data on the next event loop tick. Add some explanation here ("...on the next event loop tick because...", "...on the next event loop tick in order to..."). A reference to bug 744100 might be handy too. >+ let contentType = this.contentType; >+ Services.tm.currentThread.dispatch(function () { >+ canvas.mozFetchAsStream(aCallback, contentType); >+ }, Ci.nsIThread.DISPATCH_NORMAL); Services.tm.currentThread.dispatch(function () { canvas.mozFetchAsStream(aCallback, this.contentType); }.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
Attachment #646040 -
Flags: review?(dao) → review+
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/966051493a76
Whiteboard: [sps][snappy:P1] → [sps][snappy:P1][fixed-in-fx-team]
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/966051493a76
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sps][snappy:P1][fixed-in-fx-team] → [sps][snappy:P1]
Target Milestone: --- → Firefox 17
You need to log in
before you can comment on or make changes to this bug.
Description
•