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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: jrmuizel, Assigned: ttaubert)

Details

(Whiteboard: [sps][snappy:P1])

Attachments

(1 file, 1 obsolete file)

57ms of this is encoding the png. 52ms is glReadPixels reading back the webgl canvas
The easiest thing to fix here would be to encode the png in a separate event from the grabbing the data.
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.
Whiteboard: [sps][snappy]
We filed bug 744100 to do async screenshots
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.
Whiteboard: [sps][snappy] → [sps][snappy:P1]
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?
(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()
(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!
Component: General → Tabbed Browser
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #645872 - Flags: review?(dao)
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
(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 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-
(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 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+
https://hg.mozilla.org/integration/fx-team/rev/966051493a76
Whiteboard: [sps][snappy:P1] → [sps][snappy:P1][fixed-in-fx-team]
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.