Last Comment Bug 774811 - Thumbnail_capture causes 125ms of jank during load of webgl aquarium.
: Thumbnail_capture causes 125ms of jank during load of webgl aquarium.
Status: RESOLVED FIXED
[sps][snappy:P1]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 12:20 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-07-27 09:58 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple patch calling canvas.mozFetchAsStream() on the next tick (1.05 KB, patch)
2012-07-25 13:36 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
simple patch calling canvas.mozFetchAsStream() on the next tick (1.09 KB, patch)
2012-07-25 23:14 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-07-17 12:20:51 PDT
Here's the profile:
http://people.mozilla.com/~bgirard/cleopatra/?report=285fcf510fa6aaeac977e741adcd35b88e6a3710
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-07-17 12:23:53 PDT
57ms of this is encoding the png. 52ms is glReadPixels reading back the webgl canvas
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-07-17 12:24:48 PDT
The easiest thing to fix here would be to encode the png in a separate event from the grabbing the data.
Comment 3 Benoit Girard (:BenWa) 2012-07-17 12:34:17 PDT
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.
Comment 4 (dormant account) 2012-07-17 13:54:10 PDT
We filed bug 744100 to do async screenshots
Comment 5 Benoit Girard (:BenWa) 2012-07-17 14:41:02 PDT
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.
Comment 6 Tim Taubert [:ttaubert] 2012-07-25 12:08:14 PDT
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?
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-07-25 12:30:50 PDT
(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()
Comment 8 Tim Taubert [:ttaubert] 2012-07-25 12:31:47 PDT
(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!
Comment 9 Tim Taubert [:ttaubert] 2012-07-25 13:36:07 PDT
Created attachment 645872 [details] [diff] [review]
simple patch calling canvas.mozFetchAsStream() on the next tick
Comment 10 (dormant account) 2012-07-25 14:13:02 PDT
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
Comment 11 Tim Taubert [:ttaubert] 2012-07-25 14:17:41 PDT
(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 Dão Gottwald [:dao] 2012-07-25 19:08:27 PDT
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.
Comment 13 Tim Taubert [:ttaubert] 2012-07-25 23:14:40 PDT
Created attachment 646040 [details] [diff] [review]
simple patch calling canvas.mozFetchAsStream() on the next tick

(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.
Comment 14 Dão Gottwald [:dao] 2012-07-26 02:47:29 PDT
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);
Comment 15 Tim Taubert [:ttaubert] 2012-07-26 05:01:10 PDT
https://hg.mozilla.org/integration/fx-team/rev/966051493a76
Comment 16 Tim Taubert [:ttaubert] 2012-07-27 09:58:45 PDT
https://hg.mozilla.org/mozilla-central/rev/966051493a76

Note You need to log in before you can comment on or make changes to this bug.