The default bug view has changed. See this FAQ.

Thumbnail_capture causes 125ms of jank during load of webgl aquarium.

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: ttaubert)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Here's the profile:
http://people.mozilla.com/~bgirard/cleopatra/?report=285fcf510fa6aaeac977e741adcd35b88e6a3710
(Reporter)

Comment 1

5 years ago
57ms of this is encoding the png. 52ms is glReadPixels reading back the webgl canvas
(Reporter)

Comment 2

5 years ago
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.

Updated

5 years ago
Whiteboard: [sps][snappy]

Comment 4

5 years ago
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.

Updated

5 years ago
Whiteboard: [sps][snappy] → [sps][snappy:P1]
(Assignee)

Comment 6

5 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

5 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

5 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

5 years ago
Component: General → Tabbed Browser
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 9

5 years ago
Created attachment 645872 [details] [diff] [review]
simple patch calling canvas.mozFetchAsStream() on the next tick
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #645872 - Flags: review?(dao)

Comment 10

5 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

5 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 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

5 years ago
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.
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+
(Assignee)

Comment 15

5 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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/966051493a76
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.