Closed Bug 1186661 Opened 4 years ago Closed 4 years ago

Drag&drop freezes Firefox/GTK3 for several seconds

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: u544734, Assigned: acomminos)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Blocks: gtk3
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
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!
Flags: needinfo?(somebody.mozfr)
(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".
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.
Flags: needinfo?(somebody.mozfr)
Status: UNCONFIRMED → NEW
Ever confirmed: true
This looks to be caused by a slower drawing path being used for DND on GTK3. I'll take this.
Assignee: nobody → acomminos
Status: NEW → ASSIGNED
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.
This patch implements DND alpha pixmap drawing using cairo surfaces on GTK3, included support for device scale.
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!
Attachment #8638973 - Flags: review?(karlt)
Attachment #8638974 - Flags: review?(karlt)
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.
Attachment #8638974 - Flags: review?(karlt) → review+
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.
Attachment #8638973 - Flags: review?(karlt) → review+
(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.
Updated with tree cairo check and TODO.
Attachment #8638973 - Attachment is obsolete: true
Attachment #8639345 - Flags: review+
Keywords: checkin-needed
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
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!
Flags: needinfo?(acomminos)
Keywords: checkin-needed
Rebased off inbound, should work now. Didn't receive any merge conflicts though.
Attachment #8639345 - Attachment is obsolete: true
Attachment #8639852 - Flags: review+
Flags: needinfo?(acomminos)
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/e3d7d24ebf13
https://hg.mozilla.org/mozilla-central/rev/8a8ffb4d31b5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(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.
Depends on: 1249604
You need to log in before you can comment on or make changes to this bug.