u544734 Reporter
	Description • 9 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150721150952

Steps to reproduce:

1. I configured my Nightly to use GTK3, following this page : http://glandium.org/blog/?p=3555
2. I tried to select and drag&drop two paragraphs of a web page


Actual results:

Trying to drag&drop cause Firefox to freeze for about 5 seconds. After this delay, I can drag correctly.

The bigger the amount of text I try to drag, the longer Firefox freezes.


Expected results:

I should be able to move the selected text immediately, instead of waiting up to five seconds.

	Andrew Comminos [:acomminos] Assignee
	Comment 1 • 9 years ago

I can't seem to reproduce with GTK 3.14 on Debian Jessie. Do you mind providing your GTK version and distro (as well as any other information that might be relevant)? Thanks!

	u544734 Reporter
	Comment 2 • 9 years ago

(In reply to Andrew Comminos [:acomminos] from comment #1)
> I can't seem to reproduce with GTK 3.14 on Debian Jessie. Do you mind
> providing your GTK version and distro (as well as any other information that
> might be relevant)? Thanks!

I'm on Ubuntu 15.04, with Unity 7.3.2. My libgtk package is in version 3.14.13. The "About Nightly" window says I have the version 42.0a1 (2015-07-21).

Also, I don't have a graphic card, only Intel's chipset. My "details" screen in the settings says "Intel Haswell Mobile".

	u544734 Reporter
	Comment 3 • 9 years ago

Oh, and I just saw it seems to be particularly visible in the "Reader View". If I select several paragraphs in this view, the freeze can take about 5 seconds.

	Andrew Comminos [:acomminos] Assignee
	Comment 4 • 9 years ago

This looks to be caused by a slower drawing path being used for DND on GTK3. I'll take this.

	Andrew Comminos [:acomminos] Assignee
	Comment 5 • 9 years ago

Looks like the use of GtkOffscreenWindow is causing us to reinitialize nsScreenManagerGtk every frame due to a ConfigureNotify being sent to the root window, which doesn't help.

	Andrew Comminos [:acomminos] Assignee
	Comment 6 • 9 years ago

This patch implements DND alpha pixmap drawing using cairo surfaces on GTK3, included support for device scale.

	Andrew Comminos [:acomminos] Assignee
	Comment 7 • 9 years ago

This patch uses monitors-changed to signal monitor size changes to nsScreenManagerGtk instead of listening to the root window's ConfigureNotify, preventing GtkOffscreenWindow from generating screen manager initialization requests. Added bonus of handling monitor change events on non-X11 targets.

Thanks!

	Karl Tomlinson (:karlt)
	Comment 8 • 9 years ago

Comment on attachment 8638974 [details] [diff] [review]
Use monitors-changed signal to update screen manager on GTK.

(In reply to Andrew Comminos [:acomminos] from comment #5)
> Looks like the use of GtkOffscreenWindow is causing us to reinitialize
> nsScreenManagerGtk every frame due to a ConfigureNotify being sent to the
> root window, which doesn't help.

Do you know why/how GtkOffscreenWindow triggers native ConfigureNotify events
on the root window?

Regardless, this change is good, thanks.

	Karl Tomlinson (:karlt)
	Comment 9 • 9 years ago

Comment on attachment 8638973 [details] [diff] [review]
Draw drag and drop alpha pixmap correctly on GTK3.

Please add a build time check that system cairo is being used here.
Something like this for one of the cairo functions invoked
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/widget/gtk/nsWindow.cpp#l1948

>-    // TODO GTK3
>+    cairo_surface_t *surf = cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
>+                                                       dragRect.width,
>+                                                       dragRect.height);

This should be an x11 pixmap surface.

It will be used to draw to the drag icon window.  It makes more sense to
upload once instead of each time this is drawn, but that may not be a major
issue if the window is not actually redrawn, but just recomposited when moved.

More importantly aSurface may be a pixmap, so drawing to an image surface
would need to read back from the pixmap.  Readback is the number 1 performance
problem to avoid when drawing.

However, what is in this patch should at least give nicer results, even if
there may not be an improvement in performance, so this can land with just a
TODO comment indicating that an X11 Pixmap should be used instead of an image
surface to avoid readback.

	Andrew Comminos [:acomminos] Assignee
	Comment 10 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> Do you know why/how GtkOffscreenWindow triggers native ConfigureNotify events
> on the root window?

This appears to be caused by the root window having the SubstructureNotify event mask set, causing events to be received from the offscreen window (which is a direct child of the root).

(In reply to Karl Tomlinson (ni?:karlt) from comment #9)
> This should be an x11 pixmap surface.
> 
> It will be used to draw to the drag icon window.  It makes more sense to
> upload once instead of each time this is drawn, but that may not be a major
> issue if the window is not actually redrawn, but just recomposited when
> moved.
> 
> More importantly aSurface may be a pixmap, so drawing to an image surface
> would need to read back from the pixmap.  Readback is the number 1
> performance
> problem to avoid when drawing.
> 
> However, what is in this patch should at least give nicer results, even if
> there may not be an improvement in performance, so this can land with just a
> TODO comment indicating that an X11 Pixmap should be used instead of an image
> surface to avoid readback.

Sounds good. I suppose we'd want something similar to what is done in DrawThemeWithCairo in nsNativeThemeGtk, where we attempt to borrow the underlying X drawable or in-memory image. Perhaps it would be worth implementing a shim around tree cairo for mapping to system cairo surfaces. I'll flag these patches for checkin (with the TODO) and look into that.

	Andrew Comminos [:acomminos] Assignee
	Comment 11 • 9 years ago

Updated with tree cairo check and TODO.

	Andrew Comminos [:acomminos] Assignee
	Comment 12 • 9 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bae15f584f6a

	Carsten Book [:Tomcat]
	Comment 13 • 9 years ago

the 2nd patch failed to apply:

renamed 1186661 -> Bug-1186661---Draw-drag-and-drop-alpha-pixmap-corr.patch
applying Bug-1186661---Draw-drag-and-drop-alpha-pixmap-corr.patch
patching file widget/gtk/nsDragService.cpp
Hunk #4 FAILED at 450
1 out of 4 hunks FAILED -- saving rejects to file widget/gtk/nsDragService.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1186661---Draw-drag-and-drop-alpha-pixmap-corr.patch

could you take a look ? Thanks!

	Andrew Comminos [:acomminos] Assignee
	Comment 14 • 9 years ago

Rebased off inbound, should work now. Didn't receive any merge conflicts though.

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d7d24ebf13
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8ffb4d31b5

	Karl Tomlinson (:karlt)
	Comment 16 • 9 years ago

(In reply to Andrew Comminos [:acomminos] from comment #10)
> This appears to be caused by the root window having the SubstructureNotify
> event mask set, causing events to be received from the offscreen window
> (which is a direct child of the root).

Thanks.  Seams less than ideal if each frame creates a new window, but the fix is good anyway, thanks.

> I suppose we'd want something similar to what is done in
> DrawThemeWithCairo in nsNativeThemeGtk, where we attempt to borrow the
> underlying X drawable or in-memory image. Perhaps it would be worth
> implementing a shim around tree cairo for mapping to system cairo surfaces.
> I'll flag these patches for checkin (with the TODO) and look into that.

It's different from nsNativeThemeGtk because DrawTarget (not system cairo) is doing the drawing and the destination surface properties are known.

It can be like the code in the patch here if there is a CreateDrawTargetForPixmap
kind of function.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e3d7d24ebf13
https://hg.mozilla.org/mozilla-central/rev/8a8ffb4d31b5

	Andrew Comminos [:acomminos] Assignee
	Comment 18 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #16)
> (In reply to Andrew Comminos [:acomminos] from comment #10)
> > This appears to be caused by the root window having the SubstructureNotify
> > event mask set, causing events to be received from the offscreen window
> > (which is a direct child of the root).
> 
> Thanks.  Seams less than ideal if each frame creates a new window, but the
> fix is good anyway, thanks.

It's not creating new windows thankfully, the child window (the dragged pixmap) is just having its coordinate updates sent to the root as it's being dragged around as ConfigureNotify events. So it's not every frame (my mistake).

> 
> > I suppose we'd want something similar to what is done in
> > DrawThemeWithCairo in nsNativeThemeGtk, where we attempt to borrow the
> > underlying X drawable or in-memory image. Perhaps it would be worth
> > implementing a shim around tree cairo for mapping to system cairo surfaces.
> > I'll flag these patches for checkin (with the TODO) and look into that.
> 
> It's different from nsNativeThemeGtk because DrawTarget (not system cairo)
> is doing the drawing and the destination surface properties are known.
> 
> It can be like the code in the patch here if there is a
> CreateDrawTargetForPixmap
> kind of function.

Right, thanks. We can't really do the same thing as we need the lifetime of the surface to be managed by GTK.